diff --git a/.changelog/bugfix-pr3944.json b/.changelog/bugfix-pr3944.json new file mode 100644 index 000000000000..d6a97d5dbd06 --- /dev/null +++ b/.changelog/bugfix-pr3944.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "aws-cpp-sdk-s3-crt", + "contributor": "FranciscoMaxwell", + "description": "Fix use-after-free in S3 CRT async operations by owning a request copy" +} diff --git a/generated/src/aws-cpp-sdk-s3-crt/include/aws/s3-crt/S3CrtClient.h b/generated/src/aws-cpp-sdk-s3-crt/include/aws/s3-crt/S3CrtClient.h index 84c40db94b7a..06bb1159688f 100644 --- a/generated/src/aws-cpp-sdk-s3-crt/include/aws/s3-crt/S3CrtClient.h +++ b/generated/src/aws-cpp-sdk-s3-crt/include/aws/s3-crt/S3CrtClient.h @@ -8739,7 +8739,7 @@ class AWS_S3CRT_API S3CrtClient : public Aws::Client::AWSXMLClient, PutObjectResponseReceivedHandler putResponseHandler; CopyObjectResponseReceivedHandler copyResponseHandler; std::shared_ptr asyncCallerContext; - const Aws::AmazonWebServiceRequest* originalRequest; + Aws::UniquePtr originalRequest; std::shared_ptr request; std::shared_ptr response; std::shared_ptr crtHttpRequest; @@ -8778,7 +8778,7 @@ class AWS_S3CRT_API S3CrtClient : public Aws::Client::AWSXMLClient, void InitCrtEndpointFromUri(aws_uri& endpoint_uri, const Aws::Http::URI& uri) const; void InitCommonCrtRequestOption(CrtRequestCallbackUserData* userData, aws_s3_meta_request_options* options, - const Aws::AmazonWebServiceRequest* request, const Aws::Http::URI& uri, + Aws::UniquePtr request, const Aws::Http::URI& uri, Aws::Http::HttpMethod method) const; using CopyObjectPropertiesOutcome = Aws::Utils::Outcome, S3CrtError>; diff --git a/generated/src/aws-cpp-sdk-s3-crt/source/S3CrtClient.cpp b/generated/src/aws-cpp-sdk-s3-crt/source/S3CrtClient.cpp index 5186361b14ae..1d2c1e148f43 100644 --- a/generated/src/aws-cpp-sdk-s3-crt/source/S3CrtClient.cpp +++ b/generated/src/aws-cpp-sdk-s3-crt/source/S3CrtClient.cpp @@ -799,7 +799,7 @@ void S3CrtClient::InitCrtEndpointFromUri(aws_uri& endpoint_uri, const Aws::Http: } void S3CrtClient::InitCommonCrtRequestOption(CrtRequestCallbackUserData* userData, aws_s3_meta_request_options* options, - const Aws::AmazonWebServiceRequest* request, const Aws::Http::URI& uri, + Aws::UniquePtr request, const Aws::Http::URI& uri, Aws::Http::HttpMethod method) const { std::shared_ptr httpRequest(nullptr); if (request) { @@ -812,7 +812,7 @@ void S3CrtClient::InitCommonCrtRequestOption(CrtRequestCallbackUserData* userDat std::shared_ptr httpResponse = Aws::MakeShared(ALLOCATION_TAG, httpRequest); userData->s3CrtClient = this; - userData->originalRequest = request; + userData->originalRequest = std::move(request); userData->request = httpRequest; userData->response = httpResponse; @@ -1000,7 +1000,7 @@ static void CopyObjectRequestShutdownCallback(void* user_data) { userData->asyncCallerContext->GetMonitorContext().OnRequestSucceeded(userData->request, userData->response); } } - userData->copyResponseHandler(userData->s3CrtClient, *(reinterpret_cast(userData->originalRequest)), + userData->copyResponseHandler(userData->s3CrtClient, *(static_cast(userData->originalRequest.get())), std::move(outcome), userData->asyncCallerContext); Aws::Delete(userData); @@ -1071,7 +1071,9 @@ void S3CrtClient::CopyObjectAsync(const CopyObjectRequest& request, const CopyOb if (handlerContext) { userData->asyncCallerContext = handlerContext; } - InitCommonCrtRequestOption(userData, &options, &request, endpointResolutionOutcome.GetResult().GetURI(), Aws::Http::HttpMethod::HTTP_PUT); + auto requestCopy = Aws::MakeUnique(ALLOCATION_TAG, request); + InitCommonCrtRequestOption(userData, &options, std::move(requestCopy), endpointResolutionOutcome.GetResult().GetURI(), + Aws::Http::HttpMethod::HTTP_PUT); if (userData != nullptr && userData->request != nullptr && userData->request->GetContentBody() != nullptr && userData->request->GetContentBody()->fail()) { return handler(this, request, @@ -1245,7 +1247,7 @@ static void GetObjectRequestShutdownCallback(void* user_data) { userData->asyncCallerContext->GetMonitorContext().OnRequestSucceeded(userData->request, userData->response); } } - userData->getResponseHandler(userData->s3CrtClient, *(reinterpret_cast(userData->originalRequest)), + userData->getResponseHandler(userData->s3CrtClient, *(static_cast(userData->originalRequest.get())), std::move(outcome), userData->asyncCallerContext); Aws::Delete(userData); @@ -1309,7 +1311,9 @@ void S3CrtClient::GetObjectAsync(const GetObjectRequest& request, const GetObjec if (handlerContext) { userData->asyncCallerContext = handlerContext; } - InitCommonCrtRequestOption(userData, &options, &request, endpointResolutionOutcome.GetResult().GetURI(), Aws::Http::HttpMethod::HTTP_GET); + auto requestCopy = Aws::MakeUnique(ALLOCATION_TAG, request); + InitCommonCrtRequestOption(userData, &options, std::move(requestCopy), endpointResolutionOutcome.GetResult().GetURI(), + Aws::Http::HttpMethod::HTTP_GET); if (userData != nullptr && userData->request != nullptr && userData->request->GetContentBody() != nullptr && userData->request->GetContentBody()->fail()) { return handler(this, request, @@ -1406,7 +1410,7 @@ static void PutObjectRequestShutdownCallback(void* user_data) { userData->asyncCallerContext->GetMonitorContext().OnRequestSucceeded(userData->request, userData->response); } } - userData->putResponseHandler(userData->s3CrtClient, *(reinterpret_cast(userData->originalRequest)), + userData->putResponseHandler(userData->s3CrtClient, *(static_cast(userData->originalRequest.get())), std::move(outcome), userData->asyncCallerContext); Aws::Delete(userData); @@ -1470,7 +1474,9 @@ void S3CrtClient::PutObjectAsync(const PutObjectRequest& request, const PutObjec if (handlerContext) { userData->asyncCallerContext = handlerContext; } - InitCommonCrtRequestOption(userData, &options, &request, endpointResolutionOutcome.GetResult().GetURI(), Aws::Http::HttpMethod::HTTP_PUT); + auto requestCopy = Aws::MakeUnique(ALLOCATION_TAG, request); + InitCommonCrtRequestOption(userData, &options, std::move(requestCopy), endpointResolutionOutcome.GetResult().GetURI(), + Aws::Http::HttpMethod::HTTP_PUT); if (userData != nullptr && userData->request != nullptr && userData->request->GetContentBody() != nullptr && userData->request->GetContentBody()->fail()) { return handler(this, request, diff --git a/tests/aws-cpp-sdk-s3-crt-integration-tests/BucketAndObjectOperationTest.cpp b/tests/aws-cpp-sdk-s3-crt-integration-tests/BucketAndObjectOperationTest.cpp index d5102aaf693d..82d35db00226 100644 --- a/tests/aws-cpp-sdk-s3-crt-integration-tests/BucketAndObjectOperationTest.cpp +++ b/tests/aws-cpp-sdk-s3-crt-integration-tests/BucketAndObjectOperationTest.cpp @@ -56,6 +56,7 @@ #include #include #include +#include #ifdef _WIN32 #pragma warning(disable: 4127) @@ -1533,6 +1534,70 @@ namespace AWS_ASSERT_SUCCESS(putObjectOutcome); } + TEST_F(BucketAndObjectOperationTest, TestAsyncRequestsOutliveCallerRequest) + { + const Aws::String fullBucketName = CalculateBucketName(BASE_PUT_OBJECTS_BUCKET_NAME.c_str()); + SCOPED_TRACE(Aws::String("FullBucketName ") + fullBucketName); + CreateBucketRequest createBucketRequest; + createBucketRequest.SetBucket(fullBucketName); + createBucketRequest.SetACL(BucketCannedACL::private_); + + CreateBucketOutcome createBucketOutcome = Client->CreateBucket(createBucketRequest); + AWS_ASSERT_SUCCESS(createBucketOutcome); + ASSERT_TRUE(WaitForBucketToPropagate(fullBucketName)); + TagTestBucket(fullBucketName, Client); + + const Aws::String key = "async-lifetime"; + const Aws::String copyKey = "async-lifetime-copy"; + + std::promise> putPromise; + { + PutObjectRequest putObjectRequest; + putObjectRequest.SetBucket(fullBucketName); + putObjectRequest.SetKey(key); + putObjectRequest.SetBody(Aws::MakeShared(ALLOCATION_TAG, "Test Object")); + Client->PutObjectAsync(putObjectRequest, + [&putPromise](const S3CrtClient*, const PutObjectRequest& request, const PutObjectOutcome& outcome, + const std::shared_ptr&) { + putPromise.set_value({request.GetKey(), outcome}); + }); + } + auto putResult = putPromise.get_future().get(); + AWS_ASSERT_SUCCESS(putResult.second); + EXPECT_EQ(key, putResult.first); + + std::promise> getPromise; + { + GetObjectRequest getObjectRequest; + getObjectRequest.SetBucket(fullBucketName); + getObjectRequest.SetKey(key); + Client->GetObjectAsync(getObjectRequest, + [&getPromise](const S3CrtClient*, const GetObjectRequest& request, GetObjectOutcome outcome, + const std::shared_ptr&) { + getPromise.set_value({request.GetKey(), outcome.IsSuccess()}); + }); + } + auto getResult = getPromise.get_future().get(); + EXPECT_TRUE(getResult.second); + EXPECT_EQ(key, getResult.first); + + std::promise> copyPromise; + { + CopyObjectRequest copyObjectRequest; + copyObjectRequest.SetBucket(fullBucketName); + copyObjectRequest.SetKey(copyKey); + copyObjectRequest.SetCopySource(fullBucketName + "/" + key); + Client->CopyObjectAsync(copyObjectRequest, + [©Promise](const S3CrtClient*, const CopyObjectRequest& request, const CopyObjectOutcome& outcome, + const std::shared_ptr&) { + copyPromise.set_value({request.GetKey(), outcome}); + }); + } + auto copyResult = copyPromise.get_future().get(); + AWS_ASSERT_SUCCESS(copyResult.second); + EXPECT_EQ(copyKey, copyResult.first); + } + TEST_F(BucketAndObjectOperationTest, NoAuthPublicBucket) { Aws::S3Crt::ClientConfiguration s3ClientConfig; s3ClientConfig.region = Aws::Region::US_EAST_1; diff --git a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/S3ClientHeader.vm b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/S3ClientHeader.vm index a8b82fbf9a9f..2a382b08b8b6 100644 --- a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/S3ClientHeader.vm +++ b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/S3ClientHeader.vm @@ -214,7 +214,7 @@ namespace ${rootNamespace} PutObjectResponseReceivedHandler putResponseHandler; CopyObjectResponseReceivedHandler copyResponseHandler; std::shared_ptr asyncCallerContext; - const Aws::AmazonWebServiceRequest *originalRequest; + Aws::UniquePtr originalRequest; std::shared_ptr request; std::shared_ptr response; std::shared_ptr crtHttpRequest; @@ -252,7 +252,7 @@ namespace ${rootNamespace} void InitCommonCrtRequestOption(CrtRequestCallbackUserData *userData, aws_s3_meta_request_options *options, - const Aws::AmazonWebServiceRequest *request, + Aws::UniquePtr request, const Aws::Http::URI &uri, Aws::Http::HttpMethod method) const; using CopyObjectPropertiesOutcome = Aws::Utils::Outcome, S3CrtError>; diff --git a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/SmithyS3ClientHeader.vm b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/SmithyS3ClientHeader.vm index 7f00421a0e67..a3e88f4f52fd 100644 --- a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/SmithyS3ClientHeader.vm +++ b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/SmithyS3ClientHeader.vm @@ -230,7 +230,7 @@ namespace ${rootNamespace} PutObjectResponseReceivedHandler putResponseHandler; CopyObjectResponseReceivedHandler copyResponseHandler; std::shared_ptr asyncCallerContext; - const Aws::AmazonWebServiceRequest *originalRequest; + Aws::UniquePtr originalRequest; std::shared_ptr request; std::shared_ptr response; std::shared_ptr crtHttpRequest; @@ -264,7 +264,7 @@ namespace ${rootNamespace} void InitCommonCrtRequestOption(CrtRequestCallbackUserData *userData, aws_s3_meta_request_options *options, - const Aws::AmazonWebServiceRequest *request, + Aws::UniquePtr request, const Aws::Http::URI &uri, Aws::Http::HttpMethod method) const; using CopyObjectPropertiesOutcome = Aws::Utils::Outcome, S3CrtError>; diff --git a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/S3CrtSpecificOperations.vm b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/S3CrtSpecificOperations.vm index 0d32c1a76e70..70d35a6391ae 100644 --- a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/S3CrtSpecificOperations.vm +++ b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/S3CrtSpecificOperations.vm @@ -277,7 +277,7 @@ void S3CrtClient::InitCrtEndpointFromUri(aws_uri &endpoint_uri, const Aws::Http: void S3CrtClient::InitCommonCrtRequestOption(CrtRequestCallbackUserData *userData, aws_s3_meta_request_options *options, - const Aws::AmazonWebServiceRequest *request, + Aws::UniquePtr request, const Aws::Http::URI &uri, Aws::Http::HttpMethod method) const { @@ -295,7 +295,7 @@ void S3CrtClient::InitCommonCrtRequestOption(CrtRequestCallbackUserData *userDat std::shared_ptr httpResponse = Aws::MakeShared(ALLOCATION_TAG, httpRequest); userData->s3CrtClient = this; - userData->originalRequest = request; + userData->originalRequest = std::move(request); userData->request = httpRequest; userData->response = httpResponse; @@ -512,11 +512,11 @@ static void ${operation.name}RequestShutdownCallback(void *user_data) } #if($operation.request) #if($operation.name == "PutObject") - userData->putResponseHandler(userData->s3CrtClient, *(reinterpret_cast(userData->originalRequest)), std::move(outcome), userData->asyncCallerContext); + userData->putResponseHandler(userData->s3CrtClient, *(static_cast(userData->originalRequest.get())), std::move(outcome), userData->asyncCallerContext); #elseif($operation.name == "GetObject") - userData->getResponseHandler(userData->s3CrtClient, *(reinterpret_cast(userData->originalRequest)), std::move(outcome), userData->asyncCallerContext); + userData->getResponseHandler(userData->s3CrtClient, *(static_cast(userData->originalRequest.get())), std::move(outcome), userData->asyncCallerContext); #elseif($operation.name == "CopyObject") - userData->copyResponseHandler(userData->s3CrtClient, *(reinterpret_cast(userData->originalRequest)), std::move(outcome), userData->asyncCallerContext); + userData->copyResponseHandler(userData->s3CrtClient, *(static_cast(userData->originalRequest.get())), std::move(outcome), userData->asyncCallerContext); #end #else (*handler)(userData->s3CrtClient, outcome, userData->userCallbackContext); @@ -594,7 +594,8 @@ void ${className}::${operation.name}Async(${constText}${operation.request.shape. userData->asyncCallerContext = handlerContext; } #if($serviceModel.endpointRules) - InitCommonCrtRequestOption(userData, &options, &request, endpointResolutionOutcome.GetResult().GetURI(), Aws::Http::HttpMethod::HTTP_${operation.http.method}); + auto requestCopy = Aws::MakeUnique<${operation.request.shape.name}>(ALLOCATION_TAG, request); + InitCommonCrtRequestOption(userData, &options, std::move(requestCopy), endpointResolutionOutcome.GetResult().GetURI(), Aws::Http::HttpMethod::HTTP_${operation.http.method}); if (userData != nullptr && userData->request != nullptr && userData->request->GetContentBody() != nullptr && @@ -609,7 +610,8 @@ void ${className}::${operation.name}Async(${constText}${operation.request.shape. #end } #else - InitCommonCrtRequestOption(userData, &options, &request, uri, Aws::Http::HttpMethod::HTTP_${operation.http.method}); + auto requestCopy = Aws::MakeUnique<${operation.request.shape.name}>(ALLOCATION_TAG, request); + InitCommonCrtRequestOption(userData, &options, std::move(requestCopy), uri, Aws::Http::HttpMethod::HTTP_${operation.http.method}); #end #if($operation.name == "CopyObject") { diff --git a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/SmithyS3CrtSpecificOperations.vm b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/SmithyS3CrtSpecificOperations.vm index c912bde856b0..3d802ae14975 100644 --- a/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/SmithyS3CrtSpecificOperations.vm +++ b/tools/code-generation/generator/src/main/resources/com/amazonaws/util/awsclientgenerator/velocity/cpp/s3/s3-crt/SmithyS3CrtSpecificOperations.vm @@ -277,7 +277,7 @@ void S3CrtClient::InitCrtEndpointFromUri(aws_uri &endpoint_uri, const Aws::Http: void S3CrtClient::InitCommonCrtRequestOption(CrtRequestCallbackUserData *userData, aws_s3_meta_request_options *options, - const Aws::AmazonWebServiceRequest *request, + Aws::UniquePtr request, const Aws::Http::URI &uri, Aws::Http::HttpMethod method) const { @@ -286,7 +286,7 @@ void S3CrtClient::InitCommonCrtRequestOption(CrtRequestCallbackUserData *userDat std::shared_ptr pRequestCtx = Aws::MakeShared(ALLOCATION_TAG); assert(pRequestCtx); - pRequestCtx->m_pRequest = request; + pRequestCtx->m_pRequest = request.get(); pRequestCtx->m_invocationId = Aws::Utils::UUID::PseudoRandomUUID(); pRequestCtx->m_requestInfo.attempt = 1; pRequestCtx->m_requestInfo.maxAttempts = 0; @@ -295,7 +295,7 @@ void S3CrtClient::InitCommonCrtRequestOption(CrtRequestCallbackUserData *userDat std::shared_ptr httpResponse = Aws::MakeShared(ALLOCATION_TAG, httpRequest); userData->s3CrtClient = this; - userData->originalRequest = request; + userData->originalRequest = std::move(request); userData->request = httpRequest; userData->response = httpResponse; @@ -512,11 +512,11 @@ static void ${operation.name}RequestShutdownCallback(void *user_data) } #if($operation.request) #if($operation.name == "PutObject") - userData->putResponseHandler(userData->s3CrtClient, *(reinterpret_cast(userData->originalRequest)), std::move(outcome), userData->asyncCallerContext); + userData->putResponseHandler(userData->s3CrtClient, *(static_cast(userData->originalRequest.get())), std::move(outcome), userData->asyncCallerContext); #elseif($operation.name == "GetObject") - userData->getResponseHandler(userData->s3CrtClient, *(reinterpret_cast(userData->originalRequest)), std::move(outcome), userData->asyncCallerContext); + userData->getResponseHandler(userData->s3CrtClient, *(static_cast(userData->originalRequest.get())), std::move(outcome), userData->asyncCallerContext); #elseif($operation.name == "CopyObject") - userData->copyResponseHandler(userData->s3CrtClient, *(reinterpret_cast(userData->originalRequest)), std::move(outcome), userData->asyncCallerContext); + userData->copyResponseHandler(userData->s3CrtClient, *(static_cast(userData->originalRequest.get())), std::move(outcome), userData->asyncCallerContext); #end #else (*handler)(userData->s3CrtClient, outcome, userData->userCallbackContext); @@ -612,7 +612,8 @@ void ${className}::${operation.name}Async(${constText}${operation.request.shape. userData->asyncCallerContext = handlerContext; } #if($serviceModel.endpointRules) - InitCommonCrtRequestOption(userData, &options, &request, endpointResolutionOutcome.GetResult().GetURI(), Aws::Http::HttpMethod::HTTP_${operation.http.method}); + auto requestCopy = Aws::MakeUnique<${operation.request.shape.name}>(ALLOCATION_TAG, request); + InitCommonCrtRequestOption(userData, &options, std::move(requestCopy), endpointResolutionOutcome.GetResult().GetURI(), Aws::Http::HttpMethod::HTTP_${operation.http.method}); if (userData != nullptr && userData->request != nullptr && userData->request->GetContentBody() != nullptr && @@ -627,7 +628,8 @@ void ${className}::${operation.name}Async(${constText}${operation.request.shape. #end } #else - InitCommonCrtRequestOption(userData, &options, &request, uri, Aws::Http::HttpMethod::HTTP_${operation.http.method}); + auto requestCopy = Aws::MakeUnique<${operation.request.shape.name}>(ALLOCATION_TAG, request); + InitCommonCrtRequestOption(userData, &options, std::move(requestCopy), uri, Aws::Http::HttpMethod::HTTP_${operation.http.method}); #end #if($operation.name == "CopyObject") {