Skip to content

Fix S3 CRT async request lifetime - #3944

Open
FranciscoMaxwell wants to merge 1 commit into
aws:mainfrom
FranciscoMaxwell:fix-s3crt-request-lifetime
Open

FranciscoMaxwell wants to merge 1 commit into
aws:mainfrom
FranciscoMaxwell:fix-s3crt-request-lifetime

Conversation

@FranciscoMaxwell

Copy link
Copy Markdown

Issue #, if available:
Fixes #3881

Description of changes:
This updates the S3 CRT async callback state to keep the original service request alive until the CRT shutdown callback runs.

Previously, CrtRequestCallbackUserData stored originalRequest as a raw pointer to the request reference passed into CopyObjectAsync, GetObjectAsync, and PutObjectAsync. Those async methods can return before the CRT callbacks finish, leaving headers/body/progress/shutdown callbacks with a dangling pointer when they access GetContinueRequestHandler() or pass the typed request to the user response handler.

The callback data now owns a shared_ptr copy of the request. InitCommonCrtRequestOption receives and stores that shared_ptr, and the shutdown callbacks use the owned request when invoking the user handler.

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (No new unit test was added; this lifetime issue depends on the async CRT request lifecycle. I verified the affected target builds successfully.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

Testing:

  • git diff --check
  • cmake -S . -B build-s3crt -DBUILD_ONLY=s3-crt -DENABLE_TESTING=OFF -DAUTORUN_UNIT_TESTS=OFF
  • cmake --build build-s3crt --config Release --target aws-cpp-sdk-s3-crt -- /m

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@sbiscigl

Copy link
Copy Markdown
Collaborator

looks good to me outside two things:

  • i left a comment about using a unique pointer instead of shared
  • you only apply the on the CopyObject operation and none of the other bound out aws-c-s3 funcitons. you can fix this by editing the templates that generate the client code.

beyond those two things i am good to merge this barring anything that may turn up in CI

CopyObjectResponseReceivedHandler copyResponseHandler;
std::shared_ptr<const Aws::Client::AsyncCallerContext> asyncCallerContext;
const Aws::AmazonWebServiceRequest* originalRequest;
std::shared_ptr<const Aws::AmazonWebServiceRequest> originalRequest;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be a shared_ptr? since we are purely life time extending it why not use Aws::UniquePtr like we do for checksumConfig?

@FranciscoMaxwell
FranciscoMaxwell force-pushed the fix-s3crt-request-lifetime branch from f757fbb to abf744c Compare September 25, 2026 18:37
@FranciscoMaxwell

Copy link
Copy Markdown
Author

Thanks for the feedback. I have updated the PR accordingly.

  • Replaced the request ownership in CrtRequestCallbackUserData and InitCommonCrtRequestOption with Aws::UniquePtr<Aws::AmazonWebServiceRequest>. A single callback context owns the request for its full lifetime, so shared ownership is unnecessary. The callback still exposes the request as a const view.
  • Kept the UniquePtr element type non-const because the AWS allocator/deleter performs a polymorphic dynamic_cast<void*> during destruction; a const element type does not compile on MSVC.
  • Applied the ownership transfer in both S3 CRT generation templates (regular and Smithy), so generated S3 CRT async operations consistently retain their request instead of limiting the change to CopyObject. The checked-in generated S3 CRT client was updated to match.

Validated with:
cmake --build build-s3crt --config Release --target aws-cpp-sdk-s3-crt -- /m:1

The updated signed commit is abf744c82.

@sbiscigl sbiscigl mentioned this pull request Sep 25, 2026
11 tasks
@sbiscigl

Copy link
Copy Markdown
Collaborator

due to process reasons I created a PR with your commit on it, we will merge it from there

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SIGSEGV in S3CrtRequestHeadersCallback — use-after-free reading ContinueRequestHandler from destroyed request

2 participants