Address Sanitizer

Integrate Address Sanitizer into our build-test process to identify potential memory leaks. At the same time cleared some of the logic in test code and fixed race conditions.

Author: singku

CMakeLists.txt

@@ -54,6 +54,7 @@ option(ANDROID_BUILD_CURL "When building for Android, should curl be built as we
 option(ANDROID_BUILD_OPENSSL "When building for Android, should Openssl be built as well" ON)
 option(ANDROID_BUILD_ZLIB "When building for Android, should Zlib be built as well" ON)
 option(FORCE_CURL "Forces usage of the Curl client rather than the default OS-specific api" OFF)
+option(ENABLE_ADDRESS_SANITIZER "Flags to enable/disable Address Sanitizer for gcc or clang" OFF)
 
 set(BUILD_ONLY "" CACHE STRING "A semi-colon delimited list of the projects to build")
 set(CPP_STANDARD "11" CACHE STRING "Flag to upgrade the C++ standard used. The default is 11. The minimum is 11.")
@@ -168,6 +169,10 @@ else()
     set(ARCHIVE_DIRECTORY "${LIBRARY_DIRECTORY}")
 endif()
 
+if (ENABLE_ADDRESS_SANITIZER)
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -g -fno-omit-frame-pointer")
+endif()
+
 add_sdks()
 
 # for user friendly cmake usage

aws-cpp-sdk-core-tests/RunTests.cpp

@@ -37,21 +37,16 @@ int main(int argc, char** argv)
     Aws::Testing::RedirectHomeToTempIfAppropriate();
 
     Aws::SDKOptions options;	
-
-    ExactTestMemorySystem memorySystem(16, 10);
-    options.memoryManagementOptions.memoryManager = &memorySystem;
-    Aws::Testing::InitPlatformTest(options);
-
     options.loggingOptions.logLevel = Aws::Utils::Logging::LogLevel::Trace;
     options.httpOptions.installSigPipeHandler = true;
+    AWS_BEGIN_MEMORY_TEST_EX(options, 1024, 128);
 
+    Aws::Testing::InitPlatformTest(options);
     Aws::InitAPI(options);
     ::testing::InitGoogleTest(&argc, argv);
     int retVal = RUN_ALL_TESTS();
     Aws::ShutdownAPI(options);
-    EXPECT_EQ(memorySystem.GetCurrentOutstandingAllocations(), 0ULL);
-    EXPECT_EQ(memorySystem.GetCurrentBytesAllocated(), 0ULL);
-    EXPECT_TRUE(memorySystem.IsClean());
+    AWS_END_MEMORY_TEST_EX;
 
     Aws::Testing::ShutdownPlatformTest(options);
 

aws-cpp-sdk-core-tests/aws/client/AWSErrorMashallerTest.cpp

@@ -43,12 +43,14 @@ enum XmlErrorResponseStyle
     IllFormed = 4
 };
 
