Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

x/net/route: ParseRIB panics on message from Darwin #70528

Closed
raggi opened this issue Nov 22, 2024 · 13 comments
Closed

x/net/route: ParseRIB panics on message from Darwin #70528

raggi opened this issue Nov 22, 2024 · 13 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Milestone

Comments

@raggi
Copy link
Contributor

raggi commented Nov 22, 2024

Go version

go1.23.3 & golang.org/x/net@v0.30.0

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/raggi/Library/Caches/go-build'
GOENV='/Users/raggi/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/raggi/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/raggi/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/raggi/.cache/tailscale-go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/raggi/.cache/tailscale-go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23.1'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/raggi/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/bb/dyr_1n6j575g8nq85nmnfbt00000gn/T/go-build3032505426=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Seen from tailscale client, usage at https://github.com/tailscale/tailscale/blob/8e5cfbe4ab11713e383b3ff0d978f116320de2a3/net/netmon/netmon_darwin.go#L59

What did you see happen?

2024-11-22T19:46:58Z	panic: runtime error: slice bounds out of range [8:0]
2024-11-22T19:46:58Z	
2024-11-22T19:46:58Z	goroutine 75 [running]:
2024-11-22T19:46:58Z	golang.org/x/net/route.parseInetAddr(0x14000582dc8?, {0x1400024e090, 0x14000f38480?, 0x788})
2024-11-22T19:46:58Z	golang.org/x/net@v0.30.0/route/address.go:203 +0x280
2024-11-22T19:46:58Z	golang.org/x/net/route.parseAddrs(0x15, 0x1033fb958, {0x1400024e074, 0x34, 0x7a4})
2024-11-22T19:46:58Z	golang.org/x/net@v0.30.0/route/address.go:408 +0xd8
2024-11-22T19:46:58Z	golang.org/x/net/route.(*wireFormat).parseRouteMessage(0x1400012a018, 0x140003f8008?, {0x1400024e018, 0x90, 0x800})
2024-11-22T19:46:58Z	golang.org/x/net@v0.30.0/route/route_classic.go:70 +0x2b8
2024-11-22T19:46:58Z	golang.org/x/net/route.ParseRIB(0x1, {0x1400024e018?, 0x14000f38380?, 0x140001105b0?})
2024-11-22T19:46:58Z	golang.org/x/net@v0.30.0/route/message.go:55 +0x194
2024-11-22T19:46:58Z	tailscale.com/net/netmon.(*darwinRouteMon).Receive(0x1400024e008)
2024-11-22T19:46:58Z	tailscale.com@v1.77.0-pre.0.20241121205641-e3c6ca43d3e3/net/netmon/netmon_darwin.go:59 +0x60
2024-11-22T19:46:58Z	tailscale.com/net/netmon.(*Monitor).pump(0x140001821c0)
2024-11-22T19:46:58Z	tailscale.com@v1.77.0-pre.0.20241121205641-e3c6ca43d3e3/net/netmon/netmon.go:346 +0x78
2024-11-22T19:46:58Z	created by tailscale.com/net/netmon.(*Monitor).Start in goroutine 17
2024-11-22T19:46:58Z	tailscale.com@v1.77.0-pre.0.20241121205641-e3c6ca43d3e3/net/netmon/netmon.go:265 +0x1b8

What did you expect to see?

No panic.


Related issue: #44740
Related change: hurricanehrndz/golang-net@61924c1

@bradfitz
Copy link
Contributor

cc @hurricanehrndz

@hurricanehrndz
Copy link

Thanks, I will take a look at this on the weekend.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 22, 2024
@dmitshur dmitshur added this to the Unreleased milestone Nov 22, 2024
@hurricanehrndz
Copy link

@raggi do you have the bytes array so that I can write up a test for this?

@raggi
Copy link
Contributor Author

raggi commented Nov 22, 2024

