Skip to content

Commit 35c6117

Browse files
spikecurtisspikecurtis
committed
Fix segfault
1 parent c820eee commit 35c6117

File tree

2 files changed

+33
-21
lines changed

2 files changed

+33
-21
lines changed

‎coderd/devtunnel/tunnel.go

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ func NewWithConfig(ctx context.Context, logger slog.Logger, cfg Config) (*Tunnel
6060

6161
tun, tnet, err := netstack.CreateNetTUN(
6262
[]netip.Addr{server.ClientIP},
63-
[]netip.Addr{},
63+
[]netip.Addr{netip.AddrFrom4([4]byte{1, 1, 1, 1})},
6464
1280,
6565
)
6666
if err != nil {
@@ -71,17 +71,23 @@ func NewWithConfig(ctx context.Context, logger slog.Logger, cfg Config) (*Tunnel
7171
if err != nil {
7272
return nil, nil, xerrors.Errorf("resolve endpoint: %w", err)
7373
}
74+
// In IPv6, we need to enclose the address to in [] before passing to wireguard's endpoint key, like
75+
// [2001:abcd::1]:8888. We'll use netip.AddrPort to correctly handle this.
76+
wgAddr, err := netip.ParseAddr(wgip.String())
77+
if err != nil {
78+
return nil, nil, xerrors.Errorf("parse address: %w", err)
79+
}
80+
wgEndpoint := netip.AddrPortFrom(wgAddr, cfg.Tunnel.WireguardPort)
7481

75-
dev := device.NewDevice(tun, conn.NewDefaultBind(), device.NewLogger(device.LogLevelSilent, ""))
82+
dev := device.NewDevice(tun, conn.NewDefaultBind(), device.NewLogger(device.LogLevelError, "devtunnel "))
7683
err = dev.IpcSet(fmt.Sprintf(`private_key=%s
7784
public_key=%s
78-
endpoint=[%s]:%d
85+
endpoint=%s
7986
persistent_keepalive_interval=21
8087
allowed_ip=%s/128`,
8188
hex.EncodeToString(cfg.PrivateKey[:]),
8289
server.ServerPublicKey,
83-
wgip.IP.String(),
84-
cfg.Tunnel.WireguardPort,
90+
wgEndpoint.String(),
8591
server.ServerIP.String(),
8692
))
8793
if err != nil {
@@ -103,6 +109,9 @@ allowed_ip=%s/128`,
103109
select {
104110
case <-ctx.Done():
105111
_ = wgListen.Close()
112+
// We need to remove peers before closing to avoid a race condition between dev.Close() and the peer
113+
// goroutines which results in segfault.
114+
dev.RemoveAllPeers()
106115
dev.Close()
107116
<-routineEnd
108117
close(ch)

‎coderd/devtunnel/tunnel_test.go

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import (
1414
"testing"
1515
"time"
1616

17+
"cdr.dev/slog"
18+
1719
"github.com/stretchr/testify/assert"
1820
"github.com/stretchr/testify/require"
1921
"golang.zx2c4.com/wireguard/conn"
@@ -25,13 +27,25 @@ import (
2527
"github.com/coder/coder/coderd/devtunnel"
2628
)
2729

30+
const (
31+
ipByte1 = 0xfc
32+
ipByte2 = 0xca
33+
wgPort = 48732
34+
)
35+
36+
var (
37+
serverIP = netip.AddrFrom16([16]byte{ipByte1, ipByte2, 15: 0x1})
38+
dnsIP = netip.AddrFrom4([4]byte{1, 1, 1, 1})
39+
clientIP = netip.AddrFrom16([16]byte{ipByte1, ipByte2, 15: 0x2})
40+
)
41+
2842
// The tunnel leaks a few goroutines that aren't impactful to production scenarios.
2943
// func TestMain(m *testing.M) {
3044
// goleak.VerifyTestMain(m)
3145
// }
3246

3347
// TestTunnel cannot run in parallel because we hardcode the UDP port used by the wireguard server.
34-
// nolint: tparallel
48+
// nolint: paralleltest
3549
func TestTunnel(t *testing.T) {
3650
ctx, cancelTun := context.WithCancel(context.Background())
3751
defer cancelTun()
@@ -51,7 +65,7 @@ func TestTunnel(t *testing.T) {
5165
fTunServer := newFakeTunnelServer(t)
5266
cfg := fTunServer.config()
5367

54-
tun, errCh, err := devtunnel.NewWithConfig(ctx, slogtest.Make(t, nil), cfg)
68+
tun, errCh, err := devtunnel.NewWithConfig(ctx, slogtest.Make(t, nil).Leveled(slog.LevelDebug), cfg)
5569
require.NoError(t, err)
5670
t.Log(tun.URL)
5771

@@ -99,18 +113,6 @@ type fakeTunnelServer struct {
99113
server *httptest.Server
100114
}
101115

102-
const (
103-
ipByte1 = 0xfc
104-
ipByte2 = 0xca
105-
wgPort = 48732
106-
)
107-
108-
var (
109-
serverIP = netip.AddrFrom16([16]byte{ipByte1, ipByte2, 15: 0x1})
110-
dnsIP = netip.AddrFrom4([4]byte{1, 1, 1, 1})
111-
clientIP = netip.AddrFrom16([16]byte{ipByte1, ipByte2, 15: 0x2})
112-
)
113-
114116
func newFakeTunnelServer(t *testing.T) *fakeTunnelServer {
115117
priv, err := wgtypes.GeneratePrivateKey()
116118
privBytes := [32]byte(priv)
@@ -123,14 +125,15 @@ func newFakeTunnelServer(t *testing.T) *fakeTunnelServer {
123125
1280,
124126
)
125127
require.NoError(t, err)
126-
dev := device.NewDevice(tun, conn.NewDefaultBind(), device.NewLogger(device.LogLevelVerbose, ""))
128+
dev := device.NewDevice(tun, conn.NewDefaultBind(), device.NewLogger(device.LogLevelVerbose, "server "))
127129
err = dev.IpcSet(fmt.Sprintf(`private_key=%s
128130
listen_port=%d`,
129131
hex.EncodeToString(privBytes[:]),
130132
wgPort,
131133
))
132134
require.NoError(t, err)
133135
t.Cleanup(func() {
136+
dev.RemoveAllPeers()
134137
dev.Close()
135138
})
136139

@@ -193,7 +196,7 @@ allowed_ip=%s/128`,
193196
PublicKey: device.NoisePublicKey(pub),
194197
Tunnel: devtunnel.Node{
195198
HostnameHTTPS: strings.TrimPrefix(f.server.URL, "https://"),
196-
HostnameWireguard: "::1",
199+
HostnameWireguard: "localhost",
197200
WireguardPort: wgPort,
198201
},
199202
HTTPClient: f.server.Client(),

0 commit comments

Comments
 (0)