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

ci: add more libcryptos for fuzz batch & follow cmake idioms #4795

Merged
merged 11 commits into from
Oct 10, 2024

Conversation

jouho
Copy link
Contributor

@jouho jouho commented Sep 23, 2024

Resolved issues:

  1. The current fuzz batch jobs are built with awslc and openssl-3.0. This PR adds additional supported libcrypto versions to increase coverage.
  2. Test executables and override libraries are currently generated under the tests/fuzz and tests/fuzz/LD_PRELOAD folders. We should follow cmake build idioms (w.r.t binary location & library location)

Description of changes:

  • Added openssl-1.0.2 and openssl-1.1.1 to the s2nFuzzBatch job.

  • Modified CMakeLists.txt to remove logic that generates test binaries and library files in specific folders. Now, when compiling fuzz tests:

    • Overriding libraries are stored in s2n-tls/build/lib
    • Test executables are stored in s2n-tls/build/bin.
  • Updated runFuzzTest.sh to adjust the file paths accordingly.

Call-outs:

I attempted to add awslc-fips to the batch job, but it failed during the compile. Link to Failed CodeBuild job
I also tried awslc-fips-2022, which compiled successfully but failed to find the libcrypto. Link to CodeBuild job

I have created new issue to investigate this: #4800

After this PR is merged, I will also update s2nOmnibus to include these libcrypto versions to ensure that the patches won’t break anything.

Testing:

Tested by overriding s2nFuzzBatch job against this PR, and they are finding correct libcrypto versions:
Link to CodeBuild job

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

@github-actions github-actions bot added the s2n-core team label Sep 23, 2024
@jouho jouho marked this pull request as ready for review September 25, 2024 00:39
@jouho jouho requested a review from dougch as a code owner September 25, 2024 00:39
@jouho jouho requested review from lrstewart and jmayclin September 25, 2024 00:39
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
- remove unused library paths
- remove prefix config
- remove -fPIC option
@jouho jouho requested a review from jmayclin September 25, 2024 20:38
tests/fuzz/runFuzzTest.sh Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@jouho jouho requested review from jmayclin and dougch September 26, 2024 20:57
Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

I'd like to run runFuzzTest.sh locally, using the syntax it'll be called with...

usage
fi

TEST_NAME=$1
FUZZ_TIMEOUT_SEC=$2
CORPUS_UPLOAD_LOC=$3
ARTIFACT_UPLOAD_LOC=$4
BUILD_DIR_PATH=$3
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these arguments going to be updated in a future PR? or rather, can I review how this script is being called somewhere ?

Copy link
Contributor Author

@jouho jouho Sep 27, 2024

Choose a reason for hiding this comment

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

This script is getting called in CMakeList.txt here:
https://github.com/jouho/s2n-tls/blob/cd9eaa5fd589e7e8e785203c7dc4792e25f56436/CMakeLists.txt#L673

SCRIPT_PATH is the path to runFuzzTest.sh. Does this answer your question?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only in CMakeList.txt? No existing codebuild job is calling it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only ran in CMake after we migrated away from make. codebuild jobs will indirectly call it through cmake.

@jouho
Copy link
Contributor Author

jouho commented Sep 27, 2024

I'd like to run runFuzzTest.sh locally, using the syntax it'll be called with...

You could run the same command we have in buildspec_fuzz.yml, which is

cmake . -Bbuild \
-DCMAKE_PREFIX_PATH=/usr/local/$S2N_LIBCRYPTO \
-DS2N_FUZZ_TEST=on

Then

cmake --build ./build -- -j $(nproc)

And then

cmake --build build/ --target test -- ARGS="-L fuzz --output-on-failure"

Copy link
Contributor

Choose a reason for hiding this comment

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

Modified CMakeLists.txt to remove logic that generates test binaries and library files in specific folders

Do you know why it was using specific folders before? Was is possibly due to concerns about name collisions / to avoid overwriting a non-fuzzing library or binary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a result of inheriting make build configuration. CMake by default stores executables and libraries under build/ folder. In order to mimic the behavior of make, I had to specify specific folder to store these files. Now that I am making things a bit more "cmaky", I am removing that logic.

I couldn't find any mention of the original intention for where to store these files, but that is a good call-out. Should we store all the fuzz-related binaries under a specific folder, something like build/fuzz/bin for executables and build/fuzz/lib for libraries?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to keep the cmake standard paths, but we should make sure LD_PRELOAD is getting set to the actual libraries and not just the build/lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should make sure LD_PRELOAD is getting set to the actual libraries and not just the build/lib

What do you mean by this? Currently the .so files are being generated in build/lib, and we set the LD_PRELOAD path to be "${BUILD_DIR_PATH}/lib/lib${TEST_NAME}_overrides.so" and "${BUILD_DIR_PATH}/lib/libglobal_overrides.so". Do you suggest storing the .so files elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, that's great.

To Lindsay's point, moving everything to a generic build/lib would have been a problem if we were only specifying the LD_PRELOAD by folder (LD_PRELOAD=build/lib/), because then shenanigans would ensue, and all of the test specific overrides would be pulled in for all tests.

@jouho jouho requested review from dougch and lrstewart October 7, 2024 23:32
Copy link
Contributor

@dougch dougch left a comment

Choose a reason for hiding this comment

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

Running locally and reviewing your ad-hoc job, it looks like the tests were being run as singletons/serially. Somewhere you should have a CTEST_PARALLEL_LEVEL=$(nproc) or a -j flag to tell ctest to run in parallel.

@jouho
Copy link
Contributor Author

jouho commented Oct 9, 2024

Running multiple fuzz tests concurrently is a bit complicated, because we are already running individual tests multi-threaded. Even if we run multiple tests concurrently, the total duration would remain approximately the same, as long as we ensure each individual test's runtime remains consistent to maintain coverage.

If our goal is to reduce the total test time, there is a tracking issue #4748 to investigate how much code coverage fuzzing achieves over time. Total test time may be reduced if we reach peak coverage early into the fuzz test.

@jouho jouho changed the title ci: add more libcryptos for fuzz batch ci: add more libcryptos for fuzz batch & follow cmake idioms Oct 10, 2024
@jouho jouho enabled auto-merge (squash) October 10, 2024 18:10
@jouho jouho merged commit e1e27ec into aws:main Oct 10, 2024
37 checks passed
@jouho jouho deleted the additional-libcryptos-to-fuzzing branch October 10, 2024 20:43
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