Skip to content

fix(node): keep interface list alive during IPv6 lookup - #36351

Open
nathanwhit wants to merge 1 commit into
denoland:mainfrom
nathanwhit:fix/node-ipv6-interface-list-lifetime
Open

fix(node): keep interface list alive during IPv6 lookup#36351
nathanwhit wants to merge 1 commit into
denoland:mainfrom
nathanwhit:fix/node-ipv6-interface-list-lifetime

Conversation

@nathanwhit

Copy link
Copy Markdown
Member

Summary

  • keep the getifaddrs list owned for the full IPv6 interface traversal
  • release the list once after interface resolution completes
  • return the default interface index when name-to-index lookup fails

Details

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-0 fallback remain unchanged.

Validation

  • cargo test -p deno_node ipv6_interface_resolution --lib
  • cargo build -p deno --bin deno
  • cargo test -p unit_node_tests --test unit_node -- unit_node::dgram_test
  • ./tools/format.js --check

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the ifa_name CStr borrow.
  • Semantics preserved exactly -- getifaddrs failure -> Ok(0); match with idx != 0 -> return it; match with idx == 0 -> keep scanning; no match -> Ok(0). The only difference is that the idx == 0 continuation no longer walks freed memory.
  • The Drop null-check is right: a successful getifaddrs can still yield NULL when 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: netif is already a workspace dep (ext/os uses it) and exposes up() -> name() / address() / scope_id(). Porting this path to it would delete the remaining unsafe entirely, and would also make the bare-address path work on Windows, which currently returns 0 unconditionally.
  • nit: title scope should be ext/node to match convention.
  • CI hasn't reported any checks on this PR yet.

Comment thread ext/node/ops/udp.rs
}
}
resolve_ipv6_interface_by_address(addr_str, |name| {
// SAFETY: if_nametoindex is safe with a valid C string from getifaddrs.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread ext/node/ops/udp.rs
}

#[cfg(unix)]
struct IfAddrsList(*mut libc::ifaddrs);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread ext/node/ops/udp.rs
0
})
.expect("interface resolution should not fail");
assert!(lookup_count.get() > 0);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants