Don't assume memory layout of std::net::SocketAddr (0.3.x) #120

faern · 2020-11-06T17:35:58Z

Fixes #119

Thomasdezeeuw · 2020-11-06T17:38:59Z

I just review tokio-rs/mio#1388, let's merge that first and than apply the same changes here. I'll also apply them to the master branch after.

faern · 2020-11-06T18:48:40Z

Cargo.toml

@@ -21,7 +21,7 @@ features = ["handleapi", "ws2def", "ws2ipdef", "ws2tcpip", "minwindef"]

 [target."cfg(any(unix, target_os = \"redox\"))".dependencies]
 cfg-if = "0.1.6"
-libc = "0.2.66"
+libc = { version = "0.2.66", features = ["align"] }


The added feature is so that libc::sockaddr_in6 does not have an extra __align field (used before #[repr(align(x))] was a thing.)

Do we need this for Mio as well?

faern · 2020-11-07T00:56:09Z

What's currently in this PR now passes both CI and builds and runs all tests when built on top of a toolchain where the memory layout of SocketAddrV4 and SocketAddrV6 has changed. So I think this code is all right currently.

With the help of [patch.crates-io] I have also made sure mio builds and passes all tests when depending on this version of socket2 and runs on my edited toolchain.

src/sockaddr.rs

faern · 2020-11-07T12:15:54Z

I feel happy with the content of this PR now. Ready for review.

Thomasdezeeuw

It seems my previous comment didn't go through (I still see them as pending in the GitHub UI), so there might be double comments.

Thomasdezeeuw · 2020-11-07T09:46:16Z

Cargo.toml

@@ -21,7 +21,7 @@ features = ["handleapi", "ws2def", "ws2ipdef", "ws2tcpip", "minwindef"]

 [target."cfg(any(unix, target_os = \"redox\"))".dependencies]
 cfg-if = "0.1.6"
-libc = "0.2.66"
+libc = { version = "0.2.66", features = ["align"] }


Do we need this for Mio as well?

src/sockaddr.rs

Thomasdezeeuw · 2020-11-07T13:38:07Z

Thanks again @faern, one question left about the align feature of libc: is it also needed for Mio?

faern · 2020-11-07T13:53:44Z

@Thomasdezeeuw I'm struggling to figure out when and where this is actually needed... Looks like my comment was a bit off, it's in6_addr that has an __align field on some platforms if this feature is not given. But a forgotten field would have given a compilation error, so it can't be needed in mio.

faern · 2020-11-07T13:56:03Z

Looks like this feature is automatically activated if compiling on Rust 1.25 or newer: https://github.com/rust-lang/libc/blob/master/build.rs#L57-L59

So maybe it's because socket2 has longer backwards compatibility? Does it?

faern · 2020-11-07T14:01:56Z

So it might be needed for the mio 0.6 backport? I'm not sure what the MSRV is for these projects, but it looks like mio 0.7 bumped it to 1.39 so it should not be needed there! If latest mio 0.6 is indeed still on MSRV 1.18 it will likely be needed for that backport.

For socket2 I don't find any MSRV and there is no changelog. So I have no idea 😮

rust-lang/socket2#120

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.

faern added 4 commits November 6, 2020 18:04

Convert SocketAddrV* -> SockAddr safely

7c25373

Convert SockAddr -> SocketAddrV* safely

b5627a2

cleanup

b831b44

Convert from SocketAddrV* to SockAddr with less copying

c582e73

Thomasdezeeuw mentioned this pull request Nov 6, 2020

Port pr #120 to master #121

Closed

faern commented Nov 6, 2020

View reviewed changes

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

faern added 2 commits November 7, 2020 01:09

Adapt code to different sockaddr struct impl on Windows

738612a

Get rid of mem::transmute in favor of correctly init values

09eb49b

faern force-pushed the fix-sockaddr-convertion branch from 09f54f2 to 09eb49b Compare November 7, 2020 00:45

faern added 2 commits November 7, 2020 02:01

Cleanup

2c084f0

Replace ..mem::zeroed() with conditional real fields

7cef159

faern force-pushed the fix-sockaddr-convertion branch from 98e93f7 to 7cef159 Compare November 7, 2020 01:10

Add safety docs to two unsafe usages

487ceb5

faern commented Nov 7, 2020

View reviewed changes

src/sockaddr.rs Outdated Show resolved Hide resolved

faern added 3 commits November 7, 2020 13:01

Initialize sockaddr_storage to zero at all times

36ea8a0

Remove redundant named struct field

eae8c9f

Much cleaner writing to uninitialized sockaddr_storage

3c97dd4

faern commented Nov 7, 2020

View reviewed changes

src/sockaddr.rs Outdated Show resolved Hide resolved

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

Thomasdezeeuw approved these changes Nov 7, 2020

View reviewed changes

Thomasdezeeuw merged commit d2c15de into rust-lang:v0.3.x Nov 7, 2020

faern mentioned this pull request Nov 7, 2020

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

Closed

faern mentioned this pull request Nov 8, 2020

Don't assume memory layout of std::net::SocketAddr (master) #122

Merged

faern changed the title ~~Don't assume memory layout of std::net::SocketAddr~~ Don't assume memory layout of std::net::SocketAddr (0.3.x) Nov 8, 2020

Thomasdezeeuw mentioned this pull request Nov 13, 2020

Need --cfg=libc_align in v0.3.16 but not v0.3.15; why? #130

Closed

faern added a commit to faern/miow that referenced this pull request Nov 13, 2020

Bump socket2 dependency to a version that does not assume memory layout

a538852

rust-lang/socket2#120

This was referenced Nov 13, 2020

Bump socket2 dependency to a version that does not assume memory layout yoshuawuyts/miow#37

Closed

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

Closed

faern added a commit to faern/miow that referenced this pull request Nov 14, 2020

Bump socket2 dependency to a version that does not assume memory layout

78c1919

rust-lang/socket2#120

faern added a commit to faern/miow that referenced this pull request Nov 14, 2020

Bump socket2 dependency to a version that does not assume memory layout

1791d79

rust-lang/socket2#120

faern added a commit to faern/miow that referenced this pull request Nov 14, 2020

Bump socket2 dependency to a version that does not assume memory layout

0bf65d2

rust-lang/socket2#120

faern added a commit to faern/miow that referenced this pull request Nov 14, 2020

Bump socket2 dependency to a version that does not assume memory layout

a715eea

rust-lang/socket2#120

faern added a commit to faern/miow that referenced this pull request Nov 14, 2020

Bump socket2 dependency to a version that does not assume memory layout

3bbf698

rust-lang/socket2#120

faern added a commit to faern/miow that referenced this pull request Nov 14, 2020

Bump socket2 dependency to a version that does not assume memory layout

da2e1f9

rust-lang/socket2#120

faern added a commit to faern/miow that referenced this pull request Nov 14, 2020

Bump socket2 dependency to a version that does not assume memory layout

0483f76

rust-lang/socket2#120

faern added a commit to faern/miow that referenced this pull request Nov 14, 2020

Bump socket2 dependency to a version that does not assume memory layout

98fd0bc

rust-lang/socket2#120

faern added a commit to faern/miow that referenced this pull request Nov 14, 2020

Bump socket2 dependency to a version that does not assume memory layout

9ef9c10

rust-lang/socket2#120

faern added a commit to faern/miow that referenced this pull request Nov 15, 2020

Bump socket2 dependency to a version that does not assume memory layout

38583a6

rust-lang/socket2#120

This was referenced Nov 29, 2020

Fix compilation on Haiku #136

Merged

Fix compilation on Haiku on 0.3.x #137

Merged

faern mentioned this pull request Dec 1, 2020

Yank all 0.3 versions older than 0.3.16 #139

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

klensy mentioned this pull request Apr 24, 2021

updated deps rust-lang/rustc-perf#868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't assume memory layout of std::net::SocketAddr (0.3.x) #120

Don't assume memory layout of std::net::SocketAddr (0.3.x) #120

faern commented Nov 6, 2020

Thomasdezeeuw commented Nov 6, 2020

faern Nov 6, 2020

Thomasdezeeuw Nov 7, 2020

faern commented Nov 7, 2020

faern commented Nov 7, 2020

Thomasdezeeuw left a comment

Thomasdezeeuw Nov 7, 2020

Thomasdezeeuw commented Nov 7, 2020

faern commented Nov 7, 2020

faern commented Nov 7, 2020

faern commented Nov 7, 2020

Don't assume memory layout of std::net::SocketAddr (0.3.x) #120

Don't assume memory layout of std::net::SocketAddr (0.3.x) #120

Conversation

faern commented Nov 6, 2020

Thomasdezeeuw commented Nov 6, 2020

faern Nov 6, 2020

Choose a reason for hiding this comment

Thomasdezeeuw Nov 7, 2020

Choose a reason for hiding this comment

faern commented Nov 7, 2020

faern commented Nov 7, 2020

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

Thomasdezeeuw Nov 7, 2020

Choose a reason for hiding this comment

Thomasdezeeuw commented Nov 7, 2020

faern commented Nov 7, 2020

faern commented Nov 7, 2020

faern commented Nov 7, 2020