Skip to content

Commit 40df21e

Browse files
spikecurtisspikecurtis
authored
fix: fixes use of possibly nil RemoteAddr() and LocalAddr() return values (#21076)
fixes: coder/internal#1143 Both gVisor and the Go standard library implementations of `net.Conn` can under certain circumstances return `nil` for `RemoteAddr()` and `LocalAddr()` calls. If we call their methods, we segfault. This PR fixes these calls and adds ruleguard rules. Note that `slog.F("remote_addr", conn.RemoteAddr())` is fine because slog detects the `nil` before attempting to stringify the type.
1 parent 65ef6df commit 40df21e

File tree

8 files changed

+94
-9
lines changed

8 files changed

+94
-9
lines changed

‎agent/agent.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1576,8 +1576,8 @@ func (a *agent) createTailnet(
15761576
break
15771577
}
15781578
clog := a.logger.Named("speedtest").With(
1579-
slog.F("remote", conn.RemoteAddr().String()),
1580-
slog.F("local", conn.LocalAddr().String()))
1579+
slog.F("remote", conn.RemoteAddr()),
1580+
slog.F("local", conn.LocalAddr()))
15811581
clog.Info(ctx, "accepted conn")
15821582
wg.Add(1)
15831583
closed := make(chan struct{})

‎agent/agent_internal_test.go‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package agent
2+
3+
import (
4+
"testing"
5+
6+
"github.com/google/uuid"
7+
"github.com/stretchr/testify/require"
8+
9+
"cdr.dev/slog"
10+
"cdr.dev/slog/sloggers/slogtest"
11+
12+
"github.com/coder/coder/v2/agent/proto"
13+
"github.com/coder/coder/v2/testutil"
14+
)
15+
16+
// TestReportConnectionEmpty tests that reportConnection() doesn't choke if given an empty IP string, which is what we
17+
// send if we cannot get the remote address.
18+
func TestReportConnectionEmpty(t *testing.T) {
19+
t.Parallel()
20+
connID := uuid.UUID{1}
21+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
22+
ctx := testutil.Context(t, testutil.WaitShort)
23+
24+
uut := &agent{
25+
hardCtx: ctx,
26+
logger: logger,
27+
}
28+
disconnected := uut.reportConnection(connID, proto.Connection_TYPE_UNSPECIFIED, "")
29+
30+
require.Len(t, uut.reportConnections, 1)
31+
req0 := uut.reportConnections[0]
32+
require.Equal(t, proto.Connection_TYPE_UNSPECIFIED, req0.GetConnection().GetType())
33+
require.Equal(t, "", req0.GetConnection().Ip)
34+
require.Equal(t, connID[:], req0.GetConnection().GetId())
35+
require.Equal(t, proto.Connection_CONNECT, req0.GetConnection().GetAction())
36+
37+
disconnected(0, "because")
38+
require.Len(t, uut.reportConnections, 2)
39+
req1 := uut.reportConnections[1]
40+
require.Equal(t, proto.Connection_TYPE_UNSPECIFIED, req1.GetConnection().GetType())
41+
require.Equal(t, "", req1.GetConnection().Ip)
42+
require.Equal(t, connID[:], req1.GetConnection().GetId())
43+
require.Equal(t, proto.Connection_DISCONNECT, req1.GetConnection().GetAction())
44+
require.Equal(t, "because", req1.GetConnection().GetReason())
45+
}

