HE3 only ever sees first address of a multi-address DNS answer
Categories
(Core :: Networking: HTTP, defect, P2)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox157 | --- | fixed |
People
(Reporter: leggert, Assigned: mail)
References
(Blocks 1 open bug)
Details
(Whiteboard: [necko-triaged])
Attachments
(1 file)
[Claude found this while I had it looking at some other things. Filing it here in case this is real.]
netwerk/protocol/http/happy_eyeballs_glue/src/lib.rs:750 declares the opaque handle for mozilla::net::NetAddr as a zero-sized type:
#[repr(C)]
pub union NetAddr {
_private: [u8; 0],
}
size_of::<NetAddr>() == 0. This is fine for the *const NetAddr parameters it was modelled on (see neqo_glue/src/lib.rs:161, which uses it correctly), but this crate also uses it as a collection element type: ThinVec<NetAddr>, which cbindgen maps to const nsTArray<NetAddr>* on the C++ side, where NetAddr resolves to the real 112-byte union.
ThinVec::len() and the slice base pointer are both correct, so the loops run the right number of times over the right buffer. But a slice iterator over a zero-sized element type cannot advance a stride-0 pointer -- core::slice::Iter::next decrements a length counter and returns self.ptr unchanged for ZSTs. Every iteration therefore passes the address of element 0 to moz_netaddr_get_family / moz_netaddr_get_network_order_ip / moz_netaddr_get_ipv6.
An N-address answer is converted into N copies of addresses[0].
Affected loops:
process_dns_response_a--lib.rs:299-308process_dns_response_aaaa--lib.rs:330-341process_dns_response_https,ipv4_hintsandipv6_hints--lib.rs:381-408
Impact
Happy Eyeballs receives a degenerate address list, so connection racing and fallback within an address family are defeated: it retries one IP instead of trying the others. A host whose first A record is unreachable will not fall back to its remaining A records via this path. SVCB/HTTPS-RR IP hints are affected the same way.
network.http.happy_eyeballs_enabled defaults to true on desktop on all channels (StaticPrefList.yaml:17058), so this is enabled by default in release.
The debug_assert!/debug_assert_eq! family checks in these loops do not fire, because element 0 has the expected family -- which is why debug and CI builds have not caught it.
Suggested fix
Do not encode sizeof(NetAddr) in Rust -- the local arm of the union is #if defined(XP_UNIX) || defined(XP_WIN), so the size is platform-dependent. Keep the type opaque and move the indexing to C++, e.g.:
extern "C" size_t moz_netaddr_array_length(const nsTArray<NetAddr>* aArr);
extern "C" const NetAddr* moz_netaddr_array_at(const nsTArray<NetAddr>* aArr, size_t aIndex);
replacing the ThinVec<NetAddr> parameters and the ipv4_hints/ipv6_hints members with an opaque array handle. Passing (*const NetAddr, usize) plus a stride obtained from C++ would also work.
Worth adding alongside the fix: a compile-time guard so an opaque FFI type can never again be used as a ThinVec/slice element, and a test that exercises a hostname with more than one A record.
|
Reporter | |
Updated•8 days ago
|
Well spotted @Lars. The fact that I missed this thus far is puzzling to me. Thank you.
The glue declared NetAddr as an opaque zero-sized union and held it in a
ThinVec<NetAddr>. C++ passed full-size elements, so Rust walked the array with
a stride of 0: every slot read the first address. The engine then dropped the
duplicates as already-attempted endpoints, leaving one connection attempt and
no failover.
An opaque type is safe behind a pointer but not as a collection element, so
pass the IpAddr that cbindgen already generates for the reverse direction.
|
||
Comment 4•6 days ago
|
||
| bugherder | ||
Description
•