Add CI job with sanitizers enabled (#522)

* Fix data races in tests

.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 }}"

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)

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
         $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
         $<INSTALL_INTERFACE:include>)
 
-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")

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
         $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
         $<INSTALL_INTERFACE:include>)
 
-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")

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
         $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
         $<INSTALL_INTERFACE:include>)
 
-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")

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)

tests/HostResolverTest.cpp

@@ -35,8 +35,9 @@ static int s_TestDefaultResolution(struct aws_allocator *allocator, void *)
                 std::lock_guard<std::mutex> 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));

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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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);