From 098510152b22b31aa87cb4f4551c90bdc45f9b9f Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Tue, 9 Jan 2024 17:14:21 -0500 Subject: [PATCH 01/16] enable tsan enable tsan --- .github/workflows/ci.yml | 53 +++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5ce565edb..7db06a9c3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -297,31 +297,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 From 1f105531cdd71e1155cc49c5f33c2a75258276d1 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Fri, 26 Jan 2024 11:48:05 -0500 Subject: [PATCH 02/16] make pCurlResponse->paused atomic bool --- src/source/Response.c | 10 +++++----- src/source/Response.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/source/Response.c b/src/source/Response.c index 9b0f8397e..e58505116 100644 --- 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->pause, 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 index 670b268e4..5f625f4a9 100644 --- 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; From 493eb1dc3d5e22463d8c96012a259c37ec7cd952 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Fri, 26 Jan 2024 12:04:37 -0500 Subject: [PATCH 03/16] fix typo --- src/source/Response.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/source/Response.c b/src/source/Response.c index e58505116..b96c05108 100644 --- a/src/source/Response.c +++ b/src/source/Response.c @@ -457,7 +457,7 @@ STATUS notifyDataAvailable(PCurlResponse pCurlResponse, UINT64 durationAvailable pCurlResponse->pCurlRequest->streamName, durationAvailable, sizeAvailable, pCurlResponse->pCurlRequest->uploadHandle); if (ATOMIC_LOAD_BOOL(&pCurlResponse->paused) && pCurlResponse->pCurl != NULL) { - ATOMIC_STORE_BOOL(&pCurlResponse->pause, FALSE); + 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 From 834e3368c61d73ad780882322b8f5680bb95d0df Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Fri, 26 Jan 2024 15:32:18 -0500 Subject: [PATCH 04/16] lock around callbacks which can be invoked from multiple threads so we don't get tsan data races --- tst/ProducerTestFixture.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tst/ProducerTestFixture.cpp b/tst/ProducerTestFixture.cpp index 5ef4cdea2..52401eb26 100644 --- a/tst/ProducerTestFixture.cpp +++ b/tst/ProducerTestFixture.cpp @@ -561,6 +561,7 @@ STATUS ProducerClientTestBase::curlEasyPerformHookFunc(PCurlResponse pCurlRespon // Get the test object ProducerClientTestBase* pTest = (ProducerClientTestBase*) pCurlResponse->pCurlRequest->pCurlApiCallbacks->hookCustomData; + MUTEX_LOCK(pTest->mTestCallbackLock); DLOGV("Curl perform hook for %s", pCurlResponse->pCurlRequest->requestInfo.url); @@ -623,6 +624,7 @@ STATUS ProducerClientTestBase::curlEasyPerformHookFunc(PCurlResponse pCurlRespon pTest->mPutMediaCallResult = SERVICE_CALL_RESULT_OK; } } + MUTEX_UNLOCK(pTest->mTestCallbackLock); return retStatus; } @@ -640,6 +642,7 @@ STATUS ProducerClientTestBase::curlWriteCallbackHookFunc(PCurlResponse pCurlResp // Get the test object ProducerClientTestBase* pTest = (ProducerClientTestBase*) pCurlResponse->pCurlRequest->pCurlApiCallbacks->hookCustomData; + MUTEX_LOCK(pTest->mTestCallbackLock); pTest->mWriteCallbackFnCount++; @@ -647,6 +650,7 @@ STATUS ProducerClientTestBase::curlWriteCallbackHookFunc(PCurlResponse pCurlResp *ppRetBuffer = pTest->mWriteBuffer; *pRetDataSize = pTest->mWriteDataSize; } + MUTEX_UNLOCK(pTest->mTestCallbackLock); return pTest->mWriteStatus; } @@ -671,6 +675,7 @@ STATUS ProducerClientTestBase::curlReadCallbackHookFunc(PCurlResponse pCurlRespo // Get the test object ProducerClientTestBase* pTest = (ProducerClientTestBase*) pCurlResponse->pCurlRequest->pCurlApiCallbacks->hookCustomData; + MUTEX_LOCK(pTest->mTestCallbackLock); pTest->mReadCallbackFnCount++; @@ -679,6 +684,7 @@ STATUS ProducerClientTestBase::curlReadCallbackHookFunc(PCurlResponse pCurlRespo } else { pTest->mReadStatus = status; } + MUTEX_UNLOCK(pTest->mTestCallbackLock); return pTest->mReadStatus; } From 97bfdd120fd2450dd1d2a7d6af8f56fa6e5256f0 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Fri, 26 Jan 2024 16:15:43 -0500 Subject: [PATCH 05/16] fix more tsan issues in tests --- tst/ProducerClientBasicTest.cpp | 59 +++++++++++++++++---------------- tst/ProducerTestFixture.cpp | 6 ++-- tst/ProducerTestFixture.h | 4 +-- 3 files changed, 36 insertions(+), 33 deletions(-) diff --git a/tst/ProducerClientBasicTest.cpp b/tst/ProducerClientBasicTest.cpp index 22a97a0e6..85f5f0681 100644 --- a/tst/ProducerClientBasicTest.cpp +++ b/tst/ProducerClientBasicTest.cpp @@ -10,8 +10,8 @@ class ProducerClientBasicTest : public ProducerClientTestBase { mStreamsCreated = CVAR_CREATE(); MEMSET(mClients, 0x00, SIZEOF(mClients)); MEMSET(mClientCallbacks, 0x00, SIZEOF(mClientCallbacks)); - mActiveStreamCount = 0; - mActiveClientCount = 0; + ATOMIC_STORE(&mActiveStreamCount, 0); + ATOMIC_STORE(&mActiveClientCount, 0); } VOID deinitialize() @@ -35,8 +35,8 @@ class ProducerClientBasicTest : public ProducerClientTestBase { CVAR mStreamsCreated; CLIENT_HANDLE mClients[TEST_STREAM_COUNT]; PClientCallbacks mClientCallbacks[TEST_STREAM_COUNT]; - volatile UINT32 mActiveStreamCount; - volatile UINT32 mActiveClientCount; + volatile SIZE_T mActiveStreamCount; + volatile SIZE_T mActiveClientCount; }; extern ProducerClientTestBase* gProducerClientTestBase; @@ -137,7 +137,8 @@ PVOID ProducerClientBasicTest::staticCreateProducerClientRoutine(PVOID arg) EXPECT_EQ(STATUS_SUCCESS, retStatus = createKinesisVideoStreamSync(pTest->mClients[index], &streamInfo, &pTest->mStreams[index])); - if (++pTest->mActiveStreamCount == TEST_STREAM_COUNT) { + ATOMIC_INCREMENT(&pTest->mActiveStreamCount); + if (ATOMIC_LOAD(&pTest->mActiveStreamCount) == TEST_STREAM_COUNT) { CVAR_SIGNAL(pTest->mStreamsCreated); } @@ -196,7 +197,8 @@ PVOID ProducerClientBasicTest::staticCreateProducerRoutine(PVOID arg) retStatus = createKinesisVideoStreamSync(pTest->mClientHandle, &streamInfo, &pTest->mStreams[index]); - if (++pTest->mActiveStreamCount == TEST_STREAM_COUNT) { + ATOMIC_INCREMENT(&pTest->mActiveStreamCount); + if (ATOMIC_LOAD(&pTest->mActiveStreamCount) == TEST_STREAM_COUNT) { CVAR_SIGNAL(pTest->mStreamsCreated); } @@ -223,10 +225,10 @@ PVOID ProducerClientBasicTest::staticProducerClientStartRoutine(PVOID arg) EXPECT_EQ(STATUS_SUCCESS, kinesisVideoStreamGetStreamInfo(streamHandle, &pStreamInfo)); // Set an indicator that the producer is not stopped - pTest->mProducerStopped = FALSE; + ATOMIC_STORE_BOOL(&pTest->mProducerStopped, FALSE); // Increment the active stream/producer count - pTest->mActiveClientCount++; + ATOMIC_INCREMENT(&pTest->mActiveClientCount); // Loop until cancelled frame.version = FRAME_CURRENT_VERSION; @@ -245,7 +247,7 @@ PVOID ProducerClientBasicTest::staticProducerClientStartRoutine(PVOID arg) EXPECT_EQ(STATUS_SUCCESS, kinesisVideoStreamFormatChanged(streamHandle, cpdSize, cpd, DEFAULT_VIDEO_TRACK_ID)); - while (!pTest->mStopProducer) { + while (!ATOMIC_LOAD_BOOL(&pTest->mStopProducer)) { // Produce frames timestamp = GETTIME(); @@ -319,8 +321,9 @@ PVOID ProducerClientBasicTest::staticProducerClientStartRoutine(PVOID arg) pTest->mStreams[streamIndex] = INVALID_STREAM_HANDLE_VALUE; // Indicate that the producer routine had stopped - if (--pTest->mActiveClientCount == 0) { - pTest->mProducerStopped = true; + ATOMIC_DECREMENT(&pTest->mActiveClientCount); + if (ATOMIC_LOAD(&pTest->mActiveClientCount) == 0) { + ATOMIC_STORE_BOOL(&pTest->mProducerStopped, TRUE); } return NULL; @@ -356,7 +359,7 @@ PVOID ProducerClientTestBase::basicProducerRoutine(STREAM_HANDLE streamHandle, S EXPECT_EQ(STATUS_SUCCESS, kinesisVideoStreamGetStreamInfo(streamHandle, &pStreamInfo)); // Set an indicator that the producer is not stopped - mProducerStopped = FALSE; + ATOMIC_STORE_BOOL(&mProducerStopped, FALSE); // Loop until cancelled frame.version = FRAME_CURRENT_VERSION; @@ -375,7 +378,7 @@ PVOID ProducerClientTestBase::basicProducerRoutine(STREAM_HANDLE streamHandle, S EXPECT_EQ(STATUS_SUCCESS, kinesisVideoStreamFormatChanged(streamHandle, cpdSize, cpd, DEFAULT_VIDEO_TRACK_ID)); - while (!mStopProducer) { + while (!ATOMIC_LOAD_BOOL(&mStopProducer)) { // Produce frames if (IS_OFFLINE_STREAMING_MODE(streamingType)) { timestamp += frame.duration; @@ -459,7 +462,7 @@ EXPECT_TRUE(kinesis_video_stream->putFrame(eofr)); EXPECT_EQ(STATUS_SUCCESS, stopKinesisVideoStreamSync(streamHandle)) << "Timed out awaiting for the stream stop notification"; // Indicate that the producer routine had stopped - mProducerStopped = true; + ATOMIC_STORE_BOOL(&mProducerStopped, TRUE); return NULL; } @@ -489,7 +492,7 @@ TEST_F(ProducerClientBasicTest, create_produce_stream) for (UINT32 iter = 0; iter < 10; iter++) { THREAD_SLEEP(10 * HUNDREDS_OF_NANOS_IN_A_SECOND); DLOGD("Stopping the streams"); - mStopProducer = TRUE; + ATOMIC_STORE_BOOL(&mStopProducer, TRUE); DLOGD("Waiting for the streams to finish and close..."); THREAD_SLEEP(10 * HUNDREDS_OF_NANOS_IN_A_SECOND); @@ -501,7 +504,7 @@ TEST_F(ProducerClientBasicTest, create_produce_stream) } DLOGD("Starting the streams again"); - mStopProducer = FALSE; + ATOMIC_STORE_BOOL(&mStopProducer, FALSE); // Create new streams for (UINT32 i = 0; i < TEST_STREAM_COUNT; i++) { @@ -521,7 +524,7 @@ TEST_F(ProducerClientBasicTest, create_produce_stream) THREAD_SLEEP(2*TEST_EXECUTION_DURATION); // Indicate the cancel for the threads - mStopProducer = TRUE; + ATOMIC_STORE_BOOL(&mStopProducer, TRUE); // Join the thread and wait to exit. // NOTE: This is not a right way of doing it as for the multiple stream scenario @@ -531,9 +534,9 @@ TEST_F(ProducerClientBasicTest, create_produce_stream) UINT32 index = 0; do { THREAD_SLEEP(100 * HUNDREDS_OF_NANOS_IN_A_MILLISECOND); - } while (index++ < 300 && !mProducerStopped); + } while (index++ < 300 && !ATOMIC_LOAD_BOOL(&mProducerStopped)); - EXPECT_TRUE(mProducerStopped) << "Producer thread failed to stop cleanly"; + EXPECT_TRUE(ATOMIC_LOAD_BOOL(&mProducerStopped)) << "Producer thread failed to stop cleanly"; // We will block for some time due to an incorrect implementation of the awaiting code // NOTE: The proper implementation should use synchronization primitives to await for the @@ -576,14 +579,14 @@ TEST_F(ProducerClientBasicTest, create_produce_stream_parallel) THREAD_SLEEP(2*TEST_EXECUTION_DURATION); // Indicate the cancel for the threads - mStopProducer = TRUE; + ATOMIC_STORE_BOOL(&mStopProducer, TRUE); UINT32 index = 0; do { THREAD_SLEEP(100 * HUNDREDS_OF_NANOS_IN_A_MILLISECOND); - } while (index++ < 300 && !mProducerStopped); + } while (index++ < 300 && !ATOMIC_LOAD_BOOL(&mProducerStopped)); - EXPECT_TRUE(mProducerStopped) << "Producer thread failed to stop cleanly"; + EXPECT_TRUE(ATOMIC_LOAD_BOOL(&mProducerStopped)) << "Producer thread failed to stop cleanly"; THREAD_SLEEP(10 * HUNDREDS_OF_NANOS_IN_A_MILLISECOND); @@ -622,14 +625,14 @@ TEST_F(ProducerClientBasicTest, create_produce_client_parallel) THREAD_SLEEP(2*TEST_EXECUTION_DURATION); // Indicate the cancel for the threads - mStopProducer = TRUE; + ATOMIC_STORE_BOOL(&mStopProducer, TRUE); UINT32 index = 0; do { THREAD_SLEEP(100 * HUNDREDS_OF_NANOS_IN_A_MILLISECOND); - } while (index++ < 300 && !mProducerStopped); + } while (index++ < 300 && !ATOMIC_LOAD_BOOL(&mProducerStopped)); - EXPECT_TRUE(mProducerStopped) << "Producer thread failed to stop cleanly"; + EXPECT_TRUE(ATOMIC_LOAD_BOOL(&mProducerStopped)) << "Producer thread failed to stop cleanly"; THREAD_SLEEP(10 * HUNDREDS_OF_NANOS_IN_A_MILLISECOND); @@ -662,7 +665,7 @@ TEST_F(ProducerClientBasicTest, cachingEndpointProvider_Returns_EndpointFromCach THREAD_SLEEP(TEST_STREAMING_TOKEN_DURATION * ITERATION_COUNT); // Indicate the cancel for the threads - mStopProducer = TRUE; + ATOMIC_STORE_BOOL(&mStopProducer, TRUE); // Join the thread and wait to exit. // NOTE: This is not a right way of doing it as for the multiple stream scenario @@ -672,9 +675,9 @@ TEST_F(ProducerClientBasicTest, cachingEndpointProvider_Returns_EndpointFromCach UINT32 index = 0; do { THREAD_SLEEP(100 * HUNDREDS_OF_NANOS_IN_A_MILLISECOND); - } while (index++ < 300 && !mProducerStopped); + } while (index++ < 300 && !ATOMIC_LOAD_BOOL(&mProducerStopped)); - EXPECT_TRUE(mProducerStopped) << "Producer thread failed to stop cleanly"; + EXPECT_TRUE(ATOMIC_LOAD_BOOL(&mProducerStopped)) << "Producer thread failed to stop cleanly"; // Expect the number of calls EXPECT_EQ(((ITERATION_COUNT + 1) * TEST_STREAM_COUNT), mPutStreamFnCount); diff --git a/tst/ProducerTestFixture.cpp b/tst/ProducerTestFixture.cpp index 52401eb26..aff891510 100644 --- a/tst/ProducerTestFixture.cpp +++ b/tst/ProducerTestFixture.cpp @@ -181,9 +181,7 @@ ProducerClientTestBase::ProducerClientTestBase() : mAccessKeyIdSet(FALSE), mCaCertPath(NULL), mProducerThread(INVALID_TID_VALUE), - mProducerStopped(FALSE), mStartProducer(FALSE), - mStopProducer(FALSE), mAccessKey(NULL), mSecretKey(NULL), mSessionToken(NULL), @@ -238,6 +236,8 @@ ProducerClientTestBase::ProducerClientTestBase() : assert(STRTOUI32(logLevelStr, NULL, 10, &this->loggerLogLevel) == STATUS_SUCCESS); SET_LOGGER_LOG_LEVEL(this->loggerLogLevel); } + ATOMIC_STORE_BOOL(&mStopProducer, FALSE); + ATOMIC_STORE_BOOL(&mProducerStopped, FALSE); // Store the function pointers gTotalProducerClientMemoryUsage = 0; @@ -535,7 +535,7 @@ STATUS ProducerClientTestBase::createTestStream(UINT32 index, STREAMING_TYPE str VOID ProducerClientTestBase::freeStreams(BOOL sync) { - mProducerStopped = TRUE; + ATOMIC_STORE_BOOL(&mProducerStopped, TRUE); for (UINT32 i = 0; i < TEST_STREAM_COUNT; i++) { DLOGD("Freeing stream index %u with handle value %" PRIu64 " %s", i, mStreams[i], sync ? "synchronously" : "asynchronously"); diff --git a/tst/ProducerTestFixture.h b/tst/ProducerTestFixture.h index 2754a7e29..ce6910d22 100644 --- a/tst/ProducerTestFixture.h +++ b/tst/ProducerTestFixture.h @@ -272,8 +272,8 @@ class ProducerClientTestBase : public ::testing::Test { STREAM_HANDLE mStreams[TEST_MAX_STREAM_COUNT]; volatile bool mStartProducer; - volatile bool mStopProducer; - volatile bool mProducerStopped; + volatile ATOMIC_BOOL mStopProducer; + volatile ATOMIC_BOOL mProducerStopped; // Test callbacks ApiCallbacks mApiCallbacks; From 0d38f93653f035fbf9e7c5952c9824a596852ca3 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Mon, 29 Jan 2024 11:51:14 -0500 Subject: [PATCH 06/16] test pic branch safe-gmtime --- CMake/Dependencies/libkvspic-CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMake/Dependencies/libkvspic-CMakeLists.txt b/CMake/Dependencies/libkvspic-CMakeLists.txt index c8ebe188d..50b87d4f8 100644 --- a/CMake/Dependencies/libkvspic-CMakeLists.txt +++ b/CMake/Dependencies/libkvspic-CMakeLists.txt @@ -7,7 +7,7 @@ include(ExternalProject) # clone repo only ExternalProject_Add(libkvspic-download GIT_REPOSITORY https://github.com/awslabs/amazon-kinesis-video-streams-pic.git - GIT_TAG develop + GIT_TAG safe-gmtime SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-src" BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-build" CMAKE_ARGS From f7dd65151fee6591a3f2ef624c1b2e3fb6a4e480 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Mon, 29 Jan 2024 12:50:39 -0500 Subject: [PATCH 07/16] use GMTIME_SAFE everywhere --- src/source/Common/AwsV4Signer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/source/Common/AwsV4Signer.c b/src/source/Common/AwsV4Signer.c index 3eddaf44c..5535c67db 100644 --- 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_SAFE(&timeT)); CHK(retSize > 0, STATUS_BUFFER_TOO_SMALL); pDateTimeStr[retSize] = '\0'; From a65fd5ee2477afa424378de24803ff17aace4114 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Mon, 29 Jan 2024 15:59:59 -0500 Subject: [PATCH 08/16] update to GMTIME_THREAD_SAFE, also lock around increment in callback --- src/source/Common/AwsV4Signer.c | 2 +- tst/ProducerTestFixture.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/source/Common/AwsV4Signer.c b/src/source/Common/AwsV4Signer.c index 5535c67db..19b5831ad 100644 --- 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_SAFE(&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/tst/ProducerTestFixture.cpp b/tst/ProducerTestFixture.cpp index aff891510..3ff1f3dc5 100644 --- a/tst/ProducerTestFixture.cpp +++ b/tst/ProducerTestFixture.cpp @@ -691,10 +691,10 @@ STATUS ProducerClientTestBase::curlReadCallbackHookFunc(PCurlResponse pCurlRespo STATUS ProducerClientTestBase::testFreeApiCallbackFunc(PUINT64 customData) { - ProducerClientTestBase* pTestBase = (ProducerClientTestBase*) *customData; - - pTestBase->mFreeApiCallbacksFnCount++; - + ProducerClientTestBase* pTest = (ProducerClientTestBase*) *customData; + MUTEX_LOCK(pTest->mTestCallbackLock); + pTest->mFreeApiCallbacksFnCount++; + MUTEX_UNLOCK(pTest->mTestCallbackLock); return STATUS_SUCCESS; } From f7a0a7808fdf7d6ab6c79ad97af66fc21f4e22e8 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Mon, 29 Jan 2024 17:48:42 -0500 Subject: [PATCH 09/16] revert back to develp, set language to C --- CMake/Dependencies/libcurl-CMakeLists.txt | 2 +- CMake/Dependencies/libjsmn-CMakeLists.txt | 2 +- CMake/Dependencies/libkvspic-CMakeLists.txt | 4 ++-- CMake/Dependencies/libmbedtls-CMakeLists.txt | 2 +- CMake/Dependencies/libopenssl-CMakeLists.txt | 2 +- CMake/Dependencies/libwebsockets-CMakeLists.txt | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/CMake/Dependencies/libcurl-CMakeLists.txt b/CMake/Dependencies/libcurl-CMakeLists.txt index 7d369197e..77c895f3e 100644 --- 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/libjsmn-CMakeLists.txt b/CMake/Dependencies/libjsmn-CMakeLists.txt index b16ca0f5b..5a11e7f08 100644 --- a/CMake/Dependencies/libjsmn-CMakeLists.txt +++ b/CMake/Dependencies/libjsmn-CMakeLists.txt @@ -1,6 +1,6 @@ cmake_minimum_required(VERSION 3.6.3) -project(libjsmn-download NONE) +project(libjsmn-download LANGUAGES C) include(ExternalProject) ExternalProject_Add(project_libjsmn diff --git a/CMake/Dependencies/libkvspic-CMakeLists.txt b/CMake/Dependencies/libkvspic-CMakeLists.txt index 50b87d4f8..4e52d8e09 100644 --- a/CMake/Dependencies/libkvspic-CMakeLists.txt +++ b/CMake/Dependencies/libkvspic-CMakeLists.txt @@ -1,13 +1,13 @@ cmake_minimum_required(VERSION 3.6.3) -project(libkvspic-download NONE) +project(libkvspic-download LANGUAGES C) include(ExternalProject) # clone repo only ExternalProject_Add(libkvspic-download GIT_REPOSITORY https://github.com/awslabs/amazon-kinesis-video-streams-pic.git - GIT_TAG safe-gmtime + GIT_TAG develop SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-src" BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-build" CMAKE_ARGS diff --git a/CMake/Dependencies/libmbedtls-CMakeLists.txt b/CMake/Dependencies/libmbedtls-CMakeLists.txt index cd89da2b4..3b579b18e 100644 --- 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/libopenssl-CMakeLists.txt b/CMake/Dependencies/libopenssl-CMakeLists.txt index c97037a84..a206a69b7 100644 --- a/CMake/Dependencies/libopenssl-CMakeLists.txt +++ b/CMake/Dependencies/libopenssl-CMakeLists.txt @@ -1,6 +1,6 @@ cmake_minimum_required(VERSION 3.6.3) -project(libopenssl-download NONE) +project(libopenssl-download LANGUAGES C) if (WIN32) find_program(MAKE_EXE NAMES nmake) diff --git a/CMake/Dependencies/libwebsockets-CMakeLists.txt b/CMake/Dependencies/libwebsockets-CMakeLists.txt index e00ee305d..5325b9efd 100644 --- 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) From c6884e8043b374d2f8a2d06170092fed77b0c6dd Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Mon, 29 Jan 2024 20:30:58 -0500 Subject: [PATCH 10/16] dummy commit to make ci run --- CMake/Dependencies/libgtest-CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/CMake/Dependencies/libgtest-CMakeLists.txt b/CMake/Dependencies/libgtest-CMakeLists.txt index 916087e76..14e4a09cb 100644 --- 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 From ed24338bdbf5eadc3e6ae5833a666f50ab4f988e Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Mon, 29 Jan 2024 20:32:52 -0500 Subject: [PATCH 11/16] undo setting languages to libjsmn cmake since github is complaining about conflict --- CMake/Dependencies/libjsmn-CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMake/Dependencies/libjsmn-CMakeLists.txt b/CMake/Dependencies/libjsmn-CMakeLists.txt index 5a11e7f08..b16ca0f5b 100644 --- a/CMake/Dependencies/libjsmn-CMakeLists.txt +++ b/CMake/Dependencies/libjsmn-CMakeLists.txt @@ -1,6 +1,6 @@ cmake_minimum_required(VERSION 3.6.3) -project(libjsmn-download LANGUAGES C) +project(libjsmn-download NONE) include(ExternalProject) ExternalProject_Add(project_libjsmn From 8ce663e9ffd65840bc555842dfd6c8a146bdeb29 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Tue, 30 Jan 2024 13:06:42 -0500 Subject: [PATCH 12/16] test new fixes from PIC --- CMake/Dependencies/libkvspic-CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMake/Dependencies/libkvspic-CMakeLists.txt b/CMake/Dependencies/libkvspic-CMakeLists.txt index 4e52d8e09..9d36c5b01 100644 --- a/CMake/Dependencies/libkvspic-CMakeLists.txt +++ b/CMake/Dependencies/libkvspic-CMakeLists.txt @@ -7,7 +7,7 @@ include(ExternalProject) # clone repo only ExternalProject_Add(libkvspic-download GIT_REPOSITORY https://github.com/awslabs/amazon-kinesis-video-streams-pic.git - GIT_TAG develop + GIT_TAG fix-tsan-issues SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-src" BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-build" CMAKE_ARGS From c457427045aec2815f09ad38bf7bab764ac51f92 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Fri, 2 Feb 2024 18:51:59 -0500 Subject: [PATCH 13/16] revert back to develop --- CMake/Dependencies/libkvspic-CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMake/Dependencies/libkvspic-CMakeLists.txt b/CMake/Dependencies/libkvspic-CMakeLists.txt index 9d36c5b01..4e52d8e09 100644 --- a/CMake/Dependencies/libkvspic-CMakeLists.txt +++ b/CMake/Dependencies/libkvspic-CMakeLists.txt @@ -7,7 +7,7 @@ include(ExternalProject) # clone repo only ExternalProject_Add(libkvspic-download GIT_REPOSITORY https://github.com/awslabs/amazon-kinesis-video-streams-pic.git - GIT_TAG fix-tsan-issues + GIT_TAG develop SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-src" BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-build" CMAKE_ARGS From 7efa9a5ddb0dd57a5adb61d43d96f70c5d242146 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Fri, 2 Feb 2024 23:50:44 -0500 Subject: [PATCH 14/16] set frame buffer to 0 for tests, test new pic branch with lock-order-inversion potential deadlock in teardown addressed --- CMake/Dependencies/libkvspic-CMakeLists.txt | 2 +- tst/ProducerTestFixture.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CMake/Dependencies/libkvspic-CMakeLists.txt b/CMake/Dependencies/libkvspic-CMakeLists.txt index 4e52d8e09..696a1551d 100644 --- a/CMake/Dependencies/libkvspic-CMakeLists.txt +++ b/CMake/Dependencies/libkvspic-CMakeLists.txt @@ -7,7 +7,7 @@ include(ExternalProject) # clone repo only ExternalProject_Add(libkvspic-download GIT_REPOSITORY https://github.com/awslabs/amazon-kinesis-video-streams-pic.git - GIT_TAG develop + GIT_TAG tsan-address-deadlock-in-teardown SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-src" BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-build" CMAKE_ARGS diff --git a/tst/ProducerTestFixture.cpp b/tst/ProducerTestFixture.cpp index 3ff1f3dc5..72576786f 100644 --- a/tst/ProducerTestFixture.cpp +++ b/tst/ProducerTestFixture.cpp @@ -332,7 +332,7 @@ ProducerClientTestBase::ProducerClientTestBase() : mFps = TEST_FPS; mKeyFrameInterval = TEST_FPS; mFrameSize = TEST_FRAME_SIZE; - mFrameBuffer = (PBYTE) MEMALLOC(mFrameSize); + mFrameBuffer = (PBYTE) MEMCALLOC(1, mFrameSize); mFrame.duration = HUNDREDS_OF_NANOS_IN_A_SECOND / mFps; mFrame.frameData = mFrameBuffer; From 5566926edfd26c801111fe32758f91d92586bd5c Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Sat, 3 Feb 2024 01:11:15 -0500 Subject: [PATCH 15/16] release lock before free to fix destroy locked mutex bug which is undefined behavior --- src/source/CurlApiCallbacks.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/source/CurlApiCallbacks.c b/src/source/CurlApiCallbacks.c index 073dc5b22..c045e0e1e 100644 --- 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) { From fe6f7478abcff1f55d30b3a025d77e21a25e0c59 Mon Sep 17 00:00:00 2001 From: Hassan Sahibzada Date: Mon, 5 Feb 2024 13:11:24 -0500 Subject: [PATCH 16/16] move pic branch back to develop, remove tsan from CI --- .github/workflows/ci.yml | 56 ++++++++++----------- CMake/Dependencies/libkvspic-CMakeLists.txt | 2 +- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e1eab6383..a62b22f1d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -295,34 +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 - - name: Run tests - run: | - cd build - 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/libkvspic-CMakeLists.txt b/CMake/Dependencies/libkvspic-CMakeLists.txt index 696a1551d..4e52d8e09 100644 --- a/CMake/Dependencies/libkvspic-CMakeLists.txt +++ b/CMake/Dependencies/libkvspic-CMakeLists.txt @@ -7,7 +7,7 @@ include(ExternalProject) # clone repo only ExternalProject_Add(libkvspic-download GIT_REPOSITORY https://github.com/awslabs/amazon-kinesis-video-streams-pic.git - GIT_TAG tsan-address-deadlock-in-teardown + GIT_TAG develop SOURCE_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-src" BINARY_DIR "${CMAKE_CURRENT_BINARY_DIR}/kvspic-build" CMAKE_ARGS