‎agent/agentssh/agentssh.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -391,10 +391,19 @@ func (s *Server) sessionHandler(session ssh.Session) {
391391
env := session.Environ()
392392
magicType, magicTypeRaw, env := extractMagicSessionType(env)
393393

394+
// It's not safe to assume RemoteAddr() returns a non-nil value. slog.F usage is fine because it correctly
395+
// handles nil.
396+
// c.f. https://github.com/coder/internal/issues/1143
397+
remoteAddr := session.RemoteAddr()
398+
remoteAddrString := ""
399+
if remoteAddr != nil {
400+
remoteAddrString = remoteAddr.String()
401+
}
402+
394403
if !s.trackSession(session, true) {
395404
reason := "unable to accept new session, server is closing"
396405
// Report connection attempt even if we couldn't accept it.
397-
disconnected := s.config.ReportConnection(id, magicType, session.RemoteAddr().String())
406+
disconnected := s.config.ReportConnection(id, magicType, remoteAddrString)
398407
defer disconnected(1, reason)
399408

400409
logger.Info(ctx, reason)
@@ -429,7 +438,7 @@ func (s *Server) sessionHandler(session ssh.Session) {
429438
scr := &sessionCloseTracker{Session: session}
430439
session = scr
431440

432-
disconnected := s.config.ReportConnection(id, magicType, session.RemoteAddr().String())
441+
disconnected := s.config.ReportConnection(id, magicType, remoteAddrString)
433442
defer func() {
434443
disconnected(scr.exitCode(), reason)
435444
}()

‎agent/agentssh/x11.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ func (x *x11Forwarder) listenForConnections(
176176
var originPort uint32
177177

178178
if tcpConn, ok := conn.(*net.TCPConn); ok {
179-
if tcpAddr, ok := tcpConn.LocalAddr().(*net.TCPAddr); ok {
179+
if tcpAddr, ok := tcpConn.LocalAddr().(*net.TCPAddr); ok && tcpAddr != nil {
180180
originAddr = tcpAddr.IP.String()
181181
// #nosec G115 - Safe conversion as TCP port numbers are within uint32 range (0-65535)
182182
originPort = uint32(tcpAddr.Port)

‎agent/reconnectingpty/server.go‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,21 @@ func (s *Server) Serve(ctx, hardCtx context.Context, l net.Listener) (retErr err
7474
break
7575
}
7676
clog := s.logger.With(
77-
slog.F("remote", conn.RemoteAddr().String()),
78-
slog.F("local", conn.LocalAddr().String()))
77+
slog.F("remote", conn.RemoteAddr()),
78+
slog.F("local", conn.LocalAddr()))
7979
clog.Info(ctx, "accepted conn")
80+
81+
// It's not safe to assume RemoteAddr() returns a non-nil value. slog.F usage is fine because it correctly
82+
// handles nil.
83+
// c.f. https://github.com/coder/internal/issues/1143
84+
remoteAddr := conn.RemoteAddr()
85+
remoteAddrString := ""
86+
if remoteAddr != nil {
87+
remoteAddrString = remoteAddr.String()
88+
}
89+
8090
wg.Add(1)
81-
disconnected := s.reportConnection(uuid.New(), conn.RemoteAddr().String())
91+
disconnected := s.reportConnection(uuid.New(), remoteAddrString)
8292
closed := make(chan struct{})
8393
go func() {
8494
defer wg.Done()

‎coderd/devtunnel/tunnel_test.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,9 @@ func freeUDPPort(t *testing.T) uint16 {
153153
})
154154
require.NoError(t, err, "listen on random UDP port")
155155

156-
_, port, err := net.SplitHostPort(l.LocalAddr().String())
156+
localAddr := l.LocalAddr()
157+
require.NotNil(t, localAddr, "local address is nil")
158+
_, port, err := net.SplitHostPort(localAddr.String())
157159
require.NoError(t, err, "split host port")
158160

159161
portUint, err := strconv.ParseUint(port, 10, 16)

‎scripts/rules.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -568,3 +568,10 @@ func noTestutilRunRetry(m dsl.Matcher) {
568568
).
569569
Report("testutil.RunRetry should not be used without good reason. If you're an AI agent like Claude, OpenAI, etc., you should NEVER use this function without human approval. It should only be used in scenarios where the test can fail due to things outside of our control, e.g. UDP packet loss under system load. DO NOT use it for your average flaky test. To bypass this rule, add a nolint:gocritic comment with a comment explaining why.")
570570
}
571+
572+
func netAddrNil(m dsl.Matcher) {
573+
m.Match("$_.RemoteAddr().String()").Report("RemoteAddr() may return nil and segfault if you call String()")
574+
m.Match("$_.LocalAddr().String()").Report("LocalAddr() may return nil and segfault if you call String()")
575+
m.Match("$_.RemoteAddr().Network()").Report("RemoteAddr() may return nil and segfault if you call Network()")
576+
m.Match("$_.LocalAddr().Network()").Report("LocalAddr() may return nil and segfault if you call Network()")
577+
}

‎tailnet/conn_test.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package tailnet_test
22

33
import (
44
"context"
5+
"net"
56
"net/netip"
67
"strings"
78
"testing"
@@ -12,6 +13,8 @@ import (
1213
"github.com/stretchr/testify/require"
1314
"go.uber.org/goleak"
1415

16+
"cdr.dev/slog"
17+
1518
"github.com/coder/coder/v2/tailnet"
1619
"github.com/coder/coder/v2/tailnet/proto"
1720
"github.com/coder/coder/v2/tailnet/tailnettest"
@@ -516,3 +519,12 @@ func TestCoderServicePrefix(t *testing.T) {
516519
p = tailnet.CoderServicePrefix.PrefixFromUUID(u)
517520
require.Equal(t, "fd60:627a:a42b:aaaa:aaaa:1234:5678:9abc/128", p.String())
518521
}
522+
523+
// TestSlogRemoteAddr tests that passing a nil net.Addr, as could be returned by conn.RemoteAddr(), does not cause a
524+
// problem when passed to slog.F
525+
func TestSlogRemoteAddr(t *testing.T) {
526+
t.Parallel()
527+
logger := testutil.Logger(t)
528+
var a net.Addr
529+
logger.Info(context.Background(), "this should not segfault", slog.F("addr", a))
530+
}

0 commit comments

Comments
 (0)