Don't assume memory layout of std::net::SocketAddr #39
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@yoshuawuyts Would it be possible to have this reviewed and published? This is currently a blocker for getting the invalid memory assumption out of the |
|
Thanks for the push access / crate ownership. I'm working a bit more on some tests for now. Once they pass I will merge. |
faern
force-pushed
the
fix-sockaddr-convertion
branch
from
80f5a4f
to
e1ec48b
Compare
Do not rely on the memory layout of std::net::SocketAddrV*
faern
force-pushed
the
fix-sockaddr-convertion
branch
from
e1ec48b
to
2bb8c35
Compare
faern
force-pushed
the
fix-sockaddr-convertion
branch
from
2bb8c35
to
69a13dd
Compare
|
The CI is broken/disabled. But it builds locally for me back to Rust 1.36.0 on Windows 10. |
bors
added a commit
to rust-lang/cargo
that referenced
this pull request
Nov 29, 2020
Bump miow dependency to not invalidly assume memory layout See rust-lang/rust#78802 and yoshuawuyts/miow#39
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 31, 2022
…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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #38. Makes #37 obsolete...
Stops just converting from
std::net::SocketAddr->SOCKADDRby casting the pointer. The memory layout of the types in the standard library can't be assumed to be stable.The
socket2minimum version bump is because that crate also made this assumption. But0.3.16has been fixed and no longer does that.Helps unblock rust-lang/rust#78802