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

The code assumes the memory layout of SocketAddrV{4,6} #119

Closed
faern opened this issue Nov 6, 2020 · 8 comments
Closed

The code assumes the memory layout of SocketAddrV{4,6} #119

faern opened this issue Nov 6, 2020 · 8 comments
Labels
bug

Comments

@faern
Copy link
Contributor

faern commented Nov 6, 2020

This library casts std::net::SocketAddrV4 (and V6) into libc::sockaddr: https://github.com/rust-lang/socket2-rs/blob/b0f77842b3357dd571bb86b30b354bc55215f693/src/sockaddr.rs#L95-L115

As far as I can tell there are no guarantees from std about the layout of SocketAddrV{4,6}, and this code could silently compile and cause UB elsewhere if the representation changes.

This internals forum thread is where this discussion started: https://internals.rust-lang.org/t/why-are-socketaddrv4-socketaddrv6-based-on-low-level-sockaddr-in-6/13321

mio does the same kind of invalid casting: tokio-rs/mio#1386
@faern faern changed the title The code invalidly assumes the memory layout of SocketAddrV{4,6} The code assumes the memory layout of SocketAddrV{4,6} Nov 6, 2020
@faern faern mentioned this issue Nov 6, 2020
Merged
34 tasks
@Thomasdezeeuw
Copy link
Collaborator

@Nemo157 made me aware of this in tokio-rs/mio#1386. I didn't know about rust-lang/rust#78802, but if that will proceed than I'll fix both Mio and socket2 in the same way.

@Thomasdezeeuw Thomasdezeeuw mentioned this issue Nov 6, 2020
Closed
@faern
Copy link
Contributor Author

faern commented Nov 6, 2020 •
edited
Loading

I think this needs to be fixed no matter what happens with rust-lang/rust#78802. Because both these crates still rely on a non specified memory layout. Having these assumptions in commonly used crates makes it more troublesome for the standard library to evolve and change internal details that should not matter to the outside world.

@sfackler
Copy link
Member

sfackler commented Nov 6, 2020 •
edited
Loading

For reference, this assumption was made because Alex wrote the implementation of std's SocketAddr and then also wrote the implementation of this library.

@faern faern mentioned this issue Nov 6, 2020
Merged
This was referenced Nov 6, 2020
Closed
Closed
@faern
Copy link
Contributor Author

faern commented Nov 8, 2020

Fixed in #120

@faern faern closed this as completed Nov 8, 2020
@faern
Copy link
Contributor Author

faern commented Nov 11, 2020

@Thomasdezeeuw Thanks for the fast and smooth collaboration on getting this fixed in all the crates you maintain! I know you said you wanted to get something more merged before releasing mio, but is there anything blocking releasing a patch release of socket2 0.3.x with this fix?

@de-vri-es
Copy link
Contributor

de-vri-es commented Nov 11, 2020 •
edited
Loading

I think #117 needs to be merged before releasing, or Socket::new() would change to not setting the CLOEXEC flag. But that has some requests for changes and it needs to be rebased on #124 first, which itself isn't merged yet.

@Thomasdezeeuw
Copy link
Collaborator

@de-vri-es #117 and #124 are for the master branch which will become v0.4.x. @faern I'll prepare a release.

@Thomasdezeeuw
Copy link
Collaborator

@faern version v0.3.16 is released.

Xanewok added a commit to Xanewok/rls that referenced this issue Jan 15, 2021
@Xanewok
Bump net2 dependency
36f4a9e 
See rust-lang/socket2#119 for more details.
bors added a commit to rust-lang-ci/rust that referenced this issue 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 issue 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.
@talklittle talklittle mentioned this issue Jul 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@faern @de-vri-es @sfackler @Thomasdezeeuw