Don't assume memory layout of std::net::SocketAddr #1388

faern · 2020-11-06T09:09:03Z

This is so far only implemented in the unix implementation. A very similar thing has to be done for the Windows module. But I felt I could get this posted before that to get some feedback.

Thomasdezeeuw · 2020-11-06T09:15:15Z

I was thinking about simply recreating SocketAddr/libc::sockaddr_storage, dropping the use of references to it. This would be especially useful if rust-lang/rust#78802 is merged. I'll take a look at this this weekend.

faern · 2020-11-06T09:25:24Z

You mean mio would not use the standard library SocketAddr type? Or you mean this PR should use libc::sockaddr_storage instead of the custom SocketAddrCRepr? The former does not sound good IMO. The intention with rust-lang/rust#78802 was not to split the ecosystem, but rather unify it by allowing it into core etc.

Thomasdezeeuw · 2020-11-06T09:32:44Z

You mean mio would not use the standard library SocketAddr type? Or you mean this PR should use libc::sockaddr_storage instead of the custom SocketAddrCRepr? The former does not sound good IMO. The intention with rust-lang/rust#78802 was not to split the ecosystem, but rather unify it by allowing it into core etc.

No, Mio will continue to use SocketAddr from std lib. Instead I would create libc::sockaddr_in(6) manually from SocketAddr and for accept create SocketAddr from libc::sockaddr_storage (something we already for UDS).

faern · 2020-11-06T10:12:23Z

I see. Makes sense 👍. I will also look more into this in the weekend. I want to submit a PR to socket2 as well since that's a bit blocking for my main PR on the standard library.

Thomasdezeeuw · 2020-11-06T10:19:44Z

I see. Makes sense 👍. I will also look more into this in the weekend. I want to submit a PR to socket2 as well since that's a bit blocking for my main PR on the standard library.

For socket2 make a pr against the v0.3.x branch, master is for v0.4 which is quite a bit away from a release.

Thomasdezeeuw

After looking into a bit I think this is indeed the best way forward. I've added some small inline comments but overall this looks good.

src/sys/unix/net.rs

src/sys/windows/tcp.rs

faern · 2020-11-06T20:35:28Z

Now I have implemented the same for Windows. I might have missed some spot since any place that still just does the casting will just work with the current versions of Rust... That's the danger of that casting, the type system won't catch it.

I will need to get my custom toolchain built on top of rust-lang/rust#78802 on Windows and try out this branch there...

faern · 2020-11-06T23:31:39Z

@Thomasdezeeuw Oh... socket2 is a dependency to mio on Windows... So it sounds smoother if we fix it the other way around really. I currently can't build and run mio under my toolchain on Windows. Well I can hack around it with the Cargo [patch] section of course, but anyway, seems a bit backwards.

src/sys/windows/net.rs

Thomasdezeeuw

If you can fix the transmute and not panic in get_localaddr than this LGTM.

src/sys/windows/tcp.rs

faern · 2020-11-07T11:20:43Z

@Thomasdezeeuw Would you like me to squash the git history a bit, or do you prefer it this way? Either way is fine with me.

Thomasdezeeuw · 2020-11-07T11:33:55Z

I'll squash on merging.

Thomasdezeeuw · 2020-11-07T11:36:56Z

Thanks @faern!

faern · 2020-11-07T11:45:51Z

Thank you! I hope we can get this fix published in all relevant versions of mio and socket2 soon :)

I realized I did not add it to the changelog. Will you do that or should I submit a follow up PR?

Thomasdezeeuw · 2020-11-07T11:47:20Z

Thank you! I hope we can get this fix published in all relevant versions of mio and socket2 soon :)

I'll likely release new versions for both next week or so.

I realized I did not add it to the changelog. Will you do that or should I submit a follow up PR?

No I usually update the changelog while preparing for a release.

SocketAddr may not have the same layout as libc, rust-lang/rust#78802 - fixes #462 - ref tokio-rs/mio#1388, rust-lang/socket2#120

