FreeBSD and OpenBSD fork detection #2089
Merged
+195
−72
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## randomness_generation #2089 +/- ##
======================================================
Coverage 78.71% 78.71%
======================================================
Files 606 608 +2
Lines 102759 102791 +32
Branches 14578 14584 +6
======================================================
+ Hits 80887 80917 +30
- Misses 21161 21164 +3
+ Partials 711 710 -1 ☔ View full report in Codecov by Sentry. |
justsmth
reviewed
Jan 2, 2025
### Description of changes: BoringSSL now requires C++17 after google/boringssl@9ff8491. When we build speed.cc with BoringSSL's headers we need to specify our our build to use C++17. This is blocking all of our CI with failures like [this](https://us-west-2.codebuild.aws.amazon.com/project/eyJlbmNyeXB0ZWREYXRhIjoiWk5IUWJGRGxBcTJ2Mkp4WGF3dnBwYjc5V0ZZYSt5SVVGbkwvODkydTNTaVQ2V2FMN3hwa0tjSWNFemw2QWtCWW5welFWV3lpRFpKVitwejgvelhpRWh3NDNqcWhKalpPYW9hL2tLMDlJSDFPT1NkNyIsIml2UGFyYW1ldGVyU3BlYyI6Ilp4VXRNQXFGM1BZYVlaRkIiLCJtYXRlcmlhbFNldFNlcmlhbCI6MX0%3D/build/73b9a032-23e0-493c-b4f2-08fce7df7798). ### Call-outs: This does not change AWS-LC's normal C++11 requirement. ### Testing: The CI will build this. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
justsmth
reviewed
Jan 2, 2025
Comment on lines
+79
to
+83
| #if defined(MADV_WIPEONFORK) | ||
| OPENSSL_STATIC_ASSERT(MADV_WIPEONFORK == 18, MADV_WIPEONFORK_is_not_18) | ||
| #else | ||
| #define MADV_WIPEONFORK 18 | ||
| #endif |
There was a problem hiding this comment.
- If
MADV_WIPEONFORKwere not already defined in the headers provided by the system, why do we think it's safe to define the value here? - Since it's a syscall parameter, the value of
MADV_WIPEONFORKshould be stable for a given target, but how do we know that the value is 18 across all Linux targets?- Possibly related:
MADV_WIPEONFORKis not supported by linux kernels prior to v4.14. There are older kernels (like v4.4) that are still actively supported (till January 2027).
- Possibly related:
- Should the macro guard for this section instead be something like:
#if defined(OPENSSL_LINUX) && defined(MADV_WIPEONFORK)
There was a problem hiding this comment.
It's the existing behaviour and it has worked flawlessly.
It's just to increase support for the feature that we attempt to use a hard-coded value in case the define doesn't exist.
madvise(addr, (size_t)page_size, MADV_WIPEONFORK)
will fail if the feature is not supported.
Comment on lines
+116
to
+119
| // Sometimes (for example, on FreeBSD) MAP_INHERIT_ZERO is called INHERIT_ZERO | ||
| #if !defined(MAP_INHERIT_ZERO) && defined(INHERIT_ZERO) | ||
| #define MAP_INHERIT_ZERO INHERIT_ZERO | ||
| #endif |
There was a problem hiding this comment.
NP: Would it be safer to define our own macro with a value specific the target OS:
#if defined(OPENSSL_FREEBSD)
#define AWS_LC_MAP_INHERIT_ZERO INHERIT_ZERO
#else // defined(OPENSSL_OPENBSD)
#define AWS_LC_MAP_INHERIT_ZERO MAP_INHERIT_ZERO
#endif
There was a problem hiding this comment.
The current form is a bit shorter. It has worked flawlessly in s2n-tls where I first used this pattern.
justsmth
approved these changes
Jan 3, 2025
smittals2
approved these changes
Jan 3, 2025
torben-hansen
merged commit 2637eda
into
aws:randomness_generation
112 of 116 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
Two things are happening in this PR:
fork_detect.cto make is more readable and amendableThe new method uses
minherit, tagging a memory page with a special value. The semantics are equivalent to the existing case withmadvise.Testing:
We still need some more testing to increase test coverage for fork detection. This is an upcoming task.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.