@hurricanehrndz I'm working on getting that, in the meantime this should be sufficient defense: https://github.com/golang/net/compare/master...raggi:raggi/darwin-rib-parse?expand=1

raggi added a commit to tailscale/tailscale that referenced this issue Nov 22, 2024
Updates #14201
Updates golang/go#70528

Signed-off-by: James Tucker <james@tailscale.com>
@hurricanehrndz
Copy link

@raggi yeah that looks good.

raggi added a commit to tailscale/tailscale that referenced this issue Nov 22, 2024
Updates #14201
Updates golang/go#70528

Signed-off-by: James Tucker <james@tailscale.com>
@deansheather
Copy link

I was able to gather a panicking RIB from a real darwin amd64 machine using a coredump:

data := []byte{
	0x84, 0x00, 0x05, 0x04, 0x01, 0x00, 0x00, 0x00, 0x03, 0x08, 0x00, 0x01, 0x15, 0x00, 0x00, 0x00,
	0x1B, 0x01, 0x00, 0x00, 0xF5, 0x5A, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x02, 0x00, 0x00,
	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00,
	0x14, 0x12, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
	0x00, 0x00, 0x00, 0x00,
}

_, _ = route.ParseRIB(route.RIBTypeRoute, data)
panic: runtime error: slice bounds out of range [4:0] [recovered]
        panic: runtime error: slice bounds out of range [4:0]

goroutine 18 [running]:
testing.tRunner.func1.2({0xb775380, 0xc0000ac048})
        /usr/local/Cellar/go@1.22/1.22.9/libexec/src/testing/testing.go:1631 +0x24a
testing.tRunner.func1()
        /usr/local/Cellar/go@1.22/1.22.9/libexec/src/testing/testing.go:1634 +0x377
panic({0xb775380?, 0xc0000ac048?})
        /usr/local/Cellar/go@1.22/1.22.9/libexec/src/runtime/panic.go:770 +0x132
golang.org/x/net/route.parseInetAddr(0x80?, {0xc00010206c, 0xc0000d6200?, 0x18})
        /Users/ec2-user/net/route/address.go:188 +0x267
golang.org/x/net/route.parseAddrs(0x15, 0xb784080, {0xc00010205c, 0x28, 0x28})
        /Users/ec2-user/net/route/address.go:408 +0xdd
golang.org/x/net/route.(*wireFormat).parseRouteMessage(0xc00009c0c0, 0xc000050680?, {0xc000102000, 0x84, 0x84})
        /Users/ec2-user/net/route/route_classic.go:70 +0x2fd
golang.org/x/net/route.ParseRIB(0x1, {0xc000102000?, 0xb6bf1f3?, 0xb66ed8f?})
        /Users/ec2-user/net/route/message.go:55 +0x1b3
golang.org/x/net/route.TestPanic(0xc0000b2680?)
        /Users/ec2-user/net/route/message_test.go:271 +0x9a
testing.tRunner(0xc0000b2680, 0xb784000)
        /usr/local/Cellar/go@1.22/1.22.9/libexec/src/testing/testing.go:1689 +0xfb
created by testing.(*T).Run in goroutine 1
        /usr/local/Cellar/go@1.22/1.22.9/libexec/src/testing/testing.go:1742 +0x390

raggi's patch does seem to fix the panic in this case.

It might be related to a recent MacOS update, since the RIB above was gathered from a 15.1 machine (released Oct 28th).

raggi added a commit to tailscale/tailscale that referenced this issue Nov 25, 2024
I was hoping we'd catch an example input quickly, but the reporter had
rebooted their machine and it is no longer exhibiting the behavior. As
such this code may be sticking around quite a bit longer and we might
encounter other errors, so include the panic in the log entry.

Updates #14201
Updates #14202
Updates golang/go#70528

