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

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.

Why not use a u128 here?

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.

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.

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.

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.

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.

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.

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.

rust-highfive assigned sfackler Nov 6, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 6, 2020

This was referenced Nov 6, 2020

The code assumes the memory layout of SocketAddrV{4,6} rust-lang/socket2#119

Closed

Don't assume memory layout of std::net::SocketAddr tokio-rs/mio#1388

Merged

est31 reviewed Nov 6, 2020

View reviewed changes

Comment thread library/std/src/net/ip.rs Outdated Show resolved Hide resolved

jyn514 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 6, 2020

Thomasdezeeuw mentioned this pull request Nov 7, 2020

net2 assumes the layout of std::net::SocketAddrV{4,6} matches libc::sockaddr deprecrated/net2-rs#105

Closed

faern mentioned this pull request Nov 7, 2020

Don't assume memory layout of std::net::SocketAddr deprecrated/net2-rs#106

Closed

faern force-pushed the simplify-socketaddr branch from ef7343b to a3c4764 Compare November 11, 2020 14:08

faern added a commit to mullvad/mullvadvpn-app that referenced this pull request Nov 16, 2020

Upgrade socket2 and miow …

de857fc

For context: rust-lang/rust#78802

faern mentioned this pull request Nov 16, 2020

Upgrade Rust dependencies mullvad/mullvadvpn-app#2272

Merged

2 tasks

faern added a commit to mullvad/mullvadvpn-app that referenced this pull request Nov 17, 2020

Upgrade socket2 and miow …

6e0e2ea

For context: rust-lang/rust#78802

ghedo mentioned this pull request Aug 1, 2022

ffi: fix conversions between SocketAddr and strust sockaddr cloudflare/quiche#1290

Merged

matthiaskrgr mentioned this pull request Aug 2, 2022

Rollup of 8 pull requests #100048

Merged

jeff-hiner mentioned this pull request Aug 8, 2022

No sound way to zero-copy from socketaddr_in rust-lang/socket2#323

Open

the8472 mentioned this pull request Aug 17, 2022

regression: socket type changes #100551

Closed

Noah-Kennedy mentioned this pull request Aug 25, 2022

OsSocketAddr is unsound in upcoming rustc release a-ba/os_socketaddr#3

Closed

sunfishcode mentioned this pull request Sep 8, 2022

impl FromStr for Ipv4Addr bytecodealliance/rustix#397

Closed

ghost mentioned this pull request Sep 10, 2022

Changes in memory layout of the std net types will result in UB bnoordhuis/netif#9

Closed

caass mentioned this pull request Sep 12, 2022

Tracking issue for rust-toolchain version cloudflare/wrangler-legacy#2308

Closed

yvt mentioned this pull request Oct 22, 2022

Bump deps, determine the new MSRV nrdxp/cfdyndns#13

Closed

faern mentioned this pull request Nov 10, 2022

Move IpAddr, SocketAddr and V4+V6 related types to core #104265

Merged

StripedMonkey mentioned this pull request Dec 13, 2022

Assist/Codelens to show size of type rust-lang/rust-analyzer#4091

Closed

Alcaro mentioned this pull request Apr 10, 2023

No such device (os error 19) imsnif/bandwhich#263

Closed

jcgruenhage mentioned this pull request Apr 10, 2023

Misc dependency upgrades imsnif/bandwhich#265

Closed

stevenengler mentioned this pull request Jun 6, 2023

Invalid transmute from std::net::Ipv4Addr to libc::in_addr nix-rust/nix#2053

Closed

ptitdoc mentioned this pull request Jul 25, 2023

Cannot build since rust 1.64 : cannot transmute between types of different size... phsym/rust-sctp#12

Closed

ptitdoc mentioned this pull request Sep 10, 2024

migrate to rust 1.64 network primitives phsym/rust-sctp#13

Merged

mkeeter mentioned this pull request Jan 8, 2025

Bump socket2 dep mkeeter/pont#12

Merged

nathaniel-daniel mentioned this pull request Jan 13, 2025

std::fs::canonicalize returns UNC paths on Windows, and a lot of software doesn't support UNC paths #42869

Open

cuviper mentioned this pull request Feb 7, 2025

Reword incorrect documentation about SocketAddr having varying layout #136230

Merged

nicholasbishop mentioned this pull request May 7, 2025

uefi: use core::net types in public API, remove net type duplicates rust-osdev/uefi-rs#1645

Closed

3 tasks

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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