udp: OSX10.15 crash in `decode_recv` expecting IPv4 or v6

[mxinden]

quinn-udp's decode_recv in unix.rs expects the source address to be either an IPv4 or an IPv6, see unreachable! below:

quinn/quinn-udp/src/unix.rs

Lines 760 to 782 in 3e302f7

	let addr = match libc::c_int::from(name.ss_family) {
	libc::AF_INET => {
	// Safety: if the ss_family field is AF_INET then storage must be a sockaddr_in.
	let addr: &libc::sockaddr_in =
	unsafe { &*(&name as *const _ as *const libc::sockaddr_in) };
	SocketAddr::V4(SocketAddrV4::new(
	Ipv4Addr::from(addr.sin_addr.s_addr.to_ne_bytes()),
	u16::from_be(addr.sin_port),
	))
	}
	libc::AF_INET6 => {
	// Safety: if the ss_family field is AF_INET6 then storage must be a sockaddr_in6.
	let addr: &libc::sockaddr_in6 =
	unsafe { &*(&name as *const _ as *const libc::sockaddr_in6) };
	SocketAddr::V6(SocketAddrV6::new(
	Ipv6Addr::from(addr.sin6_addr.s6_addr),
	u16::from_be(addr.sin6_port),
	addr.sin6_flowinfo,
	addr.sin6_scope_id,
	))
	}
	_ => unreachable!(),
	};

For some (yet) unknown reason, Firefox crashes on some MacOS X 10.15 instances on exactly the above assumption.

Reported in https://bugzilla.mozilla.org/show_bug.cgi?id=1987606.

Crash reports (66 so far): https://crash-stats.mozilla.org/signature/?product=Firefox&signature=quinn_udp%3A%3Aimp%3A%3Adecode_recv

I have to investigate more. Opening up here for tracking for now.

Whether we find the root cause or not, it might be worth being a bit more defensive here, only crashing in debug mode, returning an error in release mode.

[djc]

Whether we find the root cause or not, it might be worth being a bit more defensive here, only crashing in debug mode, returning an error in release mode.

Sounds okay to me. Could also just error unconditionally?

[thomaseizinger]

That crash-report links to https://hg-edge.mozilla.org/releases/mozilla-release/file/179e4da8d58fdece16dcbf164c80199917a5946e/third_party/rust/quinn-udp/src/unix.rs#l565 which is the path for apple_slow. Is it possible that n == 0 in this case and we are reading uninitialised or zero-initialised memory?

[Ralith]

That seems plausible to me.

[thomaseizinger]

The Linux manpage says:

Datagram sockets in various domains (e.g., the UNIX and Internet domains) permit zero-size datagrams. When such a datagram is received, the return value is 0.

Should we continue here instead? Or also return 0? I think for the other codepaths we actually return 0 and let the caller handle that.

[Ralith]

Ah, on closer examination I'm not sure this is plausible. If we received no datagram, we should get n = -1 and EWOULDBLOCK or similar. If we receive a zero-length datagram, then name and hdr should be initialized with sensible values. In no case do we read memory in a weird state. The current code is already morally "also return 0", it's just filling in metadata along the way, just like the syscall itself should be.

Of course, this all assumes the kernel is well behaved.

[thomaseizinger]

The BSD manpage actually says this:

If no messages are available to be received and the peer has performed an orderly shut-down, 0 is returned.

That is also present in the Apple docs: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/recv.2.html

But is orderly shutdown a thing with UDP sockets? What would that even mean?

[Ralith]

That only makes sense to me as a reference to UDS. If Darwin is hitting that path somehow on UDP, then continue would get us stuck in a loop, but treating it like a datagram would mean reading uninit (or nonsense) metadata, and then we'd have the same loop problem on larger scale anyway...

[larseggert]

Older Apple kernels have, ahem, incomplete compliance with POSIX or their own man pages. I wouldn't rule out a kernel bug here.