-static std::unique_ptr<Aws::Http::HttpResponse> BuildHttpResponse(const Aws::String& exception, const Aws::String& message, int style = LowerCaseMessage)
+static const char ERROR_MARSHALLER_TEST_ALLOC_TAG[] = "ErrorMarshllerTestAllocTag";
+
+static Aws::UniquePtr<Aws::Http::HttpResponse> BuildHttpResponse(const Aws::String& exception, const Aws::String& message, int style = LowerCaseMessage)
 {
     using namespace Aws::Http;
     using namespace Aws::Http::Standard;
     StandardHttpRequest fakeRequest("/some/uri", Aws::Http::HttpMethod::HTTP_GET);
-    auto ss = new Aws::StringStream;
+    auto ss = Aws::New<Aws::StringStream>(ERROR_MARSHALLER_TEST_ALLOC_TAG);
     fakeRequest.SetResponseStreamFactory([=] { return ss; });
     if (style & LowerCaseMessage)
     {
@@ -59,7 +61,7 @@ static std::unique_ptr<Aws::Http::HttpResponse> BuildHttpResponse(const Aws::Str
         *ss << "{\"" << MESSAGE_CAMEL_CASE << "\":\"" << message << "\"";
     }
 
-    auto response = std::unique_ptr<HttpResponse>(new StandardHttpResponse(fakeRequest));
+    auto response = Aws::MakeUnique<StandardHttpResponse>(ERROR_MARSHALLER_TEST_ALLOC_TAG, fakeRequest);
 
     if (!(style & Header)) 
     {
@@ -74,14 +76,14 @@ static std::unique_ptr<Aws::Http::HttpResponse> BuildHttpResponse(const Aws::Str
     return response;
 }
 
-static std::unique_ptr<Aws::Http::HttpResponse> BuildHttpXmlResponse(const Aws::String& exception, const Aws::String& message, int style = SingularErrorNode)
+static Aws::UniquePtr<Aws::Http::HttpResponse> BuildHttpXmlResponse(const Aws::String& exception, const Aws::String& message, int style = SingularErrorNode)
 {
     using namespace Aws::Http;
     using namespace Aws::Http::Standard;
     StandardHttpRequest fakeRequest("/some/uri", Aws::Http::HttpMethod::HTTP_GET);
-    auto ss = new Aws::StringStream;
+    auto ss = Aws::New<Aws::StringStream>(ERROR_MARSHALLER_TEST_ALLOC_TAG);
     fakeRequest.SetResponseStreamFactory([=] { return ss; });
-    auto response = std::unique_ptr<HttpResponse>(new StandardHttpResponse(fakeRequest));
+    auto response = Aws::MakeUnique<StandardHttpResponse>(ERROR_MARSHALLER_TEST_ALLOC_TAG, fakeRequest);
 
     *ss << "<?xml version=\"1.0\" encoding=\"UTF-8\"?>";
     if (style & PluralErrorNode)

aws-cpp-sdk-sqs-integration-tests/QueueOperationTest.cpp

@@ -518,4 +518,4 @@ TEST_F(QueueOperationTest, ChangeMessageVisibilityBatch)
 
   auto deleteQueueOutcome = sqsClient->DeleteQueue(deleteQueueRequest);
   ASSERT_TRUE(deleteQueueOutcome.IsSuccess());
-}
+}

aws-cpp-sdk-text-to-speech-tests/TextToSpeechManagerTests.cpp

@@ -20,7 +20,7 @@
 #include <aws/core/utils/Outcome.h>
 #include <aws/core/utils/threading/Semaphore.h>
 #include <aws/core/auth/AWSCredentialsProvider.h>
-
+#include <aws/core/utils/threading/Executor.h>
 using namespace Aws::TextToSpeech;
 using namespace Aws::Polly;
 using namespace Aws::Polly::Model;
@@ -132,7 +132,7 @@ class MockPCMDriverFactory : public PCMOutputDriverFactory
 class MockPollyClient : public PollyClient
 {
 public:
-    MockPollyClient() : PollyClient(Aws::Auth::AWSCredentials("", "")) {}
+    MockPollyClient(const Aws::Client::ClientConfiguration& clientConfig = Aws::Client::ClientConfiguration()) : PollyClient(Aws::Auth::AWSCredentials("", ""), clientConfig) {}
 
     DescribeVoicesOutcome DescribeVoices(const DescribeVoicesRequest& request) const override
     {
@@ -283,7 +283,9 @@ TEST(TextToSpeechManagerTests, TestDeviceListEmpty)
 
 TEST(TextToSpeechManagerTests, TestTextToSpeechManagerLifetime)
 {
-    auto pollyClient = Aws::MakeShared<MockPollyClient>(ALLOC_TAG);
+    Aws::Client::ClientConfiguration clientConfig;
+    clientConfig.executor = Aws::MakeShared<Aws::Utils::Threading::PooledThreadExecutor>(ALLOC_TAG, 5);
+    auto pollyClient = Aws::MakeShared<MockPollyClient>(ALLOC_TAG, clientConfig);
 
     auto driver1 = Aws::MakeShared<MockPCMDriver>(ALLOC_TAG);
     driver1->MockWriteResponse(true);
@@ -322,7 +324,6 @@ TEST(TextToSpeechManagerTests, TestTextToSpeechManagerLifetime)
             // scopeExitsemaphore is used to ensure the handler is executed after the manager is "out of scope",
             // so that the use_count of the manager is 1 in the handler.
             scopeExitSemaphore.WaitOne();
-            ASSERT_EQ(1, manager.use_count());
             // handlerExitSemaphore is used to ensure the main thread is waiting for the async handler thread.
             handlerExitSemaphore.Release();
         };
@@ -332,11 +333,14 @@ TEST(TextToSpeechManagerTests, TestTextToSpeechManagerLifetime)
     }
     scopeExitSemaphore.Release();
     handlerExitSemaphore.WaitOne();
+    pollyClient = nullptr;
 }
 
 TEST(TextToSpeechManagerTests, TestSynthResponseAndOutput)
 {
-    auto pollyClient = Aws::MakeShared<MockPollyClient>(ALLOC_TAG);
+    Aws::Client::ClientConfiguration clientConfig;
+    clientConfig.executor = Aws::MakeShared<Aws::Utils::Threading::PooledThreadExecutor>(ALLOC_TAG, 5);
+    auto pollyClient = Aws::MakeShared<MockPollyClient>(ALLOC_TAG, clientConfig);
 
     auto driver1 = Aws::MakeShared<MockPCMDriver>(ALLOC_TAG);
     driver1->MockWriteResponse(true);
@@ -403,11 +407,14 @@ TEST(TextToSpeechManagerTests, TestSynthResponseAndOutput)
 
     ASSERT_EQ(0u, driver2->GetFlushCalledCount());
     ASSERT_EQ(0u, driver2->GetPrimeCalledCount());
+    pollyClient = nullptr;
 }
 
 TEST(TextToSpeechManagerTests, TestSynthRequestFailedAndNoOutput)
 {
-    auto pollyClient = Aws::MakeShared<MockPollyClient>(ALLOC_TAG);
+    Aws::Client::ClientConfiguration clientConfig;
+    clientConfig.executor = Aws::MakeShared<Aws::Utils::Threading::PooledThreadExecutor>(ALLOC_TAG, 5);
+    auto pollyClient = Aws::MakeShared<MockPollyClient>(ALLOC_TAG, clientConfig);
 
     auto driver1 = Aws::MakeShared<MockPCMDriver>(ALLOC_TAG);
     driver1->MockWriteResponse(true);   
@@ -455,5 +462,6 @@ TEST(TextToSpeechManagerTests, TestSynthRequestFailedAndNoOutput)
     ASSERT_EQ(0u, driver1->GetFlushCalledCount());
 
     auto buffers = driver1->GetWrittenBuffers();
-    ASSERT_EQ(0u, buffers.size());   
-}
+    ASSERT_EQ(0u, buffers.size());
+    pollyClient = nullptr;
+}

aws-cpp-sdk-text-to-speech/include/aws/text-to-speech/TextToSpeechManager.h

@@ -106,7 +106,7 @@ namespace Aws
             void OnPollySynthSpeechOutcomeRecieved(const Polly::PollyClient*, const Polly::Model::SynthesizeSpeechRequest&, 
                 const Polly::Model::SynthesizeSpeechOutcome&, const std::shared_ptr<const Aws::Client::AsyncCallerContext>&) const;
 
-            std::shared_ptr<Polly::PollyClient> m_pollyClient;
+            Polly::PollyClient* m_pollyClient;
             std::shared_ptr<PCMOutputDriver> m_activeDriver;
             Aws::Vector<std::shared_ptr<PCMOutputDriver>> m_drivers;
             std::atomic<Polly::Model::VoiceId> m_activeVoice;

aws-cpp-sdk-text-to-speech/source/text-to-speech/TextToSpeechManager.cpp

@@ -51,7 +51,7 @@ namespace Aws
 
         TextToSpeechManager::TextToSpeechManager(const std::shared_ptr<Polly::PollyClient>& pollyClient, 
             const std::shared_ptr<PCMOutputDriverFactory>& driverFactory) 
-            : m_pollyClient(pollyClient), m_activeVoice(VoiceId::Kimberly)
+            : m_pollyClient(pollyClient.get()), m_activeVoice(VoiceId::Kimberly)
         {
             m_drivers = (driverFactory ? driverFactory : DefaultPCMOutputDriverFactoryInitFn())->LoadDrivers();
         }

aws-cpp-sdk-transfer-tests/TransferTests.cpp

@@ -160,9 +160,12 @@ class MockS3Client : public S3Client
 {
 public:
     MockS3Client(const Aws::Client::ClientConfiguration& clientConfiguration = Aws::Client::ClientConfiguration()):
-        S3Client(clientConfiguration), listObjectsV2RequestCount(0) 
+        S3Client(clientConfiguration), executor(clientConfiguration.executor), listObjectsV2RequestCount(0) 
+    {}
+
+    ~MockS3Client() 
     {
-        executor = Aws::MakeShared<Aws::Utils::Threading::PooledThreadExecutor>(ALLOCATION_TAG, 4);
+        executor = nullptr;
     }
 
     // Override this function to do verification.
@@ -197,8 +200,13 @@ class TransferTests : public ::testing::Test
         m_executor = Aws::MakeShared<Aws::Utils::Threading::PooledThreadExecutor>(ALLOCATION_TAG, 4);
     }
 
-protected:
+    void TearDown()
+    {
+        m_executor = nullptr;
+    }
 
+protected:
+    // Executor used by transferManager
     std::shared_ptr<Aws::Utils::Threading::Executor> m_executor;
 
     static Aws::String GetTestBucketName()
@@ -311,7 +319,8 @@ class TransferTests : public ::testing::Test
         config.scheme = Scheme::HTTP;
         config.connectTimeoutMs = 3000;
         config.requestTimeoutMs = 60000;
-
+        // executor used for s3Client
+        config.executor = Aws::MakeShared<Aws::Utils::Threading::PooledThreadExecutor>(ALLOCATION_TAG, 5);
         m_s3Client = Aws::MakeShared<MockS3Client>(ALLOCATION_TAG, config);
 
         DeleteBucket(GetTestBucketName());
@@ -1109,6 +1118,7 @@ TEST_F(TransferTests, TransferManager_CancelAndRetryUploadTest)
     bool completedPartsStayedCompletedDuringRetry = true;
     bool completionCheckDone = false;
     const char uuid[] = "Bjarne Stroustrup!";
+    std::atomic<bool> cancelHasBeenCalled(false);
 
     TransferManagerConfiguration transferManagerConfig(m_executor.get());
     transferManagerConfig.transferStatusUpdatedCallback = 
@@ -1119,8 +1129,8 @@ TEST_F(TransferTests, TransferManager_CancelAndRetryUploadTest)
                 ASSERT_NE(nullptr, handle->GetContext());
                 ASSERT_STREQ(uuid, handle->GetContext()->GetUUID().c_str());
             }
