fix(stun): harden packet decode and listener shutdown

Fixes three crash paths in STUN discovery, one observed in the wild and
two found while tracing its error path.

ReadPacketData can return an empty buffer with a nil error, so slicing
at payloadOff panicked with "slice bounds out of range [32:0]" on
darwin/bsd. Guard the length before slicing, and guard the equivalent
buf[8:] on Linux.

Parse returns nil when the reply carries no XOR-MAPPED-ADDRESS, which
Connect then dereferenced. An RFC 3489-era server answering with only
MAPPED-ADDRESS would crash the daemon. Return ErrNoMappedAddress instead
so the resolver falls through to the next configured server.

Stop and the listener goroutines shared no shutdown signal. A listener
parked handing off a reply that arrived after Read timed out would race
Stop's close of packetChan and panic on Linux, or block Stop's
waitGroup.Wait until context cancellation on darwin/bsd. Add a done
channel and stop closing packetChan: Read already selects with a
timeout, so the close bought nothing and only opened the race. This also
stops the Linux listener spinning on a closed socket until its timeout.

Not covered by new tests: the short-packet and shutdown paths need a raw
socket or pcap handle, so they are verified by construction and the
existing suite only.

Signed-off-by: Date Huang <tjjh89017@hotmail.com>

Author: tjjh89017

internal/stun/helper.go

