From 7c15c5b60b8d7578da2857e235684f478ad9a219 Mon Sep 17 00:00:00 2001 From: Igor Abdrakhimov Date: Tue, 12 Sep 2023 13:27:53 -0700 Subject: [PATCH] Add CI job with sanitizers enabled (#522) * Fix data races in tests --- .github/workflows/ci.yml | 12 ++++++++++++ CMakeLists.txt | 4 +++- bin/elasticurl_cpp/CMakeLists.txt | 4 +++- bin/mqtt5_app/CMakeLists.txt | 4 +++- bin/mqtt5_canary/CMakeLists.txt | 4 +++- tests/CMakeLists.txt | 2 ++ tests/HostResolverTest.cpp | 3 ++- tests/IotServiceTest.cpp | 24 +++++++++++++++--------- 8 files changed, 43 insertions(+), 14 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d5368f612..bcaa8f3f5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -288,3 +288,15 @@ jobs: # note: using "@main" because "@${{env.BUILDER_VERSION}}" doesn't work # https://github.com/actions/runner/issues/480 uses: awslabs/aws-crt-builder/.github/actions/check-submodules@main + + clang-sanitizers: + runs-on: ubuntu-22.04 # latest + strategy: + matrix: + sanitizers: ["thread", "address,undefined"] + steps: + # We can't use the `uses: docker://image` version yet, GitHub lacks authentication for actions -> packages + - name: Build ${{ env.PACKAGE_NAME }} + run: | + aws s3 cp s3://aws-crt-test-stuff/ci/${{ env.BUILDER_VERSION }}/linux-container-ci.sh ./linux-container-ci.sh && chmod a+x ./linux-container-ci.sh + ./linux-container-ci.sh ${{ env.BUILDER_VERSION }} aws-crt-${{ env.LINUX_BASE_IMAGE }} build -p ${{ env.PACKAGE_NAME }} --compiler=clang-12 --cmake-extra=-DENABLE_SANITIZERS=ON --cmake-extra=-DSANITIZERS="${{ matrix.sanitizers }}" diff --git a/CMakeLists.txt b/CMakeLists.txt index 4b7f419c5..e1c9b4921 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -314,7 +314,9 @@ aws_use_package(aws-checksums) aws_use_package(aws-c-event-stream) aws_use_package(aws-c-s3) -target_link_libraries(${PROJECT_NAME} ${DEP_AWS_LIBS}) +aws_add_sanitizers(${PROJECT_NAME}) + +target_link_libraries(${PROJECT_NAME} PUBLIC ${DEP_AWS_LIBS}) install(FILES ${AWS_CRT_HEADERS} DESTINATION "include/aws/crt" COMPONENT Development) install(FILES ${AWS_CRT_AUTH_HEADERS} DESTINATION "include/aws/crt/auth" COMPONENT Development) diff --git a/bin/elasticurl_cpp/CMakeLists.txt b/bin/elasticurl_cpp/CMakeLists.txt index 46eb4c643..36185c15c 100644 --- a/bin/elasticurl_cpp/CMakeLists.txt +++ b/bin/elasticurl_cpp/CMakeLists.txt @@ -9,6 +9,8 @@ file(GLOB ELASTICURL_CPP_SRC set(ELASTICURL_CPP_PROJECT_NAME elasticurl_cpp) add_executable(${ELASTICURL_CPP_PROJECT_NAME} ${ELASTICURL_CPP_SRC}) +aws_add_sanitizers(${ELASTICURL_CPP_PROJECT_NAME}) + set_target_properties(${ELASTICURL_CPP_PROJECT_NAME} PROPERTIES LINKER_LANGUAGE CXX) set_target_properties(${ELASTICURL_CPP_PROJECT_NAME} PROPERTIES CXX_STANDARD ${CMAKE_CXX_STANDARD}) @@ -31,7 +33,7 @@ target_include_directories(${ELASTICURL_CPP_PROJECT_NAME} PUBLIC $ $) -target_link_libraries(${ELASTICURL_CPP_PROJECT_NAME} aws-crt-cpp) +target_link_libraries(${ELASTICURL_CPP_PROJECT_NAME} PRIVATE aws-crt-cpp) if (BUILD_SHARED_LIBS AND NOT WIN32) message(INFO " elasticurl will be built with shared libs, but you may need to set LD_LIBRARY_PATH=${CMAKE_INSTALL_PREFIX}/lib to run the application") diff --git a/bin/mqtt5_app/CMakeLists.txt b/bin/mqtt5_app/CMakeLists.txt index db4e67bf1..e4637d6d1 100644 --- a/bin/mqtt5_app/CMakeLists.txt +++ b/bin/mqtt5_app/CMakeLists.txt @@ -9,6 +9,8 @@ file(GLOB MQTT5_APP_SRC set(MQTT5_APP_PROJECT_NAME mqtt5_app) add_executable(${MQTT5_APP_PROJECT_NAME} ${MQTT5_APP_SRC}) +aws_add_sanitizers(${MQTT5_APP_PROJECT_NAME}) + set_target_properties(${MQTT5_APP_PROJECT_NAME} PROPERTIES LINKER_LANGUAGE CXX) set_target_properties(${MQTT5_APP_PROJECT_NAME} PROPERTIES CXX_STANDARD ${CMAKE_CXX_STANDARD}) @@ -31,7 +33,7 @@ target_include_directories(${MQTT5_APP_PROJECT_NAME} PUBLIC $ $) -target_link_libraries(${MQTT5_APP_PROJECT_NAME} aws-crt-cpp) +target_link_libraries(${MQTT5_APP_PROJECT_NAME} PRIVATE aws-crt-cpp) if (BUILD_SHARED_LIBS AND NOT WIN32) message(INFO " mqtt5 app will be built with shared libs, but you may need to set LD_LIBRARY_PATH=${CMAKE_INSTALL_PREFIX}/lib to run the application") diff --git a/bin/mqtt5_canary/CMakeLists.txt b/bin/mqtt5_canary/CMakeLists.txt index 1a5aedb43..87c594cc5 100644 --- a/bin/mqtt5_canary/CMakeLists.txt +++ b/bin/mqtt5_canary/CMakeLists.txt @@ -9,6 +9,8 @@ file(GLOB MQTT5_CANARY_SRC set(MQTT5_CANARY_PROJECT_NAME mqtt5_canary) add_executable(${MQTT5_CANARY_PROJECT_NAME} ${MQTT5_CANARY_SRC}) +aws_add_sanitizers(${MQTT5_CANARY_PROJECT_NAME}) + set_target_properties(${MQTT5_CANARY_PROJECT_NAME} PROPERTIES LINKER_LANGUAGE CXX) set_target_properties(${MQTT5_CANARY_PROJECT_NAME} PROPERTIES CXX_STANDARD ${CMAKE_CXX_STANDARD}) @@ -31,7 +33,7 @@ target_include_directories(${MQTT5_CANARY_PROJECT_NAME} PUBLIC $ $) -target_link_libraries(${MQTT5_CANARY_PROJECT_NAME} aws-crt-cpp) +target_link_libraries(${MQTT5_CANARY_PROJECT_NAME} PRIVATE aws-crt-cpp) if (BUILD_SHARED_LIBS AND NOT WIN32) message(INFO " mqtt5 canary will be built with shared libs, but you may need to set LD_LIBRARY_PATH=${CMAKE_INSTALL_PREFIX}/lib to run the application") diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2727629e0..19dc05433 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -247,6 +247,8 @@ endif() generate_cpp_test_driver(${TEST_BINARY_NAME}) +aws_add_sanitizers(${TEST_BINARY_NAME}) + # set extra warning flags if(AWS_WARNINGS_ARE_ERRORS) if(MSVC) diff --git a/tests/HostResolverTest.cpp b/tests/HostResolverTest.cpp index ad6cd5362..3697c9c63 100644 --- a/tests/HostResolverTest.cpp +++ b/tests/HostResolverTest.cpp @@ -35,8 +35,9 @@ static int s_TestDefaultResolution(struct aws_allocator *allocator, void *) std::lock_guard lock(semaphoreLock); addressCount = addresses.size(); error = errorCode; + // This notify_one call has to be under mutex, to prevent a possible use-after-free case. + semaphore.notify_one(); } - semaphore.notify_one(); }; ASSERT_TRUE(defaultHostResolver.ResolveHost("localhost", onHostResolved)); diff --git a/tests/IotServiceTest.cpp b/tests/IotServiceTest.cpp index f96e011ef..797da1a7b 100644 --- a/tests/IotServiceTest.cpp +++ b/tests/IotServiceTest.cpp @@ -208,12 +208,13 @@ static int s_TestIotPublishSubscribe(Aws::Crt::Allocator *allocator, void *ctx) }; auto onConnectionClosed = [&](MqttConnection &, OnConnectionClosedData *data) { (void)data; + printf("CLOSED\n"); { std::lock_guard lock(mutex); closed = true; + // This notify_one call has to be under mutex, to prevent a possible use-after-free case. + cv.notify_one(); } - printf("CLOSED"); - cv.notify_one(); }; mqttConnection->OnConnectionCompleted = onConnectionCompleted; @@ -332,12 +333,13 @@ static int s_TestIotConnectionSuccessTest(Aws::Crt::Allocator *allocator, void * auto onConnectionClosed = [&](MqttConnection &, OnConnectionClosedData *data) { (void)data; + printf("CLOSED"); { std::lock_guard lock(mutex); closed = true; + // This notify_one call has to be under mutex, to prevent a possible use-after-free case. + cv.notify_one(); } - printf("CLOSED"); - cv.notify_one(); }; mqttConnection->OnConnectionSuccess = onConnectionSuccess; @@ -416,12 +418,13 @@ static int s_TestIotConnectionFailureTest(Aws::Crt::Allocator *allocator, void * std::condition_variable cv; bool connection_failure = false; auto onConnectionFailure = [&](MqttConnection &, OnConnectionFailureData *data) { + printf("CONNECTION FAILURE: error=%i\n", data->error); { std::lock_guard lock(mutex); connection_failure = true; + // This notify_one call has to be under mutex, to prevent a possible use-after-free case. + cv.notify_one(); } - printf("CONNECTION FAILURE: error=%i\n", data->error); - cv.notify_one(); }; mqttConnection->OnConnectionFailure = onConnectionFailure; Aws::Crt::UUID Uuid; @@ -544,8 +547,9 @@ static int s_TestIotWillTest(Aws::Crt::Allocator *allocator, void *ctx) { std::lock_guard lock(subscriberMutex); subscriberConnected = false; + // This notify_one call has to be under mutex, to prevent a possible use-after-free case. + subscriberCv.notify_one(); } - subscriberCv.notify_one(); }; auto subscriberOnSubAck = [&](MqttConnection &, uint16_t packetId, const Aws::Crt::String &topic, QOS qos, int) { @@ -710,8 +714,9 @@ static int s_TestIotStatisticsPublishWaitStatisticsDisconnect(Aws::Crt::Allocato { std::lock_guard lock(mutex); connected = false; + // This notify_one call has to be under mutex, to prevent a possible use-after-free case. + cv.notify_one(); } - cv.notify_one(); }; auto onPubAck = [&](MqttConnection &, uint16_t packetId, int) { printf("PUBLISHED id=%d\n", packetId); @@ -839,8 +844,9 @@ static int s_TestIotStatisticsPublishStatisticsWaitDisconnect(Aws::Crt::Allocato { std::lock_guard lock(mutex); connected = false; + // This notify_one call has to be under mutex, to prevent a possible use-after-free case. + cv.notify_one(); } - cv.notify_one(); }; auto onPubAck = [&](MqttConnection &, uint16_t packetId, int) { printf("PUBLISHED id=%d\n", packetId);