Implement network primitives with ideal Rust layout, not C system layout

[faern]

This PR is the result of this internals forum thread: https://internals.rust-lang.org/t/why-are-socketaddrv4-socketaddrv6-based-on-low-level-sockaddr-in-6/13321.

Instead of basing std:::net::{Ipv4Addr, Ipv6Addr, SocketAddrV4, SocketAddrV6} on system (C) structs, they are encoded in a more optimal and idiomatic Rust way.

This changes the public API of std by introducing structural equality impls for all four types here, which means that match ipv4addr { SOME_CONSTANT => ... } will now compile, whereas previously this was an error. No other intentional changes are introduced to public API.

It's possible to observe the current layout of these types (e.g., by pointer casting); most but not all libraries which were found by Crater to do this have had updates issued and affected versions yanked. See report below.

Benefits of this change

• It will become possible to move these fundamental network types from std into core (RFC).
• Some methods that can't be made const fns today can be made const fns with this change.
• SocketAddrV4 only occupies 6 bytes instead of 16 bytes.
• These simple primitives become easier to read and uses less unsafe.
• Makes these types support structural equality, which means you can now (for instance) match an Ipv4Addr against a constant

Remaining Previous problems

This change obviously changes the memory layout of the types. And it turns out some libraries invalidly assumes the memory layout and does very dangerous pointer casts to convert them. These libraries will have undefined behaviour and perform invalid memory access until patched.

• - mio - Issue: mio::sys::unix::net::socket_addr assumes the layout of std::net::SocketAddrV{4,6} matches libc::sockaddr tokio-rs/mio#1386.
  • 0.7 branch Don't assume memory layout of std::net::SocketAddr tokio-rs/mio#1388
  • 0.7.6 published Release v0.7.6 tokio-rs/mio#1398
  • Yank all 0.7 versions older than 0.7.6
  • Report <0.7.6 to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0081.html
• - socket2 - Issue: The code assumes the memory layout of SocketAddrV{4,6} socket2#119.
  • 0.3.x branch Don't assume memory layout of std::net::SocketAddr (0.3.x) socket2#120
  • 0.3.16 published
  • master branch Don't assume memory layout of std::net::SocketAddr (master) socket2#122
  • Yank all 0.3 versions older than 0.3.16
  • Report <0.3.16 to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0079.html
• - net2 - Issue: net2 assumes the layout of std::net::SocketAddrV{4,6} matches libc::sockaddr deprecrated/net2-rs#105
  • Don't assume memory layout of std::net::SocketAddr deprecrated/net2-rs#106
  • 0.2.36 published
  • Yank all 0.2 versions older than 0.2.36
  • Report <0.2.36 to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0078.html
• - miow - Issue: Invalidly assumes the memory layout of SocketAddrV{4,6} yoshuawuyts/miow#38
  • 0.3.x - Don't assume memory layout of std::net::SocketAddr yoshuawuyts/miow#39
  • 0.3.6 published
  • 0.2.x - Don't assume memory layout of std::net::SocketAddr (0.2.x) yoshuawuyts/miow#40
  • 0.2.2 published
  • Yanked all 0.2 versions older than 0.2.2
  • Yanked all 0.3 versions older than 0.3.6
  • Report <0.2.2 and <0.3.6 to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0080.html
• - quinn master (aka what became 0.7) - Invalid assumption about SocketAddrV{4,6} layout quinn-rs/quinn#968 Sound addr quinn-rs/quinn#987
  • - quinn 0.6 - [0.6.x] Don't assume representation of SocketAddr quinn-rs/quinn#1045
  • - quinn 0.5 - [0.5.x] Don't assume representation of SocketAddr quinn-rs/quinn#1046
  • - Release 0.7.0, 0.6.2 and 0.5.4
• - nb-connect - Invalid assumption of SocketAddrV{4,6} layout smol-rs/nb-connect#1
  • - Release 1.0.3
  • - Yank all versions older than 1.0.3
• - shadowsocks-rust - Invalid layout assumptions for std::net::SocketAddrV{4, 6} shadowsocks/shadowsocks-rust#462
• - rio - Invalid layout assumptions for std::net::SocketAddrV{4, 6} spacejam/rio#44
• - seaslug - Invalid layout assumptions for std::net::SocketAddrV{4, 6} spacejam/seaslug#1

