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: cmake asan build #4048

Merged
merged 16 commits into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ option(S2N_INTEG_TESTS "Enable the integrationv2 tests" OFF)
option(S2N_FAST_INTEG_TESTS "Enable the integrationv2 with more parallelism, only has effect if S2N_INTEG_TESTS=ON" OFF)
option(S2N_INSTALL_S2NC_S2ND "Install the binaries s2nc and s2nd" OFF)
option(TSAN "Enable ThreadSanitizer to test thread safety" OFF)
option(ASAN "Enable AddressSanitizer to test memory safety" OFF)

# Turn BUILD_TESTING=ON by default
include(CTest)
Expand Down Expand Up @@ -218,6 +219,12 @@ if(TSAN)
target_link_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
endif()

if (ASAN)
# no-oimt-frame-pointer and no-optimize-sibling-calls provide better stack traces
target_compile_options(${PROJECT_NAME} PUBLIC -fsanitize=address,undefined -fno-omit-frame-pointer -fno-optimize-sibling-calls -DS2N_ADDRESS_SANITIZER=1)
target_link_options(${PROJECT_NAME} PUBLIC -fsanitize=address,undefined -fno-omit-frame-pointer -fno-optimize-sibling-calls -DS2N_ADDRESS_SANITIZER=1)
endif()

list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules")

if (NOT $ENV{S2N_LIBCRYPTO} MATCHES "awslc")
Expand Down Expand Up @@ -503,6 +510,9 @@ if (BUILD_TESTING)
add_library(allocator_overrides SHARED ${TEST_LD_PRELOAD})

set(UNIT_TEST_ENVS S2N_DONT_MLOCK=1)
if (TSAN OR ASAN)
set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} S2N_ADDRESS_SANITIZER=1)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there now a line in the next if that's a duplicate? Line 511/521?

if(TSAN)
set(TSAN_SUPPRESSIONS_FILE ${CMAKE_SOURCE_DIR}/tests/.tsan_suppressions)
if(NOT EXISTS ${TSAN_SUPPRESSIONS_FILE})
Expand Down
41 changes: 41 additions & 0 deletions codebuild/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Docker Image Structure
The codebuild specifications are run on a custom docker images that have the test dependencies installed. The docker image structure is described below.

### libcrypto
Various libcryptos are installed to `/usr/local/$LIBCRYPTO` directories. For example
```
# non-exhaustive list
/usr/local/openssl-1.0.2/lib/libcrypto.a
/usr/local/openssl-1.0.2/lib/libcrypto.so
/usr/local/openssl-1.0.2/lib/libcrypto.so.1.0.0
/usr/local/openssl-1.0.2/lib/pkgconfig/libcrypto.pc
/usr/local/openssl-3.0/lib64/libcrypto.a
/usr/local/openssl-3.0/lib64/libcrypto.so.3
/usr/local/openssl-3.0/lib64/libcrypto.so
/usr/local/openssl-3.0/lib64/pkgconfig/libcrypto.pc
/usr/local/boringssl/lib/libcrypto.so
/usr/local/awslc/lib/libcrypto.a
/usr/local/awslc/lib/libcrypto.so
```

Packages installed from the `apt` package manager can generally be found in `/usr/lib`. For example, our 32 bit build uses the 32 bit `i386` libcrypto, and it's artifacts are located at
```
/usr/lib/i386-linux-gnu/libcrypto.a
/usr/lib/i386-linux-gnu/libcrypto.so.3
/usr/lib/i386-linux-gnu/libcrypto.so
/usr/lib/i386-linux-gnu/pkgconfig/libcrypto.pc
```

When the docker image is available locally, the structure can be easily examined by attaching an interactive terminal to the container with the following command
```
docker run --entrypoint /bin/bash -it --privileged <image id>
```

Then the `find` command can be used to look at the various artifacts that are available.
```
sudo find / -name libcrypto* # list all libcrypto artifacts
```
or
```
sudo find / -name clang* # find all clang binaries
```
1 change: 0 additions & 1 deletion codebuild/bin/s2n_codebuild.sh
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ if [[ "$TESTS" == "ALL" || "$TESTS" == "sawHMACPlus" ]] && [[ "$OS_NAME" == "lin
if [[ "$TESTS" == "ALL" || "$TESTS" == "unit" ]]; then run_unit_tests; fi
if [[ "$TESTS" == "ALL" || "$TESTS" == "interning" ]]; then ./codebuild/bin/test_libcrypto_interning.sh; fi
if [[ "$TESTS" == "ALL" || "$TESTS" == "exec_leak" ]]; then ./codebuild/bin/test_exec_leak.sh; fi
if [[ "$TESTS" == "ALL" || "$TESTS" == "asan" ]]; then make clean; S2N_ADDRESS_SANITIZER=1 make -j $JOBS ; fi
if [[ "$TESTS" == "ALL" || "$TESTS" == "integrationv2" ]]; then run_integration_v2_tests; fi
if [[ "$TESTS" == "ALL" || "$TESTS" == "crt" ]]; then ./codebuild/bin/build_aws_crt_cpp.sh $(mktemp -d) $(mktemp -d); fi
if [[ "$TESTS" == "ALL" || "$TESTS" == "sharedandstatic" ]]; then ./codebuild/bin/test_install_shared_and_static.sh $(mktemp -d); fi
Expand Down
63 changes: 63 additions & 0 deletions codebuild/spec/buildspec_asan.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
---
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License"). You may not use
# this file except in compliance with the License. A copy of the License is
# located at
#
# http://aws.amazon.com/apache2.0/
#
# or in the "license" file accompanying this file. This file is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
# implied. See the License for the specific language governing permissions and
# limitations under the License.
version: 0.2

# This buildspec runs on an Ubuntu22 image. That configuration is a property of
# the codebuild job itself.

# Codebuild's matrix jobs have non-differentiated names so use batch-list
# instead.
batch:
build-list:
# awslc is the happy path libcrypto for s2n-tls
- identifier: awslc
env:
compute-type: BUILD_GENERAL1_LARGE
variables:
S2N_LIBCRYPTO: awslc
# s2n-tls takes different code paths for ossl3, so make sure we run asan on
# it. See pr 4033 for a historical motivating example.
- identifier: openssl_3_0
env:
compute-type: BUILD_GENERAL1_LARGE
variables:
S2N_LIBCRYPTO: openssl-3.0
# openssl 1.1.1 is a widely deployed version of openssl.
- identifier: openssl_1_1_1
env:
compute-type: BUILD_GENERAL1_LARGE
variables:
S2N_LIBCRYPTO: openssl-1.1.1
# openssl 1.0.2 is the default distributed on AL2, and AL2 is still widely
# deployed
- identifier: openssl_1_0_2
env:
compute-type: BUILD_GENERAL1_LARGE
variables:
S2N_LIBCRYPTO: openssl-1.0.2

phases:
build:
on-failure: ABORT
commands:
- |
cmake . -Bbuild \
-DCMAKE_C_COMPILER=/usr/bin/clang \
-DCMAKE_PREFIX_PATH=/usr/local/$S2N_LIBCRYPTO \
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for dropping the ./test-deps/$S2N_LIBCRYPTO pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

-DASAN=ON
- cmake --build ./build -- -j $(nproc)
post_build:
on-failure: ABORT
commands:
- CTEST_OUTPUT_ON_FAILURE=1 CTEST_PARALLEL_LEVEL=$(nproc) make -C build test
37 changes: 0 additions & 37 deletions codebuild/spec/buildspec_omnibus.yml
Original file line number Diff line number Diff line change
Expand Up @@ -120,43 +120,6 @@ batch:
S2N_LIBCRYPTO: 'awslc-fips'
BUILD_S2N: 'true'

- identifier: s2nAsanOpenSSL111Coverage
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just update these with the new job?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

buildspec: codebuild/spec/buildspec_ubuntu.yml
env:
privileged-mode: true
compute-type: BUILD_GENERAL1_LARGE
image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu18codebuild
variables:
TESTS: asan
GCC_VERSION: '6'
S2N_LIBCRYPTO: 'openssl-1.1.1'
BUILD_S2N: 'true'
S2N_COVERAGE: 'true'

- identifier: s2nAsanOpenssl3
buildspec: codebuild/spec/buildspec_ubuntu.yml
env:
privileged-mode: true
compute-type: BUILD_GENERAL1_LARGE
image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu18codebuild
variables:
TESTS: asan
GCC_VERSION: '6'
S2N_LIBCRYPTO: 'openssl-3.0'
BUILD_S2N: 'true'

- identifier: s2nAsanOpenssl102
buildspec: codebuild/spec/buildspec_ubuntu.yml
env:
privileged-mode: true
compute-type: BUILD_GENERAL1_LARGE
image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu18codebuild
variables:
TESTS: asan
GCC_VERSION: '6'
S2N_LIBCRYPTO: 'openssl-1.0.2'
BUILD_S2N: 'true'

- identifier: s2nUnitNoPQ
buildspec: codebuild/spec/buildspec_ubuntu.yml
env:
Expand Down
5 changes: 4 additions & 1 deletion utils/s2n_random.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a false positive, it's an actual bug
#4310

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this! I'll go ahead and update the PR.

*/
static int s2n_rand_rdrand_impl(void *data, uint32_t size)
ASAN_IGNORE static int s2n_rand_rdrand_impl(void *data, uint32_t size)
{
#if defined(__x86_64__) || defined(__i386__)
struct s2n_blob out = { 0 };
Expand Down
9 changes: 9 additions & 0 deletions utils/s2n_safety.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@
#define FALL_THROUGH ((void) 0)
#endif

#if defined(S2N_ADDRESS_SANITIZER)
/**
* Marks that a function should be ignored for address sanitizer jobs
*/
#define ASAN_IGNORE __attribute__((no_address_safety_analysis))
#else
#define ASAN_IGNORE
#endif /* defined(S2N_ADDRESS_SANITIZER) */

int s2n_in_unit_test_set(bool is_unit);
int s2n_in_integ_test_set(bool is_integ);
bool s2n_in_unit_test();
Expand Down