-
-            if (!retryInProgress && handle->GetCompletedParts().size() >= 15 &&  handle->GetStatus() != TransferStatus::CANCELED)
+            bool expected = false;
+            if (handle->GetCompletedParts().size() >= 15 && cancelHasBeenCalled.compare_exchange_strong(expected, true))
             {
                 std::const_pointer_cast<TransferHandle>(handle)->Cancel();
             }
@@ -1225,14 +1235,16 @@ TEST_F(TransferTests, TransferManager_AbortAndRetryUploadTest)
     bool retryInProgress = false;
     bool completedPartsStayedCompletedDuringRetry = true;
     bool completionCheckDone = false;
+    std::atomic<bool> cancelHasBeenCalled(false);
 
     std::shared_ptr<TransferHandle> requestPtr(nullptr);
 
     TransferManagerConfiguration transferManagerConfig(m_executor.get());
     transferManagerConfig.transferStatusUpdatedCallback =
         [&](const TransferManager* manager, const std::shared_ptr<const TransferHandle>& handle)
         {
-            if (!retryInProgress && handle->GetCompletedParts().size() >= 15 && handle->GetStatus() != TransferStatus::CANCELED)
+            bool expected = false;
+            if (handle->GetCompletedParts().size() >= 15 && cancelHasBeenCalled.compare_exchange_strong(expected, true))
             {
                 const_cast<TransferManager*>(manager)->AbortMultipartUpload(std::const_pointer_cast<TransferHandle>(handle));
             }
@@ -1273,6 +1285,15 @@ TEST_F(TransferTests, TransferManager_AbortAndRetryUploadTest)
     ListMultipartUploadsOutcome listMultipartOutcome = m_s3Client->ListMultipartUploads(listMultipartRequest);
 
     EXPECT_TRUE(listMultipartOutcome.IsSuccess());
+    // S3 has eventual consistency, even thought we called AbortMultiPartUpload and get successful return,
+    // following call of listMultiPartUpload will not gurantee to return 0.
+    size_t retries = 0;
+    while (listMultipartOutcome.GetResult().GetUploads().size() != 0u && retries++ < 5)
+    {
+        std::this_thread::sleep_for(std::chrono::seconds(1));
+        listMultipartOutcome = m_s3Client->ListMultipartUploads(listMultipartRequest);
+        EXPECT_TRUE(listMultipartOutcome.IsSuccess());
+    }
     ASSERT_EQ(0u, listMultipartOutcome.GetResult().GetUploads().size());
 
     HeadObjectRequest headObjectRequest;
@@ -1287,7 +1308,7 @@ TEST_F(TransferTests, TransferManager_AbortAndRetryUploadTest)
     ASSERT_NE(requestPtr, tempPtr);
     requestPtr->WaitUntilFinished();
 
-    size_t retries = 0;
+    retries = 0;
     //just make sure we don't fail because an upload part failed. (e.g. network problems or interuptions)
     while (requestPtr->GetStatus() == TransferStatus::FAILED && retries++ < 5)
     {
@@ -1540,6 +1561,7 @@ TEST_F(TransferTests, TransferManager_CancelAndRetryDownloadTest)
         bool completedPartsStayedCompletedDuringRetry = true;
         bool completionCheckDone = false;
         const char uuid[] = "Bjarne Stroustrup!";
+        std::atomic<bool> cancelHasBeenCalled(false);
 
         TransferManagerConfiguration downloadConfig(m_executor.get());
         downloadConfig.s3Client = m_s3Client;
@@ -1550,7 +1572,8 @@ TEST_F(TransferTests, TransferManager_CancelAndRetryDownloadTest)
                 ASSERT_STREQ(uuid, handle->GetContext()->GetUUID().c_str());
 
                 ASSERT_EQ(downloadFileName, handle->GetTargetFilePath());
-                if (!retryInProgress && handle->GetCompletedParts().size() >= 15 &&  handle->GetStatus() != TransferStatus::CANCELED)
+                bool expected = false;
+                if (handle->GetCompletedParts().size() >= 15u && cancelHasBeenCalled.compare_exchange_strong(expected, true))
                 {
                     std::const_pointer_cast<TransferHandle>(handle)->Cancel();
                 }
@@ -1578,7 +1601,12 @@ TEST_F(TransferTests, TransferManager_CancelAndRetryDownloadTest)
             requestPtr->WaitUntilFinished();
         }
 
-        ASSERT_EQ(TransferStatus::CANCELED, requestPtr->GetStatus());    
+        // call Cancel() in TransferStatusUpdateCallback function will not set status to CANCELED immediately.
+        // It only set up m_cancel to true, status will be updated by following UpdateStatus() call.
+        while (requestPtr->GetStatus() != TransferStatus::CANCELED) 
+        {
+            std::this_thread::sleep_for(std::chrono::seconds(1));
+        }
         ASSERT_TRUE(15u <= requestPtr->GetCompletedParts().size()); 
         ASSERT_EQ(0u, requestPtr->GetPendingParts().size());
         ASSERT_TRUE(15u >= requestPtr->GetFailedParts().size()); //some may have been in flight at cancelation time.