Fixed crate versions

All crates I have found that assumed the memory layout have been fixed and published. The crates and versions that will continue working even as/if this PR is merged is (please upgrade these to help unblock this PR):

• net2 0.2.36
• socket2 0.3.16
• miow 0.2.2
• miow 0.3.6
• mio 0.7.6
• mio 0.6.23 - Never had the invalid assumption itself, but has now been bumped to only allow fixed dependencies (net2 + miow)
• nb-connect 1.0.3
• quinn 0.5.4
• quinn 0.6.2

Release notes draft

This release changes the memory layout of Ipv4Addr, Ipv6Addr, SocketAddrV4 and SocketAddrV6. The standard library no longer implements these as the corresponding libc structs (sockaddr_in, sockaddr_in6 etc.). This internal representation was never exposed, but some crates relied on it anyway by unsafely transmuting. This change will cause those crates to make invalid memory accesses. Notably net2 <0.2.36, socket2 <0.3.16, mio <0.7.6, miow <0.3.6 and a few other crates are affected. All known affected crates have been patched and have had fixed versions published over a year ago. If any affected crate is still in your dependency tree, you need to upgrade them before using this version of Rust.

[rust-highfive]

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

[Thomasdezeeuw]

Is the reduced size the only argument for this change? I'm opposed to it per say, but it will cost some compute to go from Rust's address to C's address format. I imagine the most common use case for most address types is use in system calls, so are there any benchmarks for this?

Also I maintain both Mio and socket2 so changes those shouldn't be problem, thanks for the heads-up.

[faern]

Is the reduced size the only argument for this change?

No. There is a list of benefits in the PR description. I think the most asked for is the move to core. Personally I really like the constification.

I'm working on benchmarks. See the internals forums threads linked in the PR description. But my initial results show that any conversion is dirt cheap anyway so it does not really matter.

[est31]

Why not use a u128 here?

[faern]

Any benefit of doing that? It looks like the value is accessed as an array more often, so this felt like a more natural representation. Also it lowers the memory alignment from 4 to 1.

[nagisa]

It is not entirely clear to me that a lowered alignment is actually beneficial. At least given the fact there will now be a fair number of conversions between this type and the libc one when interacting with the system APIs, the alignment of 1 effectively forces (especially on architectures with strict alignment requirements) a memcpy for each such conversion.

[faern]

If you are talking about performance I would argue it's not a valid argument unless proven. As everything regarding optimization. Can you find any usage of this type where it's converted to/from the system representation where said conversion takes up more than a tiny fraction of the time the entire syscall takes?

I'm not saying my benchmarks are perfect. But I have at least tried to check this, and I have not found anything indicating this PR makes anything slower: https://internals.rust-lang.org/t/why-are-socketaddrv4-socketaddrv6-based-on-low-level-sockaddr-in-6/13321/13

I'm not saying alignment 1 is strictly better than 4. Just that I can't find any downsides. And it would be artificial to try to keep it at 4 unless needed, since the most natural representation does not have alignment 4 automatically.

[CDirkx]

If the representation was a u128 then checking if an address is in a prefix like ::ffff/96 (IPv4-mapped addresses) could be efficiently computed with:

let mask: u128 = u128::MAX << (128 - 96);
self.inner & mask == Ipv6Addr::from("::ffff").inner

Still possible if the representation is [u8; 16] using u128::from_be_bytes, but because of the alignment that might require a copy.

[JelteF]

The soon to be released Go 1.18 includes a whole new IP package called net/netip: https://github.com/golang/go/blob/master/src/net/netip/netip.go

A blogpost on its design can be found here (the package was originally called inet.af/netaddr here, because it was not in the stdlib): https://tailscale.com/blog/netaddr-new-ip-type-for-go/

Importantly for this discussion is that using a pair of uint64 (Go does not have native uint128 support) was quite a bit faster than using an array of bytes. See commit message here for details: inetaf/netaddr@4eb479d

I think it's definitely worth running similar benchmarks here.

[faern]

I agree that we should run benchmarks and try to find the best internal representation. But I don't think it should block this PR. Unless we think the PR as is will mean performance regressions I think it brings so many other nice things to the table that it can be ignored for now and iterated upon later. The memory layout of these types are not part of the public API and can change any number of times after this PR is merged.

[Kixunil]

