quicreuse: don't return a source-IP selector with no route table - #3538
Open
certaindaniel wants to merge 1 commit into
Open
certaindaniel wants to merge 1 commit into
certaindaniel wants to merge 1 commit into
Conversation
netroute.New() fails wherever the process cannot read the kernel route
table, which on Android is the norm rather than an edge case: an
unprivileged app has no netlink access ("route ip+net: netlinkrib:
permission denied"). Whether it can at all varies by OEM and Android
version, so this reproduces on some devices and not others.
defaultSourceIPSelectorFn handed that failure back as
(&netrouteSourceIPSelector{routes: nil}, err) — a non-nil selector
wrapping a nil router. Both call sites discard the error ("Ignore the
error, there's nothing we can do about it") and then guard with
`if router != nil`, which passes, because what is nil is the field and not
the wrapper. The next QUIC dial dereferenced it:
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV code=0x1 addr=0x18]
quicreuse.(*netrouteSourceIPSelector).PreferredSourceIPForDestination
quicreuse.(*reuse).TransportWithAssociationForDial
quicreuse.(*ConnManager).DialQUIC
quic.(*transport).dialWithScope
swarm.(*Swarm).dialAddr
swarm.(*dialLimiter).executeDial
newSourceIPSelector returns an untyped nil when there is no router, so the
existing guard means what it says and the dial proceeds without source-IP
affinity. That affinity is an optimisation for multi-homed hosts, not a
correctness requirement, and it was already absent wherever netroute fails
— only there it panicked instead of degrading.
PreferredSourceIPForDestination also refuses rather than dereferences, so a
selector built directly cannot reintroduce this.
Fixes libp2p#3537
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3537.
netroute.New()fails wherever the process cannot read the kernel route table. On Android that is the norm rather than an edge case — an unprivileged app has no netlink access:defaultSourceIPSelectorFnhanded that back as(&netrouteSourceIPSelector{routes: nil}, err): a non-nil selector wrapping a nil router. Both call sites discard the error, then guard withif router != nil— which passes, because what is nil is the field rather than the wrapper. The next QUIC dial dereferenced it and took the process down.The fix
newSourceIPSelectorreturns an untyped nil when there is no router, so the existing guard finally means what it says. A typed nil ((*netrouteSourceIPSelector)(nil)) would still satisfy!= niland change nothing — there is a test for that specifically.PreferredSourceIPForDestinationalso refuses rather than dereferences, so a selector constructed directly cannot reintroduce it.Degraded behaviour is a dial without source-IP affinity. That is an optimisation for multi-homed hosts rather than a correctness requirement, and it was already absent on every platform where netroute fails — only there it panicked instead of degrading.
Tests
Four, in
source_ip_selector_test.go. Reverting either half of the fix fails them, andTestDialWithoutASourceIPSelectorreproduces the original panic at the same line and sameaddr=0x18:Provenance
We hit this in production on Android: 21 crashes in 19 hours on one device, every one on a QUIC dial. The equivalent patch has been shipping to users for a day and the crash is gone on the device that reported it.
@bunnysayzz said on the issue that they were preparing a PR — apologies for the overlap. We already had this written and deployed, so I am offering it rather than sitting on it; happy to close this in favour of yours if you have one in flight, or for you to review this one.