fix(node): keep interface list alive during IPv6 lookup - #36351
fix(node): keep interface list alive during IPv6 lookup#36351nathanwhit wants to merge 1 commit into
Conversation
bartlomieju
left a comment
There was a problem hiding this comment.
Confirmed the bug against main (udp.rs:215-266): the old code called freeifaddrs(addrs) as soon as an address matched, before checking the if_nametoindex result -- so when the lookup returned 0 the loop kept walking freed ifaddrs nodes and the fall-through path freed the list a second time. Reachable from JS via addMembership()/setMulticastInterface() with a bare IPv6 address matching a local interface whose name-to-index lookup fails. Good catch.
The fix looks correct:
- Single free via
Drop, and the list stays alive for the whole traversal including theifa_nameCStrborrow. - Semantics preserved exactly --
getifaddrsfailure ->Ok(0); match withidx != 0-> return it; match withidx == 0-> keep scanning; no match ->Ok(0). The only difference is that theidx == 0continuation no longer walks freed memory. - The
Dropnull-check is right: a successfulgetifaddrscan still yieldNULLwhen there are no interfaces. - Windows/other platforms unchanged.
Two small things inline, plus a couple of notes:
- The test is a regression guard rather than a detector -- without ASAN/miri the old code's UAF wouldn't reliably fail here. Fine as-is, just worth not over-claiming.
- Optional follow-up, not this PR:
netifis already a workspace dep (ext/osuses it) and exposesup()->name()/address()/scope_id(). Porting this path to it would delete the remainingunsafeentirely, and would also make the bare-address path work on Windows, which currently returns0unconditionally. - nit: title scope should be
ext/nodeto match convention. - CI hasn't reported any checks on this PR yet.
| } | ||
| } | ||
| resolve_ipv6_interface_by_address(addr_str, |name| { | ||
| // SAFETY: if_nametoindex is safe with a valid C string from getifaddrs. |
There was a problem hiding this comment.
The pointer passed to if_nametoindex here comes from the locally-built CString, not from getifaddrs -- worth rewording so the comment names the invariant that actually holds (a valid, NUL-terminated CString that outlives the call).
| } | ||
|
|
||
| #[cfg(unix)] | ||
| struct IfAddrsList(*mut libc::ifaddrs); |
There was a problem hiding this comment.
Nice -- a single owner with one Drop is the right shape for this, and it makes the early-free class of bug unrepresentable rather than just fixed at one call site.
| 0 | ||
| }) | ||
| .expect("interface resolution should not fail"); | ||
| assert!(lookup_count.get() > 0); |
There was a problem hiding this comment.
This makes the test depend on ::1 actually being assigned to a local interface. True on normal GitHub runners, but not in containers with net.ipv6.conf.all.disable_ipv6=1, where it would fail for an unrelated reason.
assert_eq!(result, 0) on the next line is the assertion that carries the regression coverage. If you want to keep the "the closure was reached" check, either parameterize the interface list too so the test is hermetic, or gate this assert on having seen any AF_INET6 entry.
Summary
getifaddrslist owned for the full IPv6 interface traversalDetails
IPv6 multicast interface resolution released the interface list as soon as an address matched, before knowing whether the corresponding interface-name lookup succeeded. When that lookup returned index
0, traversal continued even though the list was no longer alive, and the normal cleanup path attempted to release it again.This change gives the list a single lifetime guard that remains active through traversal. Interface-name conversion, address matching, successful index selection, and the index-
0fallback remain unchanged.Validation
cargo test -p deno_node ipv6_interface_resolution --libcargo build -p deno --bin denocargo test -p unit_node_tests --test unit_node -- unit_node::dgram_test./tools/format.js --check