fix tsan related issues (#427)

fix tsan related issues
awslabs · Feb 5, 2024 · 76a72e9 · 76a72e9
1 parent 13523a2
commit 76a72e9
Show file tree

Hide file tree

Showing 13 changed files with 106 additions and 86 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -295,31 +295,34 @@ jobs:
   #         ulimit -c unlimited -S
   #         timeout --signal=SIGABRT 150m ./tst/producer_test --gtest_break_on_failure
 
-  # thread-sanitizer:
-  #   runs-on: ubuntu-20.04
-  #   permissions:
-  #     id-token: write
-  #     contents: read
-  #   env:
-  #     CC: clang
-  #     CXX: clang++
-  #     AWS_KVS_LOG_LEVEL: 2
-  #   steps:
-  #     - name: Clone repository
-  #       uses: actions/checkout@v3
-  #     - name: Configure AWS Credentials
-  #       uses: aws-actions/configure-aws-credentials@v1-node16
-  #       with:
-  #         role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
-  #         role-session-name: ${{ secrets.AWS_ROLE_SESSION_NAME }}
-  #         aws-region: ${{ secrets.AWS_REGION }}
-  #     - name: Build repository
-  #       run: |
-  #         mkdir build && cd build
-  #         cmake .. -DBUILD_TEST=TRUE -DTHREAD_SANITIZER=TRUE -DBUILD_COMMON_LWS=TRUE
-  #         make
-  #         ulimit -c unlimited -S
-  #         timeout --signal=SIGABRT 150m ./tst/producer_test --gtest_break_on_failure
+  #thread-sanitizer:
+  #  runs-on: ubuntu-20.04
+  #  permissions:
+  #    id-token: write
+  #    contents: read
+  #  env:
+  #    CC: clang
+  #    CXX: clang++
+  #    AWS_KVS_LOG_LEVEL: 2
+  #  steps:
+  #    - name: Clone repository
+  #      uses: actions/checkout@v3
+  #    - name: Configure AWS Credentials
+  #      uses: aws-actions/configure-aws-credentials@v1-node16
+  #      with:
+  #        role-to-assume: ${{ secrets.AWS_ROLE_TO_ASSUME }}
+  #        role-session-name: ${{ secrets.AWS_ROLE_SESSION_NAME }}
+  #        aws-region: ${{ secrets.AWS_REGION }}
+  #    - name: Build repository
+  #      run: |
+  #        mkdir build && cd build
+  #        cmake .. -DBUILD_TEST=TRUE -DTHREAD_SANITIZER=TRUE -DBUILD_COMMON_LWS=TRUE
+  #        make
+  #    - name: Run tests
+  #      run: |
+  #        cd build
+  #        ulimit -c unlimited -S
+  #        timeout --signal=SIGABRT 150m ./tst/producer_test --gtest_break_on_failure
 
   ubuntu-gcc:
     runs-on: ubuntu-20.04

diff --git a/CMake/Dependencies/libcurl-CMakeLists.txt b/CMake/Dependencies/libcurl-CMakeLists.txt
@@ -1,6 +1,6 @@
 cmake_minimum_required(VERSION 3.6.3)
 
-project(libcurl-download NONE)
+project(libcurl-download LANGUAGES C)
 
 find_program(MAKE_EXE NAMES make)
 

diff --git a/CMake/Dependencies/libgtest-CMakeLists.txt b/CMake/Dependencies/libgtest-CMakeLists.txt
@@ -3,7 +3,6 @@ cmake_minimum_required(VERSION 3.6.3)
 project(libgtest-download NONE)
 
 include(ExternalProject)
-
 ExternalProject_Add(libgtest-download
     GIT_REPOSITORY    https://github.com/google/googletest.git
     GIT_TAG           release-1.12.1

diff --git a/CMake/Dependencies/libkvspic-CMakeLists.txt b/CMake/Dependencies/libkvspic-CMakeLists.txt
@@ -1,6 +1,6 @@
 cmake_minimum_required(VERSION 3.6.3)
 
-project(libkvspic-download NONE)
+project(libkvspic-download LANGUAGES C)
 
 include(ExternalProject)
 

diff --git a/CMake/Dependencies/libmbedtls-CMakeLists.txt b/CMake/Dependencies/libmbedtls-CMakeLists.txt
@@ -1,6 +1,6 @@
 cmake_minimum_required(VERSION 3.6.3)
 
-project(libmbedtls-download NONE)
+project(libmbedtls-download LANGUAGES C)
 
 include(ExternalProject)
 

diff --git a/CMake/Dependencies/libwebsockets-CMakeLists.txt b/CMake/Dependencies/libwebsockets-CMakeLists.txt
@@ -1,6 +1,6 @@
 cmake_minimum_required(VERSION 3.6.3)
 
-project(libwebsocket-download NONE)
+project(libwebsocket-download LANGUAGES C)
 
 include(ExternalProject)
 

diff --git a/src/source/Common/AwsV4Signer.c b/src/source/Common/AwsV4Signer.c
@@ -659,7 +659,7 @@ STATUS generateSignatureDateTime(UINT64 currentTime, PCHAR pDateTimeStr)
 
     // Convert to time_t
     timeT = (time_t) (currentTime / HUNDREDS_OF_NANOS_IN_A_SECOND);
-    retSize = STRFTIME(pDateTimeStr, SIGNATURE_DATE_TIME_STRING_LEN, DATE_TIME_STRING_FORMAT, GMTIME(&timeT));
+    retSize = STRFTIME(pDateTimeStr, SIGNATURE_DATE_TIME_STRING_LEN, DATE_TIME_STRING_FORMAT, GMTIME_THREAD_SAFE(&timeT));
     CHK(retSize > 0, STATUS_BUFFER_TOO_SMALL);
     pDateTimeStr[retSize] = '\0';
 

diff --git a/src/source/CurlApiCallbacks.c b/src/source/CurlApiCallbacks.c
@@ -1005,15 +1005,17 @@ STATUS createStreamCurl(UINT64 customData, PCHAR deviceName, PCHAR streamName, P
 
 CleanUp:
 
+    if (startLocked) {
+        // Release the lock to let the awaiting handler thread to continue
+        pCallbacksProvider->clientCallbacks.unlockMutexFn(pCallbacksProvider->clientCallbacks.customData, pCurlRequest->startLock);
+    }
+
     if (STATUS_FAILED(retStatus)) {
         if (IS_VALID_TID_VALUE(threadId)) {
             THREAD_CANCEL(threadId);
         }
 
         freeCurlRequest(&pCurlRequest);
-    } else if (startLocked) {
-        // Release the lock to let the awaiting handler thread to continue
-        pCallbacksProvider->clientCallbacks.unlockMutexFn(pCallbacksProvider->clientCallbacks.customData, pCurlRequest->startLock);
     }
 
     if (shutdownLocked) {
@@ -1237,15 +1239,17 @@ STATUS describeStreamCurl(UINT64 customData, PCHAR streamName, PServiceCallConte
 
 CleanUp:
 
+    if (startLocked) {
+        // Release the lock to let the awaiting handler thread to continue
+        pCallbacksProvider->clientCallbacks.unlockMutexFn(pCallbacksProvider->clientCallbacks.customData, pCurlRequest->startLock);
+    }
+
     if (STATUS_FAILED(retStatus)) {
         if (IS_VALID_TID_VALUE(threadId)) {
             THREAD_CANCEL(threadId);
         }
 
         freeCurlRequest(&pCurlRequest);
-    } else if (startLocked) {
-        // Release the lock to let the awaiting handler thread to continue
-        pCallbacksProvider->clientCallbacks.unlockMutexFn(pCallbacksProvider->clientCallbacks.customData, pCurlRequest->startLock);
     }
 
     if (shutdownLocked) {
@@ -1548,15 +1552,18 @@ STATUS getStreamingEndpointCurl(UINT64 customData, PCHAR streamName, PCHAR apiNa
 
 CleanUp:
 
+    if (startLocked) {
+        // Release the lock to let the awaiting handler thread to continue.
+        // This needs to be done before freeCurlRequest because there we will free the startLock mutex
+        pCallbacksProvider->clientCallbacks.unlockMutexFn(pCallbacksProvider->clientCallbacks.customData, pCurlRequest->startLock);
+    }
+
     if (STATUS_FAILED(retStatus)) {
         if (IS_VALID_TID_VALUE(threadId)) {
             THREAD_CANCEL(threadId);
         }
 
         freeCurlRequest(&pCurlRequest);
-    } else if (startLocked) {
-        // Release the lock to let the awaiting handler thread to continue
-        pCallbacksProvider->clientCallbacks.unlockMutexFn(pCallbacksProvider->clientCallbacks.customData, pCurlRequest->startLock);
     }
 
     if (shutdownLocked) {
@@ -1877,15 +1884,17 @@ STATUS tagResourceCurl(UINT64 customData, PCHAR streamArn, UINT32 tagCount, PTag
 
 CleanUp:
 
+    if (startLocked) {
+        // Release the lock to let the awaiting handler thread to continue
+        pCallbacksProvider->clientCallbacks.unlockMutexFn(pCallbacksProvider->clientCallbacks.customData, pCurlRequest->startLock);
+    }
+
     if (STATUS_FAILED(retStatus)) {
         if (IS_VALID_TID_VALUE(threadId)) {
             THREAD_CANCEL(threadId);
         }
 
         freeCurlRequest(&pCurlRequest);
-    } else if (startLocked) {
-        // Release the lock to let the awaiting handler thread to continue
-        pCallbacksProvider->clientCallbacks.unlockMutexFn(pCallbacksProvider->clientCallbacks.customData, pCurlRequest->startLock);
     }
 
     if (shutdownLocked) {

diff --git a/src/source/Response.c b/src/source/Response.c
@@ -31,7 +31,7 @@ STATUS createCurlResponse(PCurlRequest pCurlRequest, PCurlResponse* ppCurlRespon
 
     // init putMedia related members
     pCurlResponse->endOfStream = FALSE;
-    pCurlResponse->paused = TRUE;
+    ATOMIC_STORE_BOOL(&pCurlResponse->paused, TRUE);
     pCurlResponse->debugDumpFile = FALSE;
     pCurlResponse->debugDumpFilePath[0] = '\0';
 
@@ -456,8 +456,8 @@ STATUS notifyDataAvailable(PCurlResponse pCurlResponse, UINT64 durationAvailable
         DLOGV("[%s] Note data received: duration(100ns): %" PRIu64 " bytes %" PRIu64 " for stream handle %" PRIu64,
               pCurlResponse->pCurlRequest->streamName, durationAvailable, sizeAvailable, pCurlResponse->pCurlRequest->uploadHandle);
 
-        if (pCurlResponse->paused && pCurlResponse->pCurl != NULL) {
-            pCurlResponse->paused = FALSE;
+        if (ATOMIC_LOAD_BOOL(&pCurlResponse->paused) && pCurlResponse->pCurl != NULL) {
+            ATOMIC_STORE_BOOL(&pCurlResponse->paused, FALSE);
             // frequent pause unpause causes curl segfault in offline scenario
             THREAD_SLEEP(10 * HUNDREDS_OF_NANOS_IN_A_MILLISECOND);
             // un-pause curl
@@ -634,7 +634,7 @@ SIZE_T postReadCallback(PCHAR pBuffer, SIZE_T size, SIZE_T numItems, PVOID custo
     pCurlApiCallbacks = pCurlRequest->pCurlApiCallbacks;
     uploadHandle = pCurlResponse->pCurlRequest->uploadHandle;
 
-    if (pCurlResponse->paused) {
+    if (ATOMIC_LOAD_BOOL(&pCurlResponse->paused)) {
         bytesWritten = CURL_READFUNC_PAUSE;
         CHK(FALSE, retStatus);
     }
@@ -721,7 +721,7 @@ SIZE_T postReadCallback(PCHAR pBuffer, SIZE_T size, SIZE_T numItems, PVOID custo
             }
         }
     } else if (bytesWritten == CURL_READFUNC_PAUSE) {
-        pCurlResponse->paused = TRUE;
+        ATOMIC_STORE_BOOL(&pCurlResponse->paused, TRUE);
     }
 
     // Since curl is about to terminate gracefully, set flag to prevent shutdown thread from timing it out.

diff --git a/src/source/Response.h b/src/source/Response.h
@@ -62,7 +62,7 @@ struct __CurlResponse {
     BOOL endOfStream;
 
     // Whether curl is paused
-    volatile BOOL paused;
+    volatile ATOMIC_BOOL paused;
 
     // Whether to dump streaming session into mkv file
     BOOL debugDumpFile;