Repository navigation
The code assumes the memory layout of SocketAddrV{4,6} #119
Description
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
Activity
[-]The code invalidly assumes the memory layout of SocketAddrV{4,6}[/-][+]The code assumes the memory layout of SocketAddrV{4,6}[/+]Thomasdezeeuw commentedon Nov 6, 2020
@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.
faern commentedon Nov 6, 2020
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 commentedon Nov 6, 2020
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 commentedon Nov 8, 2020
Fixed in #120
faern commentedon 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 ofsocket2 0.3.xwith this fix?de-vri-es commentedon Nov 11, 2020
I think #117 needs to be merged before releasing, or
Socket::new()would change to not setting theCLOEXECflag. But that has some requests for changes and it needs to be rebased on #124 first, which itself isn't merged yet.Thomasdezeeuw commentedon Nov 11, 2020
@de-vri-es #117 and #124 are for the master branch which will become v0.4.x. @faern I'll prepare a release.
Thomasdezeeuw commentedon Nov 11, 2020
@faern version v0.3.16 is released.
Bump net2 dependency
Auto merge of #78802 - faern:simplify-socketaddr, r=joshtriplett
Auto merge of #78802 - faern:simplify-socketaddr, r=joshtriplett