ci: cmake asan build #4048
ci: cmake asan build #4048
Conversation
jmayclin
force-pushed
the
cmake-asan-build
branch
4 times, most recently
from
3a8fba0 to
e581e12
Compare
jmayclin
force-pushed
the
cmake-asan-build
branch
from
6f6393f to
c3ac669
Compare
| - | | ||
| cmake . -Bbuild \ | ||
| -DCMAKE_C_COMPILER=/usr/bin/clang \ | ||
| -DCMAKE_PREFIX_PATH=/usr/local/$S2N_LIBCRYPTO \ |
There was a problem hiding this comment.
Thanks for dropping the ./test-deps/$S2N_LIBCRYPTO pattern.
There was a problem hiding this comment.
Ya, the fact that the codebuild src folder is always different, and therefore test-deps is always a different path felt like a bit of forbidden magic 😄 . I also hope that this pattern will reduce the delta between "how we run things in CI" and "how we build things locally"
codebuild/spec/buildspec_omnibus.yml
Outdated
| @@ -120,43 +120,6 @@ batch: | |||
| S2N_LIBCRYPTO: 'awslc-fips' | |||
| BUILD_S2N: 'true' | |||
|
|
|||
| - identifier: s2nAsanOpenSSL111Coverage | |||
There was a problem hiding this comment.
Why not just update these with the new job?
There was a problem hiding this comment.
Hmm. I guess that depends on how exactly we are using the omnibus job. I usually think of it as "approximate list of the CI jobs that we run", and since the Asan jobs are now documented in the Asan buildspec, I figured it made sense to remove it.
But perhaps your point is that we do run the omnibus job, in which case we should keep the asan jobs in there? I'd be fine with that in the short term to unblock the PR, but long term it feels a bit odd to duplicate all of our CI build specs. Could whatever is using the omnibus just switch the using the normal CI jobs that we have?
There was a problem hiding this comment.
We've had some drift since the original setup, so it's worth a review. Internal releases still use the Omnibus job, but it no longer contains all the jobs (it was originally the source of truth that the other spec files came from).
* move ASAN ignore to s2n_safety * remove newline from CMakeLists.txt
CMakeLists.txt
Outdated
| set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} S2N_ADDRESS_SANITIZER=1) | ||
| endif() |
There was a problem hiding this comment.
Isn't there now a line in the next if that's a duplicate? Line 511/521?
Co-authored-by: Lindsay Stewart <[email protected]>
|
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. |
utils/s2n_random.c
Outdated
| @@ -568,8 +568,11 @@ S2N_RESULT s2n_set_private_drbg_for_test(struct s2n_drbg drbg) | |||
| /* | |||
| * volatile is important to prevent the compiler from | |||
| * re-ordering or optimizing the use of RDRAND. | |||
| * | |||
| * This is marked with ASAN_IGNORE because address sanitizer is unable to deal | |||
| * with the inline assembly and emits false positives. | |||
There was a problem hiding this comment.
It's not a false positive, it's an actual bug
#4310
There was a problem hiding this comment.
Thanks for catching this! I'll go ahead and update the PR.
Once I have the AddressSanatizer job included in our CI then I will remove the unnecessary batches.
Resolved issues:
Description of changes:
Currently the address sanitizer job can't be run through CMake. This PR
I added the asan buildspec as a separate codebuild job because it feels simpler and a little bit more self-documenting. It also reduces maintenance because now we can use the buildspec as the actual specification for the codebuild job rather than copying and pasting everytime we update it.
Call-outs:
After this PR is merged
Testing:
Here is a codebuild job confirming that we fail when there is a memory leak present: link This was the result of running the codebuild job on mainline s2n-tls with the mem fix for ossl3 reverted.
Here is the successful asan codebuild job for the PR: link
All other existing CI should pass.
Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.