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.

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

Rearrange sockaddr_storage padding/alignment fields on Linux and Fuchsia #3010

Merged
merged 2 commits into from Nov 30, 2022

Conversation

stevenengler
Copy link
Contributor

@stevenengler stevenengler commented Nov 21, 2022

Part of #3004.

Previously on Linux, the sockaddr_storage structure had padding bytes between the ss_family and __ss_align fields. The __ss_align field has now been moved to the end of the structure to eliminate these padding bytes, matching recent glibc versions: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/bits/socket.h;h=4f1f810ea1d9bf00ff428e4e7c49a52c71620775;hb=c804cd1c00adde061ca51711f63068c103e94eef#l190

After the PR on Linux x86-64:

print-type-size type: `unix::linux_like::sockaddr_storage`: 128 bytes, alignment: 8 bytes
print-type-size     field `.ss_family`: 2 bytes
print-type-size     field `.__ss_pad2`: 118 bytes
print-type-size     field `.__ss_align`: 8 bytes

These moved fields are private but they are used in the sockaddr_storages PartialEq, Debug, and Hash implementations, so the exact behaviour may change slightly, but I don't think anyone should be depending on this.

(It looks like Fuchsia has the same issue, but I didn't modify its structure because I don't know much about it, or how to test it.)

Previously on Linux, the `sockaddr_storage` structure had padding bytes between
the `ss_family` and `__ss_align` fields. The `__ss_align` field has now been
moved to the end of the structure to eliminate these padding bytes, matching
recent glibc versions:

https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/bits/socket.h;h=4f1f810ea1d9bf00ff428e4e7c49a52c71620775;hb=c804cd1c00adde061ca51711f63068c103e94eef#l190
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

Please see the contribution instructions for more information.

@asomers
Copy link
Contributor

asomers commented Nov 21, 2022

I think Fuchsia's definition is here, and yes I think you should fix it in libc.
https://fuchsia.googlesource.com/fuchsia/+/refs/heads/main/zircon/third_party/ulib/musl/include/sys/socket.h#376

@stevenengler stevenengler changed the title Rearrange sockaddr_storage padding/alignment fields on Linux Rearrange sockaddr_storage padding/alignment fields on Linux and Fuchsia Nov 21, 2022
@stevenengler
Copy link
Contributor Author

I think Fuchsia's definition is here, and yes I think you should fix it in libc. https://fuchsia.googlesource.com/fuchsia/+/refs/heads/main/zircon/third_party/ulib/musl/include/sys/socket.h#376

Added a fixup commit to change Fuchsia's definition.

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

The upstream change is old enough and looking at #1412 and https://github.com/bminor/glibc/releases/tag/glibc-2.26, we could assume the effect is quite limited.

@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 30, 2022

📌 Commit 7bbbfb0 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 30, 2022

⌛ Testing commit 7bbbfb0 with merge 855a6fc...

@bors
Copy link
Contributor

bors commented Nov 30, 2022

☀️ Test successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Approved by: JohnTitor
Pushing 855a6fc to master...

@bors bors merged commit 855a6fc into rust-lang:master Nov 30, 2022
@stevenengler stevenengler deleted the sock-storage-repr branch November 30, 2022 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants