Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 12 commits into from
Nov 7, 2020
Merged

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

merged 12 commits into from
Nov 7, 2020

Conversation

faern
Copy link
Contributor

@faern faern commented Nov 6, 2020

Fixes #119

@Thomasdezeeuw
Copy link
Collaborator

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.

@Thomasdezeeuw Thomasdezeeuw mentioned this pull request Nov 6, 2020
Closed
faern
faern commented Nov 6, 2020
View reviewed changes
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"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this for Mio as well?

@faern faern mentioned this pull request Nov 6, 2020
Merged
34 tasks
@faern faern force-pushed the fix-sockaddr-convertion branch from 09f54f2 to 09eb49b Compare November 7, 2020 00:45
@faern
Copy link
Contributor Author

faern commented Nov 7, 2020

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.

@faern faern force-pushed the fix-sockaddr-convertion branch from 98e93f7 to 7cef159 Compare November 7, 2020 01:10
faern
faern commented Nov 7, 2020
View reviewed changes
src/sockaddr.rs Outdated Show resolved Hide resolved
faern
faern commented Nov 7, 2020
View reviewed changes
src/sockaddr.rs Outdated Show resolved Hide resolved
@faern
Copy link
Contributor Author

faern commented Nov 7, 2020

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

@Thomasdezeeuw Thomasdezeeuw mentioned this pull request Nov 7, 2020
Closed
Thomasdezeeuw
Thomasdezeeuw approved these changes Nov 7, 2020
View reviewed changes
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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"] }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this for Mio as well?

src/sockaddr.rs Outdated Show resolved Hide resolved
@Thomasdezeeuw Thomasdezeeuw merged commit d2c15de into rust-lang:v0.3.x Nov 7, 2020
@Thomasdezeeuw
Copy link
Collaborator

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

@faern
Copy link
Contributor Author

faern commented Nov 7, 2020

@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
Copy link
Contributor Author

faern commented Nov 7, 2020

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
Copy link
Contributor Author

faern commented Nov 7, 2020

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 😮

@faern faern mentioned this pull request Nov 7, 2020
Closed
@faern faern mentioned this pull request Nov 8, 2020
Merged
@faern 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 Thomasdezeeuw mentioned this pull request Nov 13, 2020
Closed
faern added a commit to faern/miow that referenced this pull request Nov 13, 2020
@faern
Bump socket2 dependency to a version that does not assume memory layout
a538852 
rust-lang/socket2#120
This was referenced Nov 13, 2020
Closed
Closed
faern added a commit to faern/miow that referenced this pull request Nov 14, 2020
@faern
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
@faern
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
@faern
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
@faern
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
@faern
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
@faern
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
@faern
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
@faern
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
@faern
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
@faern
Bump socket2 dependency to a version that does not assume memory layout
38583a6 
rust-lang/socket2#120
This was referenced Nov 29, 2020
Merged
Merged
@faern faern mentioned this pull request Dec 1, 2020
Closed
This was referenced Mar 21, 2021
Closed
Open
Open
zonyitoo added a commit to shadowsocks/shadowsocks-rust that referenced this pull request Mar 22, 2021
@zonyitoo
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
@zonyitoo
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 klensy mentioned this pull request Apr 24, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 31, 2022
@bors
Auto merge of rust-lang#78802 - faern:simplify-socketaddr, r=joshtrip…
3405e40 
…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.
workingjubilee pushed a commit to tcdi/postgrestd that referenced this pull request Sep 15, 2022
@bors
Auto merge of #78802 - faern:simplify-socketaddr, r=joshtriplett
06e77d2 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thomasdezeeuw Thomasdezeeuw Thomasdezeeuw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@faern @Thomasdezeeuw