Signed-off-by: James Tucker <james@tailscale.com>
raggi added a commit to raggi/net that referenced this issue Nov 25, 2024
Zero-length sockaddrs were observed in RIBs within golang/go#70528.
These records are to be skipped, and an invariant for later slice
manipulation is to be enforced by a defensive check in parseAddr.

Fixes golang/go#70528
raggi added a commit to raggi/net that referenced this issue Nov 25, 2024
Zero-length sockaddrs were observed in RIBs within golang/go#70528.
These records are to be skipped, and an invariant for later slice
manipulation is to be enforced by a defensive check in parseAddr.

Fixes golang/go#70528
@raggi
Copy link
Contributor Author

raggi commented Nov 25, 2024

agreed, 15.1 changes are the likely cause.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/631475 mentions this issue: route: fix parse of zero-length sockaddrs in RIBs

raggi added a commit to tailscale/tailscale that referenced this issue Nov 25, 2024
I was hoping we'd catch an example input quickly, but the reporter had
rebooted their machine and it is no longer exhibiting the behavior. As
such this code may be sticking around quite a bit longer and we might
encounter other errors, so include the panic in the log entry.

Updates #14201
Updates #14202
Updates golang/go#70528

Signed-off-by: James Tucker <james@tailscale.com>
raggi added a commit to raggi/net that referenced this issue Nov 25, 2024
Zero-length sockaddrs were observed in RIBs within golang/go#70528.
These records are to be skipped, and an invariant for later slice
manipulation is to be enforced by a defensive check in parseAddr.

Fixes golang/go#70528
raggi added a commit to raggi/net that referenced this issue Nov 25, 2024
Zero-length sockaddrs were observed in RIBs within golang/go#70528.
These records are to be skipped, and an invariant for later slice
manipulation is to be enforced by a defensive check in parseAddr.

Fixes golang/go#70528
@joeljeske
Copy link

I know this issue is closed, but I wanted to chime in that I can reproduce this panic (without golang/net@e9cd716) on darwin on macOS 14.7.1, as opposed to only 15.1 like previously suggested.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 4, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/646555 mentions this issue: route: fix parsing network address of length zero

gopherbot pushed a commit to golang/net that referenced this issue Feb 4, 2025
sa_len of 0 should be valid, for Chapter 18 of UNIX® Network Programming
Volume 1, Third Edition: The Sockets Networking API, states:

The socket address structures are variable-length, but this code assumes
that each has an sa_len field specifying its length. There are two
complications that must be handled. First, the two masks, the network
mask and the cloning mask, can be returned in a socket address structure
with an sa_len of 0, but this really occupies the size of an unsigned
long. (Chapter 19 of TCPv2 discusses the cloning feature of the 4.4BSD
routing table). This value represents a mask of all zero bits, which we
printed as 0.0.0.0 for the network mask of the default route in our
earlier example.

There are other references in the book which also state sa_len of 0 is
valid.

Fixes golang/go#70528

Change-Id: I9205a674f9cdf8091b1cc8b8a56609cd1cf4c670
GitHub-Last-Rev: df63086
GitHub-Pull-Request: #230
Reviewed-on: https://go-review.googlesource.com/c/net/+/646555
Auto-Submit: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/646676 mentions this issue: internal/routebsd: fix parsing network address of length zero

gopherbot pushed a commit that referenced this issue Feb 5, 2025
This applies CL 646555 from the net repository to this copy.

For #70528

Change-Id: Ib7e23accfa3f278392e7bdca6f8544b8f1395e7e
Reviewed-on: https://go-review.googlesource.com/c/go/+/646676
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
TryBot-Bypass: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/646975 mentions this issue: internal/routebsd: fix typo in sockAddrLen identifer name

gopherbot pushed a commit that referenced this issue Feb 5, 2025
For #70528.

Change-Id: I0db75cb998aeb299676384fe59bf241db18ebc5c
Reviewed-on: https://go-review.googlesource.com/c/go/+/646975
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Projects
None yet
8 participants