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

_FORTIFY_SOURCE guidance confuses people, should suggest undefining with more care (as order matters) #658

Open
thesamesam opened this issue Oct 14, 2024 · 5 comments

Comments

@thesamesam
Copy link
Contributor

Wireshark recently had an MR at https://gitlab.com/wireshark/wireshark/-/merge_requests/16860 that did -D_FORTIFY_SOURCE=3 -U_FORTIFY_SOURCE, possibly with the intent being to sort the options alphabetically, but the order matters.

The documentation at https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++#tldr-what-compiler-options-should-i-use should make clear the order matters and that it must be -U_FORTIFY_SOURCE and then -D_FORTIFY_SOURCE=3.

@thesamesam
Copy link
Contributor Author

thesamesam commented Oct 14, 2024

See also #272 and #659.

@thesamesam thesamesam changed the title _FORTIFY_SOURCE guidance confuses people, could do with refinement _FORTIFY_SOURCE guidance confuses people, should suggest undefining with more care Oct 14, 2024
@thesamesam thesamesam changed the title _FORTIFY_SOURCE guidance confuses people, should suggest undefining with more care _FORTIFY_SOURCE guidance confuses people, should suggest undefining with more care (as order matters) Oct 14, 2024
@cooljeanius
Copy link

Cross-referencing against GCC bug 40960: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=40960

@thomasnyman
Copy link
Contributor

The link to the POSIX specifications in GCC bug 40960 is broken as the name of the C compilation system utility is periodically changed. The current link (as of POSIX.1-2024) is https://pubs.opengroup.org/onlinepubs/9799919799/utilities/c17.html

From a practical perspective, neither GCC not Clang conform to POSIX in this regard and that seems highly unlikely to change.

@david-a-wheeler
Copy link
Contributor

Frankly, I think the POSIX specification is broken in this situation, especially since gcc and clang don't work that way (and I believe they never have).

Some text like this:

We recommend using -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 in this order. We recommend using _FORTIFY_SOURCE level 3, the highest level, to maximize fortification. We had considered recommending a higher level (so higher levels would be automatically incorporated), but this leads to warnings, and those warnings halt the compilation if warnings are considered errors. We recommend undefining _FORTIFY_SOURCE first, because setting _FORTIFY_SOURCE without undefining it first can also yield a warning. The POSIX specification for c17 requires that defining has a lower precedence than undefining, so strictly speaking this approach doesn't comply with POSIX. However, in practice neither gcc nor clang consider defining lower precedence when called with their respective names (they use left-to-right order), and there is no practical alternative for ensuring that the definition is used.

@cooljeanius
Copy link

The link to the POSIX specifications in GCC bug 40960 is broken as the name of the C compilation system utility is periodically changed. The current link (as of POSIX.1-2024) is pubs.opengroup.org/onlinepubs/9799919799/utilities/c17.html

Thanks, I crossposted this back to the GCC bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants