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.

Sign up for GitHub

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

Jump to bottom

Add ThreadSanitizer #4046

Merged
merged 5 commits into from
Jun 13, 2023
Merged

Add ThreadSanitizer #4046

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b79d53d
Add ThreadSanitizer
lrstewart Jun 8, 2023
1ffc31d
Address PR comments
lrstewart Jun 12, 2023
4482da6
Merge branch 'main' into tsan
jmayclin Jun 12, 2023
dac015c
Merge branch 'main' into tsan
lrstewart Jun 12, 2023
b42e69c
Merge branch 'main' into tsan
lrstewart Jun 12, 2023
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
29 changes: 17 additions & 12 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ option(EXPERIMENTAL_TREAT_WARNINGS_AS_ERRORS "Additional compiler warnings are t
indicate danger points where you should verify with the S2N-TLS developers that the security of
the library is not compromised. These warnings are currently failing for some builds; once the problems are fixed,
they will be moved to UNSAFE_TREAT_WARNINGS_AS_ERRORS." OFF)
option(TSAN "Enable ThreadSanitizer to test thread safety" OFF)

# Turn BUILD_TESTING=ON by default
include(CTest)
Expand Down Expand Up @@ -220,6 +221,11 @@ if(S2N_UNSAFE_FUZZING_MODE)
target_compile_options(${PROJECT_NAME} PRIVATE -fsanitize-coverage=trace-pc-guard -fsanitize=address,undefined,leak -fuse-ld=gold -DS2N_ADDRESS_SANITIZER=1)
endif()

if(TSAN)
target_compile_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
target_link_options(${PROJECT_NAME} PUBLIC -fsanitize=thread)
endif()

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

if (NOT $ENV{S2N_LIBCRYPTO} MATCHES "awslc")
Expand Down Expand Up @@ -496,6 +502,16 @@ if (BUILD_TESTING)
file (GLOB TEST_LD_PRELOAD "tests/LD_PRELOAD/*.c")
add_library(allocator_overrides SHARED ${TEST_LD_PRELOAD})

set(UNIT_TEST_ENVS S2N_DONT_MLOCK=1)
if(TSAN)
set(TSAN_SUPPRESSIONS_FILE ${CMAKE_SOURCE_DIR}/tests/.tsan_suppressions)
lrstewart marked this conversation as resolved.
Show resolved Hide resolved
if(NOT EXISTS ${TSAN_SUPPRESSIONS_FILE})
message(FATAL_ERROR "TSAN suppression file ${TSAN_SUPPRESSIONS_FILE} missing")
endif()
set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} S2N_ADDRESS_SANITIZER=1)
set(UNIT_TEST_ENVS ${UNIT_TEST_ENVS} TSAN_OPTIONS=suppressions=${TSAN_SUPPRESSIONS_FILE})
endif()

file(GLOB UNITTESTS_SRC "tests/unit/*.c")
foreach(test_case ${UNITTESTS_SRC})
string(REGEX REPLACE ".+\\/(.+)\\.c" "\\1" test_case_name ${test_case})
Expand All @@ -519,18 +535,7 @@ if (BUILD_TESTING)
endif()
add_test(NAME ${test_case_name} COMMAND $<TARGET_FILE:${test_case_name}> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/tests/unit)
set_property(TEST ${test_case_name} PROPERTY LABELS "unit")

set_property(
TEST
${test_case_name}
PROPERTY
ENVIRONMENT LD_PRELOAD=$<TARGET_FILE:allocator_overrides>)

set_property(
TEST
${test_case_name}
PROPERTY
ENVIRONMENT S2N_DONT_MLOCK=1)
set_property(TEST ${test_case_name} PROPERTY ENVIRONMENT ${UNIT_TEST_ENVS})
Comment on lines -524 to +540
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting ENVIRONMENT like this overrides instead of appends. So the first call to set LD_PRELOAD has no affect on how the tests are ultimately run, but the call to S2N_DONT_MLOCK does. CMake 3.22 added ENVIRONMENT_MODIFICATION to support modifying instead of completely overriding, but our CI uses some very old versions of CMake and we shouldn't assume our customers are using the latest either.

I fixed this problem by constructing the full set of environment variables as a string so that I could set ENVIRONMENT once.


endforeach(test_case)

Expand Down
25 changes: 25 additions & 0 deletions codebuild/spec/buildspec_tsan.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
---
# 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

phases:
build:
on-failure: ABORT
commands:
- cmake . -Bbuild -DTSAN=on
- cmake --build ./build -j $(nproc)
post_build:
on-failure: ABORT
commands:
- CTEST_PARALLEL_LEVEL=$(nproc) make -C build test
lrstewart marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions tests/.tsan_suppressions
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# We don't care about the thread safety of our test count
race:test_count
Loading