…lett Implement network primitives with ideal Rust layout, not C system layout 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](rust-lang/rfcs#2832)). - Some methods that can't be made `const fn`s today can be made `const fn`s 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. - [x] - `mio` - Issue: tokio-rs/mio#1386. - [x] `0.7` branch tokio-rs/mio#1388 - [x] `0.7.6` published tokio-rs/mio#1398 - [x] Yank all `0.7` versions older than `0.7.6` - [x] Report `<0.7.6` to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0081.html - [x] - `socket2` - Issue: rust-lang/socket2#119. - [x] `0.3.x` branch rust-lang/socket2#120 - [x] `0.3.16` published - [x] `master` branch rust-lang/socket2#122 - [x] Yank all `0.3` versions older than `0.3.16` - [x] Report `<0.3.16` to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0079.html - [x] - `net2` - Issue: deprecrated/net2-rs#105 - [x] deprecrated/net2-rs#106 - [x] `0.2.36` published - [x] Yank all `0.2` versions older than `0.2.36` - [x] Report `<0.2.36` to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0078.html - [x] - `miow` - Issue: yoshuawuyts/miow#38 - [x] `0.3.x` - yoshuawuyts/miow#39 - [x] `0.3.6` published - [x] `0.2.x` - yoshuawuyts/miow#40 - [x] `0.2.2` published - [x] Yanked all `0.2` versions older than `0.2.2` - [x] Yanked all `0.3` versions older than `0.3.6` - [x] Report `<0.2.2` and `<0.3.6` to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0080.html - [x] - `quinn master` (aka what became 0.7) - quinn-rs/quinn#968 quinn-rs/quinn#987 - [x] - `quinn 0.6` - quinn-rs/quinn#1045 - [x] - `quinn 0.5` - quinn-rs/quinn#1046 - [x] - Release `0.7.0`, `0.6.2` and `0.5.4` - [x] - `nb-connect` - smol-rs/nb-connect#1 - [x] - Release `1.0.3` - [x] - Yank all versions older than `1.0.3` - [x] - `shadowsocks-rust` - shadowsocks/shadowsocks-rust#462 - [ ] - `rio` - spacejam/rio#44 - [ ] - `seaslug` - 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.

Implement network primitives with ideal Rust layout, not C system layout 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](rust-lang/rfcs#2832)). - Some methods that can't be made `const fn`s today can be made `const fn`s 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. - [x] - `mio` - Issue: tokio-rs/mio#1386. - [x] `0.7` branch tokio-rs/mio#1388 - [x] `0.7.6` published tokio-rs/mio#1398 - [x] Yank all `0.7` versions older than `0.7.6` - [x] Report `<0.7.6` to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0081.html - [x] - `socket2` - Issue: rust-lang/socket2#119. - [x] `0.3.x` branch rust-lang/socket2#120 - [x] `0.3.16` published - [x] `master` branch rust-lang/socket2#122 - [x] Yank all `0.3` versions older than `0.3.16` - [x] Report `<0.3.16` to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0079.html - [x] - `net2` - Issue: deprecrated/net2-rs#105 - [x] deprecrated/net2-rs#106 - [x] `0.2.36` published - [x] Yank all `0.2` versions older than `0.2.36` - [x] Report `<0.2.36` to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0078.html - [x] - `miow` - Issue: yoshuawuyts/miow#38 - [x] `0.3.x` - yoshuawuyts/miow#39 - [x] `0.3.6` published - [x] `0.2.x` - yoshuawuyts/miow#40 - [x] `0.2.2` published - [x] Yanked all `0.2` versions older than `0.2.2` - [x] Yanked all `0.3` versions older than `0.3.6` - [x] Report `<0.2.2` and `<0.3.6` to RustSec Advisory Database https://rustsec.org/advisories/RUSTSEC-2020-0080.html - [x] - `quinn master` (aka what became 0.7) - quinn-rs/quinn#968 quinn-rs/quinn#987 - [x] - `quinn 0.6` - quinn-rs/quinn#1045 - [x] - `quinn 0.5` - quinn-rs/quinn#1046 - [x] - Release `0.7.0`, `0.6.2` and `0.5.4` - [x] - `nb-connect` - smol-rs/nb-connect#1 - [x] - Release `1.0.3` - [x] - Yank all versions older than `1.0.3` - [x] - `shadowsocks-rust` - shadowsocks/shadowsocks-rust#462 - [ ] - `rio` - spacejam/rio#44 - [ ] - `seaslug` - 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.

Don't assume memory layout of std::net::SocketAddr

a790f99

faern mentioned this pull request Nov 6, 2020

Implement network primitives with ideal Rust layout, not C system layout rust-lang/rust#78802

Merged

34 tasks

Thomasdezeeuw reviewed Nov 6, 2020

View reviewed changes

This was referenced Nov 6, 2020

Don't assume memory layout of std::net::SocketAddr (0.3.x) rust-lang/socket2#120

Merged

Backport pr #1388 to v0.6 #1390

Closed

faern added 6 commits November 6, 2020 18:43

Remove mem::zeroed() in favor of explicitly initializing fields

6c47b15

Move import to top of file

036cc9a

Add proper safety documentation to to_socket_addr

2c08872

Add sin_len to sockaddr_in

f0ef994

Initialize __sin6_src_id on solarish

022a89d

Make SocketAddrCRepr::as_ptr a pub(crate)

102b629

faern force-pushed the fix-sockaddr-convertion branch 2 times, most recently from 085fc19 to d32ff16 Compare November 6, 2020 19:32

faern commented Nov 6, 2020

View reviewed changes

src/sys/windows/tcp.rs Outdated Show resolved Hide resolved

faern force-pushed the fix-sockaddr-convertion branch from 0260ee5 to a3a6c15 Compare November 6, 2020 20:32

Windows: Don't assume memory layout of std::net::SocketAddr

75cdea2

faern force-pushed the fix-sockaddr-convertion branch from 5413e26 to a00e696 Compare November 6, 2020 20:43

faern commented Nov 6, 2020

View reviewed changes

src/sys/windows/net.rs Outdated Show resolved Hide resolved

Thomasdezeeuw reviewed Nov 7, 2020

View reviewed changes

src/sys/windows/tcp.rs Outdated Show resolved Hide resolved

faern added 3 commits November 7, 2020 11:20

Windows: Make converting SOCKADDR -> SocketAddr safe

b6f89d6

Replace panic with error value

ba9a2f9

Replace transmuting with better winapi union handling

3826266

faern force-pushed the fix-sockaddr-convertion branch from a00e696 to 3826266 Compare November 7, 2020 11:01

More consistent naming

8d3ed77

Thomasdezeeuw approved these changes Nov 7, 2020

View reviewed changes

Thomasdezeeuw merged commit 152e075 into tokio-rs:master Nov 7, 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

This was referenced Nov 7, 2020

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

Closed

Invalidly assumes the memory layout of SocketAddrV{4,6} yoshuawuyts/miow#38

Closed

faern mentioned this pull request Dec 1, 2020

Yank all 0.7 versions older than 0.7.6 #1412

Closed

zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Mar 22, 2021

convert libc sockaddr to SocketAddr explicitly

772f3c1

SocketAddr may not have the same layout as libc, rust-lang/rust#78802 - fixes #462 - ref tokio-rs/mio#1388, rust-lang/socket2#120

zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Mar 22, 2021

convert libc sockaddr to SocketAddr explicitly

587e566

SocketAddr may not have the same layout as libc, rust-lang/rust#78802 - fixes #462 - ref tokio-rs/mio#1388, rust-lang/socket2#120

tdunlap607 mentioned this pull request Apr 11, 2023

[GHSA-pf3p-x6qj-6j7q] mio invalidly assumes the memory layout of std::net::SocketAddr github/advisory-database#2047

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't assume memory layout of std::net::SocketAddr #1388

Don't assume memory layout of std::net::SocketAddr #1388

faern commented Nov 6, 2020

Thomasdezeeuw commented Nov 6, 2020

faern commented Nov 6, 2020

Thomasdezeeuw commented Nov 6, 2020

faern commented Nov 6, 2020

Thomasdezeeuw commented Nov 6, 2020

Thomasdezeeuw left a comment

faern commented Nov 6, 2020

faern commented Nov 6, 2020

Thomasdezeeuw left a comment

faern commented Nov 7, 2020

Thomasdezeeuw commented Nov 7, 2020

Thomasdezeeuw commented Nov 7, 2020

faern commented Nov 7, 2020

Thomasdezeeuw commented Nov 7, 2020

Don't assume memory layout of std::net::SocketAddr #1388

Don't assume memory layout of std::net::SocketAddr #1388

Conversation

faern commented Nov 6, 2020

Thomasdezeeuw commented Nov 6, 2020

faern commented Nov 6, 2020

Thomasdezeeuw commented Nov 6, 2020

faern commented Nov 6, 2020

Thomasdezeeuw commented Nov 6, 2020

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

faern commented Nov 6, 2020

faern commented Nov 6, 2020

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

faern commented Nov 7, 2020

Thomasdezeeuw commented Nov 7, 2020

Thomasdezeeuw commented Nov 7, 2020

faern commented Nov 7, 2020

Thomasdezeeuw commented Nov 7, 2020