From b7d807b464d9fe22dc91580096205dadcbf410c8 Mon Sep 17 00:00:00 2001 From: ginuerzh Date: Sat, 20 Jun 2026 13:52:09 +0800 Subject: [PATCH] fix(wrapper): prevent nil-pointer dereference in WrapUDPConn chain when stats is nil MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stats.WrapUDPConn returned nil when pStats was nil (introduced in 7c95d40), but callers in the kcp and rudp listeners passed the result directly to admission.WrapUDPConn and limiter.WrapUDPConn, which wrapped a nil PacketConn. When service.Serve() called conn.LocalAddr(), the auto-generated method delegated to the nil PacketConn, causing a panic. Root cause: WrapUDPConn behavior was inconsistent with WrapConn and WrapPacketConn in the same package, which both return the original connection instead of nil when the optional config is absent. Fixes: 1. listener/kcp, listener/rudp — guard stats.WrapUDPConn with Stats != nil (consistent with quic and wt listeners which already had this guard). 2. observer/stats/wrapper — when pStats is nil, pass through the original connection instead of returning nil (matches WrapConn/WrapPacketConn). 3. admission/wrapper, limiter/traffic/wrapper — add defensive nil guards: nil pc → return nil; nil config → pass through original connection. All four WrapUDPConn implementations now share the same nil semantics. 4. listener/udp_wrapper_safety_test.go — regression test that verifies the full metrics→stats→admission→limiter chain with all optional configs nil does not produce a wrapper with a nil internal PacketConn. Fixes: go-gost/gost#873 --- admission/wrapper/conn.go | 14 ++++- limiter/traffic/wrapper/conn.go | 14 ++++- listener/kcp/listener.go | 4 +- listener/rudp/listener.go | 4 +- listener/udp_wrapper_safety_test.go | 97 +++++++++++++++++++++++++++++ observer/stats/wrapper/conn.go | 14 ++++- 6 files changed, 138 insertions(+), 9 deletions(-) create mode 100644 listener/udp_wrapper_safety_test.go diff --git a/admission/wrapper/conn.go b/admission/wrapper/conn.go index a77e95a6..c349beb2 100644 --- a/admission/wrapper/conn.go +++ b/admission/wrapper/conn.go @@ -163,10 +163,20 @@ type udpConn struct { // WrapUDPConn wraps a net.PacketConn as a udp.Conn with admission control. // This is used in UDP forwarding paths where the connection is treated // as a connected UDP socket. -func WrapUDPConn(admission admission.Admission, pc net.PacketConn) udp.Conn { +// If pc is nil, nil is returned. If admission is nil, the original connection +// is returned unchanged (no-op). +func WrapUDPConn(adm admission.Admission, pc net.PacketConn) udp.Conn { + if pc == nil { + return nil + } + if adm == nil { + if uc, ok := pc.(udp.Conn); ok { + return uc + } + } return &udpConn{ PacketConn: pc, - admission: admission, + admission: adm, } } diff --git a/limiter/traffic/wrapper/conn.go b/limiter/traffic/wrapper/conn.go index a96b2555..429c472c 100644 --- a/limiter/traffic/wrapper/conn.go +++ b/limiter/traffic/wrapper/conn.go @@ -227,10 +227,20 @@ func (c *udpConn) DroppedPackets() int64 { } // WrapUDPConn wraps a net.PacketConn as a udp.Conn with traffic rate limiting. -func WrapUDPConn(pc net.PacketConn, limiter traffic.TrafficLimiter, key string, opts ...limiter.Option) udp.Conn { +// If pc is nil, nil is returned. If limiter is nil, the original connection is +// returned unchanged (no-op). +func WrapUDPConn(pc net.PacketConn, lim traffic.TrafficLimiter, key string, opts ...limiter.Option) udp.Conn { + if pc == nil { + return nil + } + if lim == nil { + if uc, ok := pc.(udp.Conn); ok { + return uc + } + } return &udpConn{ PacketConn: pc, - limiter: limiter, + limiter: lim, opts: opts, key: key, } diff --git a/listener/kcp/listener.go b/listener/kcp/listener.go index 71dd0531..51f1e50f 100644 --- a/listener/kcp/listener.go +++ b/listener/kcp/listener.go @@ -78,7 +78,9 @@ func (l *kcpListener) Init(md md.Metadata) (err error) { } conn = metrics.WrapUDPConn(l.options.Service, conn) - conn = stats.WrapUDPConn(conn, l.options.Stats) + if l.options.Stats != nil { + conn = stats.WrapUDPConn(conn, l.options.Stats) + } conn = admission.WrapUDPConn(l.options.Admission, conn) conn = limiter_wrapper.WrapUDPConn( conn, diff --git a/listener/rudp/listener.go b/listener/rudp/listener.go index 77f652b8..034a4157 100644 --- a/listener/rudp/listener.go +++ b/listener/rudp/listener.go @@ -106,7 +106,9 @@ func (l *rudpListener) Accept() (conn net.Conn, err error) { if pc, ok := conn.(net.PacketConn); ok { uc := metrics.WrapUDPConn(l.options.Service, pc) - uc = stats.WrapUDPConn(uc, l.options.Stats) + if l.options.Stats != nil { + uc = stats.WrapUDPConn(uc, l.options.Stats) + } uc = admission.WrapUDPConn(l.options.Admission, uc) conn = limiter_wrapper.WrapUDPConn( uc, diff --git a/listener/udp_wrapper_safety_test.go b/listener/udp_wrapper_safety_test.go new file mode 100644 index 00000000..3b1b7d24 --- /dev/null +++ b/listener/udp_wrapper_safety_test.go @@ -0,0 +1,97 @@ +package listener_test + +import ( + "net" + "testing" + "time" + + admission_wrapper "github.com/go-gost/x/admission/wrapper" + limiter_wrapper "github.com/go-gost/x/limiter/traffic/wrapper" + metrics_wrapper "github.com/go-gost/x/metrics/wrapper" + stats_wrapper "github.com/go-gost/x/observer/stats/wrapper" +) + +// TestWrapUDPConnChainNoCrashWithNilConfigs verifies that chaining +// WrapUDPConn calls with nil optional configs (stats, admission, limiter) +// does not produce a wrapper with a nil internal PacketConn. +// +// Regression test for go-gost/gost#873 — stats.WrapUDPConn returning nil +// when stats is nil, which then entered admission.WrapUDPConn with a nil +// PacketConn, causing a nil-pointer dereference in service.Serve(). +func TestWrapUDPConnChainNoCrashWithNilConfigs(t *testing.T) { + // Simulate a listener's Init() chaining: metrics → stats → admission → limiter. + pc := &fakePacketConn{} + + // Step 1: metrics.WrapUDPConn — always returns non-nil for non-nil input. + uc := metrics_wrapper.WrapUDPConn("test-service", pc) + if uc == nil { + t.Fatal("metrics.WrapUDPConn returned nil for non-nil input") + } + if uc.LocalAddr() == nil { + t.Error("LocalAddr() returned nil after metrics wrapper") + } + + // Step 2: stats.WrapUDPConn with nil stats — must NOT return nil. + uc = stats_wrapper.WrapUDPConn(uc, nil) + if uc == nil { + t.Fatal("stats.WrapUDPConn returned nil for nil stats (bug — should return original)") + } + if uc.LocalAddr() == nil { + t.Error("LocalAddr() returned nil after stats wrapper with nil stats") + } + + // Step 3: admission.WrapUDPConn with nil admission — must NOT return nil. + uc = admission_wrapper.WrapUDPConn(nil, uc) + if uc == nil { + t.Fatal("admission.WrapUDPConn returned nil for nil admission (bug — should return original)") + } + if uc.LocalAddr() == nil { + t.Error("LocalAddr() returned nil after admission wrapper with nil admission") + } + + // Step 4: limiter.WrapUDPConn with nil limiter — must NOT return nil. + uc = limiter_wrapper.WrapUDPConn(uc, nil, "key") + if uc == nil { + t.Fatal("limiter.WrapUDPConn returned nil for nil limiter (bug — should return original)") + } + if uc.LocalAddr() == nil { + t.Error("LocalAddr() returned nil after limiter wrapper with nil limiter") + } +} + +// TestWrapUDPConnNilInputReturnsNil verifies that all WrapUDPConn variants +// return nil when given a nil net.PacketConn (defensive guard). +func TestWrapUDPConnNilInputReturnsNil(t *testing.T) { + if v := metrics_wrapper.WrapUDPConn("test", nil); v != nil { + t.Error("metrics.WrapUDPConn(nil) should return nil") + } + if v := stats_wrapper.WrapUDPConn(nil, nil); v != nil { + t.Error("stats.WrapUDPConn(nil, nil) should return nil") + } + if v := admission_wrapper.WrapUDPConn(nil, nil); v != nil { + t.Error("admission.WrapUDPConn(nil, nil) should return nil") + } + if v := limiter_wrapper.WrapUDPConn(nil, nil, ""); v != nil { + t.Error("limiter.WrapUDPConn(nil, nil, '') should return nil") + } +} + +// fakePacketConn is a minimal net.PacketConn for testing wrapper chains. +type fakePacketConn struct{} + +func (c *fakePacketConn) ReadFrom(p []byte) (n int, addr net.Addr, err error) { + return 0, nil, nil +} +func (c *fakePacketConn) WriteTo(p []byte, addr net.Addr) (n int, err error) { + return 0, nil +} +func (c *fakePacketConn) Close() error { return nil } +func (c *fakePacketConn) LocalAddr() net.Addr { return &fakeAddr{} } +func (c *fakePacketConn) SetDeadline(t time.Time) error { return nil } +func (c *fakePacketConn) SetReadDeadline(t time.Time) error { return nil } +func (c *fakePacketConn) SetWriteDeadline(t time.Time) error { return nil } + +type fakeAddr struct{} + +func (a *fakeAddr) Network() string { return "udp" } +func (a *fakeAddr) String() string { return "127.0.0.1:0" } diff --git a/observer/stats/wrapper/conn.go b/observer/stats/wrapper/conn.go index b0385d0e..f1a96678 100644 --- a/observer/stats/wrapper/conn.go +++ b/observer/stats/wrapper/conn.go @@ -156,10 +156,18 @@ type udpConn struct { // accepted UDP client session == one connection, mirroring WrapConn) and // tracks bytes read and written across all read/write methods (Read, Write, // ReadFrom, WriteTo, ReadFromUDP, ReadMsgUDP, WriteToUDP, WriteMsgUDP). On -// Close, it decrements the current connection count. If pc or pStats is nil, -// it returns nil. +// Close, it decrements the current connection count. +// +// If pc is nil, nil is returned. If pStats is nil, the original connection +// is returned unchanged (consistent with WrapConn and WrapPacketConn). func WrapUDPConn(pc net.PacketConn, pStats stats.Stats) udp.Conn { - if pc == nil || pStats == nil { + if pc == nil { + return nil + } + if pStats == nil { + if uc, ok := pc.(udp.Conn); ok { + return uc + } return nil }