Regarding perf regressions, this is most likely used around syscalls and thus irrelevant. Various Rust APIs like File already heap allocate on syscalls based on same arguments and heap allocations are significantly slower than a few movs here.

[sfackler]

This is not going to be able to land for quite a while, FYI.

[djc]

This is not going to be able to land for quite a while, FYI.

Why is that?

[sfackler]

Because it will break large swaths of the ecosystem.

[Thomasdezeeuw]

Because it will break large swaths of the ecosystem.

@faern already fixed Mio (tokio-rs/mio#1388) and socket2 (rust-lang/socket2#120), I'll release updates for both next week. Could we run crater after that to test what crates also break?

[faern]

Also waiting for this to be fixed in net2 (deprecrated/net2-rs#106). But after all of those crates have been fixed and released I'm aiming to make this PR pass the CI.

[faern]

socket2 0.3.16 has been published, with this issue fixed. The first one out in the ecosystem, and also the crate that created CI failures for this PR. This PR now bumps socket2 in the main Cargo.lock (just to see if it helps the CI pass). I'll probably split all such dependency upgrades up to a separate PR later. Because the upgrades should be a no-brainer, but this PR will take a while before it lands. And lockfiles are notoriously conflicty. But I'll wait for net2 and mio to land first.

This repository is a giant workspace with a Cargo.lock at the top. And src/tools/rls is part of this workspace. So it should use the same single lockfile. But rls is also a submodule and its own repository. So there is also a src/tools/rls/Cargo.lock. A bit messy. So I guess each tool in a submodule must have version bumping done to them separately as well.

[Xanewok]

@faern only top-level lockfile will be respected when building a workspace member package (which RLS is), and so its lockfile will be effectively ignored when building RLS in the context of this workspace.

I can update RLS' lockfile separately in https://github.com/rust-lang/rls but this should not block anything here.

[faern]

There is good progress. Out of all the offending crates we have already had this fixed and published in socket2 0.3.16, miow 0.3.6 and mio 0.7.6. By extension this means tokio 0.3 is now on the safe side of things. 🎉

The problem still exists in the tokio 0.2/mio 0.6 ecosystems though. And the vast majority of projects is still using that version and probably will for a long time. To unblock this, we need to get rid of this invalid casting in net2. A deprecated crate, that can hopefully receive some love for this type of issue. According to crates.io, that crate is owned by the rust-lang library team. Would it be possible to have someone from that team bless deprecrated/net2-rs#106? @sfackler @m-ou-se

[CDirkx]

Note that the change to #[derive(PartialEq, Eq)] makes the primitives #[structural_match], allowing new code such as:

let ip : Ipv4Addr = ...;

match ip {
  Ipv4Addr::LOCALHOST => { ... },
  _ => { ... }
}

This becomes part of the contract of these primitives, falling under the stability guarantees of the standard library, which is why I propose adding a regression test similar those added when the representation of RangeInclusive was changed and the type became #[structural_match]:

rust/library/core/tests/ops.rs

Lines 161 to 199 in c6a6105

	#[test]
	fn range_structural_match() {
	// test that all range types can be structurally matched upon
	const RANGE: Range<usize> = 0..1000;
	match RANGE {
	RANGE => {}
	_ => unreachable!(),
	}
	const RANGE_FROM: RangeFrom<usize> = 0..;
	match RANGE_FROM {
	RANGE_FROM => {}
	_ => unreachable!(),
	}
	const RANGE_FULL: RangeFull = ..;
	match RANGE_FULL {
	RANGE_FULL => {}
	}
	const RANGE_INCLUSIVE: RangeInclusive<usize> = 0..=999;
	match RANGE_INCLUSIVE {
	RANGE_INCLUSIVE => {}
	_ => unreachable!(),
	}
	const RANGE_TO: RangeTo<usize> = ..1000;
	match RANGE_TO {
	RANGE_TO => {}
	_ => unreachable!(),
	}
	const RANGE_TO_INCLUSIVE: RangeToInclusive<usize> = ..=999;
	match RANGE_TO_INCLUSIVE {
	RANGE_TO_INCLUSIVE => {}
	_ => unreachable!(),
	}
	}

[faern]

This has now been in stable for a full release cycle. Seems to have worked out well. Maybe it's time to move on with the improvements this allows for? For example stabilizing the constructors as const and move the types into core.