@@ -11,6 +11,10 @@ import (
 var (
 	ErrResponseMessage = errors.New("error reading from response message channel")
 	ErrTimeout         = errors.New("timed out waiting for response")
+	// ErrNoMappedAddress means the reply decoded cleanly but carried no
+	// XOR-MAPPED-ADDRESS, so there is no reflexive endpoint to report. An
+	// RFC 3489-era server answering with only MAPPED-ADDRESS lands here.
+	ErrNoMappedAddress = errors.New("stun response has no XOR-MAPPED-ADDRESS")
 )
 
 const BindingPacketHeaderSize = 8

internal/stun/stun_darwinbsd.go

@@ -38,6 +38,12 @@ type Stun struct {
 	once       sync.Once
 	packetChan chan *stun.Message
 	waitGroup  sync.WaitGroup
+	// done is closed by Stop to release a listener goroutine parked trying to
+	// hand off a packet nobody is waiting for any more. Without it Stop's
+	// waitGroup.Wait blocks until the context is cancelled, hanging Resolve's
+	// deferred cleanup.
+	done     chan struct{}
+	stopOnce sync.Once
 }
 
 func calculatePayloadOffset(linkType uint32, protocol string) uint32 {
@@ -153,6 +159,7 @@ func New(ctx context.Context, excludeInterface string, port uint16, protocol str
 		protocol:   protocol,
 		handles:    handles,
 		packetChan: make(chan *stun.Message),
+		done:       make(chan struct{}),
 	}
 
 	if err := setPacketConn(s, c); err != nil {
@@ -198,9 +205,13 @@ func setPacketConn(s *Stun, c net.PacketConn) error {
 	return nil
 }
 
+// Stop signals the per-interface listeners, waits for them, then closes the
+// socket. packetChan is deliberately left open: Read already selects with a
+// timeout, so closing it buys nothing and only opens a send-on-closed-channel
+// race. The channel is garbage collected along with the Stun.
 func (s *Stun) Stop() error {
+	s.stopOnce.Do(func() { close(s.done) })
 	s.waitGroup.Wait()
-	close(s.packetChan)
 	if s.protocol == "ipv6" {
 		return s.conn6.Close()
 	}
@@ -227,6 +238,8 @@ func (s *Stun) Start(ctx context.Context) {
 					select {
 					case <-ctx.Done():
 						return
+					case <-s.done:
+						return
 					case <-timeout:
 						return
 					default:
@@ -239,6 +252,14 @@ func (s *Stun) Start(ctx context.Context) {
 							logger.Trace().Msgf("fail to read packet data from %s, err %v", handle.name, err)
 							continue
 						}
+						// ReadPacketData can hand back a short or empty
+						// buffer with a nil error (a timed-out read yields
+						// len 0), so the payload offset is not safe to slice
+						// with until it is known to be in range.
+						if uint32(len(buf)) < handle.payloadOff {
+							logger.Trace().Msgf("short packet (%d bytes, need %d) from %s", len(buf), handle.payloadOff, handle.name)
+							continue
+						}
 						// decode STUN
 						m := &stun.Message{
 							Raw: buf[handle.payloadOff:],
@@ -251,6 +272,8 @@ func (s *Stun) Start(ctx context.Context) {
 						case s.packetChan <- m:
 						case <-ctx.Done():
 							return
+						case <-s.done:
+							return
 						}
 						return
 					}
@@ -298,7 +321,12 @@ func (s *Stun) Connect(ctx context.Context, stunAddr string) (_ string, _ int, e
 		return "", 0, err
 	}
 
+	// Parse returns nil when the reply carries no XOR-MAPPED-ADDRESS; the
+	// resolver treats the error as "this server failed" and moves to the next.
 	replyAddr := Parse(ctx, reply)
+	if replyAddr == nil {
+		return "", 0, ErrNoMappedAddress
+	}
 
 	return replyAddr.IP.String(), replyAddr.Port, nil
 }

internal/stun/stun_linux.go

@@ -4,6 +4,7 @@ package stun
 import (
 	"context"
 	"encoding/binary"
+	"fmt"
 	"net"
 	"sync"
 	"syscall"
@@ -25,6 +26,12 @@ type Stun struct {
 	conn6      *ipv6.PacketConn
 	once       sync.Once
 	packetChan chan []byte
+	// done is closed by Stop to release a listener goroutine that is parked
+	// trying to hand off a packet nobody is waiting for any more (a reply that
+	// arrived after Read already timed out). Without it Stop's close of
+	// packetChan would race that send and panic.
+	done     chan struct{}
+	stopOnce sync.Once
 }
 
 // markControl stamps firewallMark on the socket before it is bound, so the
@@ -116,6 +123,7 @@ func New(ctx context.Context, excludeInterface string, port uint16, protocol str
 		port:       port,
 		protocol:   protocol,
 		packetChan: make(chan []byte),
+		done:       make(chan struct{}),
 	}
 
 	if err := setPacketConn(s, c, filter); err != nil {
@@ -125,8 +133,12 @@ func New(ctx context.Context, excludeInterface string, port uint16, protocol str
 	return s, nil
 }
 
+// Stop releases the listener goroutine and closes the socket. packetChan is
+// deliberately left open: Read already selects with a timeout, so closing it
+// buys nothing and only opens a send-on-closed-channel race with a listener
+// still holding a late reply. The channel is garbage collected with the Stun.
 func (s *Stun) Stop() error {
-	close(s.packetChan)
+	s.stopOnce.Do(func() { close(s.done) })
 	if s.protocol == "ipv6" {
 		return s.conn6.Close()
 	}
@@ -150,19 +162,30 @@ func (s *Stun) Start(ctx context.Context) {
 				select {
 				case <-ctx.Done():
 					return
+				case <-s.done:
+					return
 				case <-timeout:
 					return
 				default:
 					buf := make([]byte, PacketSize)
 					n, err := s.readFrom(buf)
 					if err != nil {
+						// Stop closed the socket out from under us; leave
+						// rather than spin on a dead fd until timeout.
+						select {
+						case <-s.done:
+							return
+						default:
+						}
 						continue
 					}
 					select {
 					case s.packetChan <- buf[:n]:
 						return
 					case <-ctx.Done():
 						return
+					case <-s.done:
+						return
 					}
 				}
 			}
@@ -218,7 +241,12 @@ func (s *Stun) Connect(ctx context.Context, stunAddr string) (string, int, error
 		return "", 0, err
 	}
 
+	// Parse returns nil when the reply carries no XOR-MAPPED-ADDRESS; the
+	// resolver treats the error as "this server failed" and moves to the next.
 	replyAddr := Parse(ctx, reply)
+	if replyAddr == nil {
+		return "", 0, ErrNoMappedAddress
+	}
 
 	return replyAddr.IP.String(), replyAddr.Port, nil
 }
@@ -229,6 +257,9 @@ func (s *Stun) Read(ctx context.Context) (*stun.Message, error) {
 		// Linux kernel strips IP headers for both IPv4 and IPv6 raw sockets
 		// We only receive: UDP header (8 bytes) + STUN payload
 		// Note: BPF filter runs before IP header stripping, so it needs different offsets
+		if len(buf) < 8 {
+			return nil, fmt.Errorf("short packet: %d bytes, need at least a UDP header", len(buf))
+		}
 		m := &stun.Message{
 			Raw: buf[8:], // Skip UDP header
 		}