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

sys/socket: fix struct sockaddr_storage alignment issue #14595

Merged

Conversation

nuttxs
Copy link
Contributor

@nuttxs nuttxs commented Nov 1, 2024

Summary

sys/socket: use attribute((packed)) to avoid changes in the size of struct sockaddr_storage due to struct alignment.

Referencing RFC 2553#section-3.10 and the POSIX standard documentation.
Defines the maximum size of the structure, which is typically 128 bytes. But the current output structure size (sizeof(struct sockaddr_storage)) is 136, which does not match this.
This discrepancy can also lead to the following potential issues:
1.An increase in the .bss segment size.
2.Functions may experience alignment issues, such as getifaddrs(&ifap) being unable to read the correct IP address.
The data passed to struct sockaddr_in is shifted, causing incorrect address parsing.

Impact

Testing

Testing with ESP32/ESP32S3 and sim.

the size of struct sockaddr_storage due to struct alignment
@github-actions github-actions bot added the Size: XS The size of the change in this PR is very small label Nov 1, 2024
@acassis acassis merged commit 3818ce8 into apache:master Nov 1, 2024
27 checks passed
@@ -327,7 +327,7 @@ struct sockaddr_storage

/* Following fields are implementation-defined */

struct
struct __attribute__((packed))
Copy link
Contributor

Choose a reason for hiding this comment

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

@nuttxs please replace with begin_packed_struct/end_packed_struct

Copy link
Contributor

Choose a reason for hiding this comment

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

@xiaoxiang781216 what is the advance of begin/end_packed_struct over attribute((packed)) ?

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Nov 1, 2024

Choose a reason for hiding this comment

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

__attribute__((packed)) is gcc specific extension, other compiler(e.g. msvc) doesn't support it at all. @lupyuen could we restore some Windows ci now?

Copy link
Member

Choose a reason for hiding this comment

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

@xiaoxiang781216 I'm already running the Mirrored Builds twice every day, why don't we use it? https://github.com/NuttX/nuttx/actions/runs/11630100298

@nuttxs To verify the Windows Build on your own NuttX Repo, please follow the steps in "To Run Windows Jobs" thanks!

Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 Nov 1, 2024

Choose a reason for hiding this comment

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

@xiaoxiang781216 I'm already running the Mirrored Builds twice every day, why don't we use it? https://github.com/NuttX/nuttx/actions/runs/11630100298

@nuttxs To verify the Windows Build on your own NuttX Repo, please follow the steps in "To Run Windows Jobs" thanks!

But it's too late, it's better to catch the error before merging. Please restore one macOS, Windows and msys2 to ci if we have the free quota. If no free quota is available, it's better to remove some Ubuntu ARM build from CI to bring back them.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@nuttxs nuttxs Nov 3, 2024

Choose a reason for hiding this comment

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

@xiaoxiang781216
My apologies for the late response. I noticed that you have already submitted a pull request (PR#14608) addressing the compatibility of __attribute__((packed)) for MSVC.

By the way, does MSVC support the __attribute__((aligned(n))) attribute or keyword? I couldn't find the corresponding definition in compiler.h, and Nuttx makes extensive use of __attribute__((aligned(n))) for alignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lupyuen
I will refer to the guide (#14407) process description. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: XS The size of the change in this PR is very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants