From 993419d84564fb0d165455cd30f38b2af0a5556c Mon Sep 17 00:00:00 2001 From: ginuerzh Date: Tue, 13 Oct 2026 14:12:50 +0000 Subject: [PATCH] fix(handler/tun): guard every packet classifier against an empty buffer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit waterutil.IsIPv4 and IsIPv6 index packet[0] with no length check, and a tun device can legally return zero bytes with no error — an Android VpnService fd does exactly that. routing such a read into a classifier panics with "index out of range [0] with length 0", and in the c-shared library the phone loads the runtime cannot unwind past the cgo boundary, so the panic took the app: the spoke died on 2026-10-12 with three such lines in crash.log, no stack, and a 17-minute gap before Android restarted it. The knowledge was already in the tree — sourceOf and destinationOf in handler/tun/p2p.go carry the guard, with a comment naming this exact panic — but the eight reads in the client and server loops and the association classifier in handler/router did not. The asymmetry was the bug. The guard now lives once, in internal/util/tun, as IPv4Packet and IPv6Packet, and every classifier goes through it. Tested at the real entry point: a fake device that returns (0, nil) before handing over a real IPv4 packet, asserted to be survived and forwarded. --- handler/router/associate.go | 5 +- handler/tun/client.go | 8 +-- handler/tun/empty_packet_test.go | 93 ++++++++++++++++++++++++++++++++ handler/tun/server.go | 8 +-- internal/util/tun/packet.go | 21 ++++++++ internal/util/tun/packet_test.go | 36 +++++++++++++ 6 files changed, 161 insertions(+), 10 deletions(-) create mode 100644 handler/tun/empty_packet_test.go create mode 100644 internal/util/tun/packet.go create mode 100644 internal/util/tun/packet_test.go diff --git a/handler/router/associate.go b/handler/router/associate.go index 740ce96..12ce54f 100644 --- a/handler/router/associate.go +++ b/handler/router/associate.go @@ -16,6 +16,7 @@ import ( "github.com/go-gost/core/router" "github.com/go-gost/core/sd" "github.com/go-gost/relay" + tun_util "github.com/go-gost/x/internal/util/tun" xip "github.com/go-gost/x/internal/net/ip" "github.com/go-gost/x/internal/util/cache" traffic_wrapper "github.com/go-gost/x/limiter/traffic/wrapper" @@ -234,7 +235,7 @@ func (h *routerHandler) sdRenew(ctx context.Context, clientID string, connectorI func (h *routerHandler) handlePacket(ctx context.Context, data []byte, routerID relay.TunnelID, log logger.Logger) error { // ---- Parse IP header ---- var dstIP net.IP - if waterutil.IsIPv4(data) { + if tun_util.IPv4Packet(data) { header, err := ipv4.ParseHeader(data) if err != nil { return err @@ -247,7 +248,7 @@ func (h *routerHandler) handlePacket(ctx context.Context, data []byte, routerID header.Src, header.Dst, xip.Protocol(waterutil.IPv4Protocol(data)), header.Len, header.TotalLen, header.ID, header.Flags) } - } else if waterutil.IsIPv6(data) { + } else if tun_util.IPv6Packet(data) { header, err := ipv6.ParseHeader(data) if err != nil { return err diff --git a/handler/tun/client.go b/handler/tun/client.go index cddbf42..95b90c0 100644 --- a/handler/tun/client.go +++ b/handler/tun/client.go @@ -147,7 +147,7 @@ func (h *tunHandler) transportClient(ctx context.Context, tun io.ReadWriter, con return fmt.Errorf("%w: read: %w", ErrTun, err) } - if waterutil.IsIPv4(b[:n]) { + if tun_util.IPv4Packet(b[:n]) { header, err := ipv4.ParseHeader(b[:n]) if err != nil { log.Warn(err) @@ -159,7 +159,7 @@ func (h *tunHandler) transportClient(ctx context.Context, tun io.ReadWriter, con header.Src, header.Dst, xip.Protocol(waterutil.IPv4Protocol(b[:n])), header.Len, header.TotalLen, header.ID, header.Flags) } - } else if waterutil.IsIPv6(b[:n]) { + } else if tun_util.IPv6Packet(b[:n]) { header, err := ipv6.ParseHeader(b[:n]) if err != nil { log.Warn(err) @@ -248,7 +248,7 @@ func (h *tunHandler) transportClient(ctx context.Context, tun io.ReadWriter, con return nil } - if waterutil.IsIPv4(b[:n]) { + if tun_util.IPv4Packet(b[:n]) { header, err := ipv4.ParseHeader(b[:n]) if err != nil { log.Warn(err) @@ -260,7 +260,7 @@ func (h *tunHandler) transportClient(ctx context.Context, tun io.ReadWriter, con header.Src, header.Dst, xip.Protocol(waterutil.IPv4Protocol(b[:n])), header.Len, header.TotalLen, header.ID, header.Flags) } - } else if waterutil.IsIPv6(b[:n]) { + } else if tun_util.IPv6Packet(b[:n]) { header, err := ipv6.ParseHeader(b[:n]) if err != nil { log.Warn(err) diff --git a/handler/tun/empty_packet_test.go b/handler/tun/empty_packet_test.go new file mode 100644 index 0000000..ce4c68b --- /dev/null +++ b/handler/tun/empty_packet_test.go @@ -0,0 +1,93 @@ +package tun + +import ( + "context" + "net" + "testing" + "time" + + xlogger "github.com/go-gost/x/logger" +) + +// zeroLengthThenPacket is a tun device that hands the reader one empty packet +// before a real one. An Android VpnService fd does exactly that, and the read +// is legal: zero bytes, no error. +// +// This is the shape of the crash that took the spoke down on 2026-10-12. The +// last line the phone logged was a dial inside this relay path +// ("connect /ip", forward/connector.go:44) and crash.log held only +// "panic: runtime error: index out of range [0] with length 0" — because +// waterutil.IsIPv4/IsIPv6 index packet[0] with no length check, so classifying +// an empty read panicked right there. sourceOf/destinationOf (p2p.go) already +// guard the empty buffer; these read loops never did. +type zeroLengthThenPacket struct { + empties int + packet []byte + sent bool +} + +func (d *zeroLengthThenPacket) Read(p []byte) (int, error) { + if d.empties > 0 { + d.empties-- + return 0, nil + } + if !d.sent && len(d.packet) > 0 { + d.sent = true + return copy(p, d.packet), nil + } + return 0, net.ErrClosed +} + +func (d *zeroLengthThenPacket) Write(p []byte) (int, error) { return len(p), nil } + +// TestTransportClientSurvivesAnEmptyPacketRead: the relay loop must drop a +// zero-length read and carry on — the packet that follows it still reaches the +// link — instead of panicking inside waterutil on packet[0]. +func TestTransportClientSurvivesAnEmptyPacketRead(t *testing.T) { + h := newTestHandler(nil, nil) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + dev := &zeroLengthThenPacket{empties: 1, packet: realIPv4Packet()} + ccA, ccB := net.Pipe() + defer ccA.Close() + defer ccB.Close() + + // Whatever the relay writes to the link, collected here: the proof that the + // loop was still running after the empty read. + got := make(chan []byte, 1) + go func() { + buf := make([]byte, 256) + ccB.SetReadDeadline(time.Now().Add(3 * time.Second)) + n, err := ccB.Read(buf) + if err != nil && n <= 0 { + got <- nil + return + } + got <- append([]byte{}, buf[:n]...) + }() + + panicked := make(chan any, 1) + errCh := make(chan error, 1) + go func() { + defer func() { + if r := recover(); r != nil { + panicked <- r + } + }() + errCh <- h.transportClient(ctx, dev, ccA, xlogger.Nop()) + }() + + select { + case r := <-panicked: + t.Fatalf("transportClient panicked on an empty packet read: %v", r) + case pkt := <-got: + if len(pkt) == 0 { + t.Fatal("nothing reached the link: the loop died with the empty read") + } + case err := <-errCh: + t.Fatalf("transportClient returned (%v) instead of forwarding the packet after the empty read", err) + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for the relay to forward the packet") + } +} diff --git a/handler/tun/server.go b/handler/tun/server.go index f7b196e..4991a28 100644 --- a/handler/tun/server.go +++ b/handler/tun/server.go @@ -64,7 +64,7 @@ func (h *tunHandler) transportServer(ctx context.Context, tun io.ReadWriter, con } var src, dst net.IP - if waterutil.IsIPv4(b[:n]) { + if tun_util.IPv4Packet(b[:n]) { header, err := ipv4.ParseHeader(b[:n]) if err != nil { log.Warnf("parse ipv4 packet header: %v", err) @@ -77,7 +77,7 @@ func (h *tunHandler) transportServer(ctx context.Context, tun io.ReadWriter, con src, dst, xip.Protocol(waterutil.IPv4Protocol(b[:n])), header.Len, header.TotalLen, header.ID, header.Flags) } - } else if waterutil.IsIPv6(b[:n]) { + } else if tun_util.IPv6Packet(b[:n]) { header, err := ipv6.ParseHeader(b[:n]) if err != nil { log.Warnf("parse ipv6 packet header: %v", err) @@ -189,7 +189,7 @@ func (h *tunHandler) transportServer(ctx context.Context, tun io.ReadWriter, con } var src, dst net.IP - if waterutil.IsIPv4(b[:n]) { + if tun_util.IPv4Packet(b[:n]) { header, err := ipv4.ParseHeader(b[:n]) if err != nil { log.Warnf("parse ipv4 packet header: %v", err) @@ -202,7 +202,7 @@ func (h *tunHandler) transportServer(ctx context.Context, tun io.ReadWriter, con src, dst, xip.Protocol(waterutil.IPv4Protocol(b[:n])), header.Len, header.TotalLen, header.ID, header.Flags) } - } else if waterutil.IsIPv6(b[:n]) { + } else if tun_util.IPv6Packet(b[:n]) { header, err := ipv6.ParseHeader(b[:n]) if err != nil { log.Warnf("parse ipv6 packet header: %v", err) diff --git a/internal/util/tun/packet.go b/internal/util/tun/packet.go new file mode 100644 index 0000000..0006ef9 --- /dev/null +++ b/internal/util/tun/packet.go @@ -0,0 +1,21 @@ +package tun + +import "github.com/songgao/water/waterutil" + +// IPv4Packet and IPv6Packet are the only ways a reader should ask waterutil +// which family a buffer is. +// +// waterutil.IsIPv4/IsIPv6 index packet[0] with no length check, so an empty +// buffer panics there instead of reporting "neither". An empty buffer is not +// hypothetical: a device may legally return zero bytes with no error — an +// Android VpnService tun fd does exactly that — and that read produced the +// "panic: runtime error: index out of range [0] with length 0" that took the +// spoke down on 2026-10-12, three times, with no stack left in crash.log. In a +// c-shared library the runtime cannot unwind past the cgo boundary, so the +// panic took the app with it. +// +// Every packet classifier needs this guard: the read loops in handler/tun, +// the association classifier in handler/router, and anything added later. +func IPv4Packet(b []byte) bool { return len(b) > 0 && waterutil.IsIPv4(b) } + +func IPv6Packet(b []byte) bool { return len(b) > 0 && waterutil.IsIPv6(b) } diff --git a/internal/util/tun/packet_test.go b/internal/util/tun/packet_test.go new file mode 100644 index 0000000..939db71 --- /dev/null +++ b/internal/util/tun/packet_test.go @@ -0,0 +1,36 @@ +package tun + +import "testing" + +// TestPacketFamilyGuardsTheEmptyBuffer pins the guard every packet classifier +// in this repo must carry. waterutil.IsIPv4/IsIPv6 index packet[0] with no +// length check, so an empty buffer — a device's legal zero-byte read — panics +// with "index out of range [0] with length 0": the panic that took the spoke +// down on 2026-10-12, in a c-shared library where nothing could catch it. +func TestPacketFamilyGuardsTheEmptyBuffer(t *testing.T) { + if IPv4Packet(nil) || IPv4Packet([]byte{}) { + t.Error("an empty buffer is not an IPv4 packet") + } + if IPv6Packet(nil) || IPv6Packet([]byte{}) { + t.Error("an empty buffer is not an IPv6 packet") + } + + // The guard must not reject the packets it reports on: a zero-length buffer + // is the only case it answers "no" for. + v4 := []byte{0x45, 0, 0, 20, 0, 0, 0, 0, 0, 1} + if !IPv4Packet(v4) { + t.Error("a real IPv4 header is not reported as one") + } + if IPv6Packet(v4) { + t.Error("an IPv4 header is reported as IPv6") + } + + v6 := make([]byte, 40) + v6[0] = 0x60 + if !IPv6Packet(v6) { + t.Error("a real IPv6 header is not reported as one") + } + if IPv4Packet(v6) { + t.Error("an IPv6 header is reported as IPv4") + } +}