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

Reword incorrect documentation about SocketAddr having varying layout #136230

Merged
merged 1 commit into from
Mar 14, 2025
Merged

Reword incorrect documentation about SocketAddr having varying layout #136230

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions library/core/src/net/socket_addr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,15 @@ use crate::net::{IpAddr, Ipv4Addr, Ipv6Addr};
/// as possibly some version-dependent additional information. See [`SocketAddrV4`]'s and
/// [`SocketAddrV6`]'s respective documentation for more details.
///
/// The size of a `SocketAddr` instance may vary depending on the target operating
/// system.
///
/// [IP address]: IpAddr
///
/// # Portability
///
/// `SocketAddr` is intended to be a portable representation of socket addresses and is likely not
/// the same as the internal socket address type used by the target operating system's API. Like all
/// `repr(Rust)` structs, however, its exact layout remains undefined and should not be relied upon
/// between builds.
///
/// # Examples
///
/// ```
Expand Down Expand Up @@ -42,13 +46,16 @@ pub enum SocketAddr {
///
/// See [`SocketAddr`] for a type encompassing both IPv4 and IPv6 socket addresses.
///
/// The size of a `SocketAddrV4` struct may vary depending on the target operating
/// system. Do not assume that this type has the same memory layout as the underlying
/// system representation.
Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with removing the size comment, but isn't the other layout note still relevant? (e.g. don't try to treat this like a C struct sockaddr!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, it's technically relevant, but it feels unprecedented in the standard library, since no other type adds a comment like this. I actually took a bit to look through the existing types, though, and I guess these are the types where this would technically be most applicable. (You wouldn't expect a File to be equivalent to a file handler, for example.)

Like, most people are not going to try transmuting random standard library structs to libc structs unless it's explicitly marked as allowed, and since transmuting is unsafe, they're also kind of admitting fault if they do this.

But also, is there actually a reason we couldn't make this equivalent to a C sockaddr anyway? Since no padding is required, and we don't really gain anything from letting the fields be randomly arranged.

Copy link
Member

Choose a reason for hiding this comment

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

We used to wrap sockaddr directly, and prominent crates did make unsafe assumptions about that. We changed to Rust-native types in #78802, which also documented many of those problems along the way.

So yes, in general users shouldn't make any assumptions about the underlying type, but they did in this case. Therefore, I think it's worth keeping some warning in the docs, even though it would be obviously broken if you tried it anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, for some reason, I had naïvely assumed that sockaddr just used the IETF definition on all platforms, which is clearly not correct. And besides that, it's not the same definition as the one we use.

I'll update to remark on this.

///
/// [IETF RFC 793]: https://tools.ietf.org/html/rfc793
/// [`IPv4` address]: Ipv4Addr
///
/// # Portability
///
/// `SocketAddrV4` is intended to be a portable representation of socket addresses and is likely not
/// the same as the internal socket address type used by the target operating system's API. Like all
/// `repr(Rust)` structs, however, its exact layout remains undefined and should not be relied upon
/// between builds.
///
/// # Textual representation
///
/// `SocketAddrV4` provides a [`FromStr`](crate::str::FromStr) implementation.
Expand Down Expand Up @@ -84,13 +91,16 @@ pub struct SocketAddrV4 {
///
/// See [`SocketAddr`] for a type encompassing both IPv4 and IPv6 socket addresses.
///
/// The size of a `SocketAddrV6` struct may vary depending on the target operating
/// system. Do not assume that this type has the same memory layout as the underlying
/// system representation.
///
/// [IETF RFC 2553, Section 3.3]: https://tools.ietf.org/html/rfc2553#section-3.3
/// [`IPv6` address]: Ipv6Addr
///
/// # Portability
///
/// `SocketAddrV6` is intended to be a portable representation of socket addresses and is likely not
/// the same as the internal socket address type used by the target operating system's API. Like all
/// `repr(Rust)` structs, however, its exact layout remains undefined and should not be relied upon
/// between builds.
///
/// # Textual representation
///
/// `SocketAddrV6` provides a [`FromStr`](crate::str::FromStr) implementation,
Expand Down
Loading