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

build: add s2n_prelude.h to consolidate defines #4465

Merged
merged 5 commits into from
Nov 4, 2024

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Mar 21, 2024

Description of changes:

This change adds a s2n_prelude.h that is included as part of every file on compilation. This aims to simplify/consolidate the project-wide defines, including S2N_API, _POSIX_C_SOURCE, _GNU_SOURCE, and _FORTIFY_SOURCE.

Call-outs:

I fixed a few related issues as part of this change

  • The madvise/minherit feature probe headers were not in sync with the actual usage in s2n_fork_detection.c. This caused a mismatch in behavior on OpenBSD. To make this easier to maintain, I moved all of the include/setup into s2n_fork_detection_internal.h.
  • In the s2n-tls-sys rust crate, we were keying on PROFILE == release instead of OPT_LEVEL > 0 to enable fortify source. As noted in the cargo documention, OPT_LEVEL is more correct, since workspaces can create different profiles outside of release.

Testing:

Since this is a build change, all of the existing tests show it working as expected.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@camshaft camshaft force-pushed the camshaft/prelude branch 2 times, most recently from bda4405 to 4e2083d Compare August 27, 2024 17:37
@camshaft camshaft marked this pull request as ready for review August 27, 2024 18:35
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
api/s2n.h Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Comment on lines +195 to 196
target_compile_options(${PROJECT_NAME} PUBLIC -fPIC -include "${CMAKE_CURRENT_LIST_DIR}/utils/s2n_prelude.h")

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we accidentally remove this "include"? Does it fail loudly?

And as discussed offline, maybe an assert or a test would give a nice clear failure.

utils/s2n_fork_detection_internal.h Outdated Show resolved Hide resolved
@camshaft camshaft enabled auto-merge (squash) October 28, 2024 20:53
@camshaft camshaft merged commit 53691f9 into aws:main Nov 4, 2024
37 checks passed
@camshaft camshaft deleted the camshaft/prelude branch November 5, 2024 17:14
@@ -192,11 +192,12 @@ if(S2N_BLOCK_NONPORTABLE_OPTIMIZATIONS)
target_compile_options(${PROJECT_NAME} PUBLIC -DS2N_BLOCK_NONPORTABLE_OPTIMIZATIONS=1)
endif()

target_compile_options(${PROJECT_NAME} PUBLIC -fPIC)
target_compile_options(${PROJECT_NAME} PUBLIC -fPIC -include "${CMAKE_CURRENT_LIST_DIR}/utils/s2n_prelude.h")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be like this:

Suggested change
target_compile_options(${PROJECT_NAME} PUBLIC -fPIC -include "${CMAKE_CURRENT_LIST_DIR}/utils/s2n_prelude.h")
target_compile_options(${PROJECT_NAME} PUBLIC -fPIC)
target_compile_options(${PROJECT_NAME} PRIVATE -include "${CMAKE_CURRENT_LIST_DIR}/utils/s2n_prelude.h")

By making this PUBLIC, any project that depends, directly or indirectly, on S2N gets this compile option added to all their .c/.cpp files

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

Successfully merging this pull request may close these issues.

4 participants