S3CrtClient::GetObjectAsync Doesn't copy handler

[mgraczyk]

Confirm by changing [ ] to [x] below to ensure that it's a bug:

• I've gone though Developer Guide and API reference
• I've searched for previous similar issues and didn't find any solution

Describe the bug
The API for the new S3CrtClient::GetObjectAsync function looks like the old API S3Client::GetObjectAsync, but unlike the old API the new one does not copy the handler. This should either be changed or documented because code cannot safely switch from old to new API without potentially hidden bugs.

Example code that is buggy:

void MyFunction() {
  std::string local_thing;
  auto callback = [local_thing](...) {
     ... use local_thing
  };
  s3_crt_client.GetObjectAsync(..., callback, ...);
}

The captured variable local_think could be accessed after its destructor is called because GetObjectAsync takes a pointer rather than a copy of the handler:

userData->userCallback = static_cast<const void*>(&handler);

What's the point of taking std::function as input if the caller still has to manage the lifetime of the handler?

Thanks for reading, let me know if it would be helpful for me to provide additional information.

SDK version number
1.9.22

Platform/OS/Hardware/Device
What are you running the sdk on?
Linux

To Reproduce (observed behavior)
Steps to reproduce the behavior (please share code)

Something like the example code.

Expected behavior
Either the difference between new and old API should be documented, or the new API should manage ownership of the handler, or the new API should accept a function pointer instead of std::function

Additional context
The same problem exists with PutObjectAsync, but not with other methods that still like the thread executor (like CompleteMultipartUploadAsync)
Thanks for reading!

[KaibaLopez]

Hi @mgraczyk ,
Thanks for pointing this out to us.
Yea this makes sense and it's something we'd have to take a closer look at... at the very least put a comment there to warn users of the change in behavior.
In the mean time, could you provide sample code of the situation where you found the problem? Like a minimal working sample on say putObjectAsync and the error it caused. I feel that would be the easiest and clearest way to see the issue.

[mgraczyk]

Here's a minimal repro.

This causes SIGSEGV for me 100% of the time

#include <aws/core/Aws.h>
#include <unistd.h>

#include <sstream>
#include <thread>

#define USE_AWS_CRT 1

#if USE_AWS_CRT == 1
#include "aws/s3-crt/S3CrtClient.h"
#include "aws/s3-crt/model/PutObjectRequest.h"
using S3Client = Aws::S3Crt::S3CrtClient;
namespace s3 = Aws::S3Crt;
using ClientConfiguration = s3::ClientConfiguration;
#else
#include "aws/s3/S3Client.h"
#include "aws/s3/model/PutObjectRequest.h"
namespace s3 = Aws::S3;
using S3Client = Aws::S3::S3Client;
using ClientConfiguration = Aws::Client::ClientConfiguration;
#endif

void Callback(
    const S3Client* s3_client,
    const s3::Model::PutObjectRequest& request,
    const s3::Model::PutObjectOutcome& outcome,
    const std::shared_ptr<const Aws::Client::AsyncCallerContext>& context)
{
}

void DoPutObject(S3Client* s3_client)
{
    auto request =
        s3::Model::PutObjectRequest().WithBucket("my-bucket").WithKey("putobject-example-test-key");
    request.SetBody(std::make_shared<std::stringstream>("test"));
    s3_client->PutObjectAsync(request, Callback, nullptr);
}

int main()
{
    Aws::InitAPI(Aws::SDKOptions{});
    S3Client s3_client(ClientConfiguration{});

    std::thread(DoPutObject, &s3_client).join();

    std::cout << "waiting" << std::endl;
    usleep(2000 * 1000);
    std::cout << "done waiting" << std::endl;

    Aws::ShutdownAPI(Aws::SDKOptions{});
}

Outcome GDB:

Thread 2 "test" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7ffff6b8a700 (LWP 228623)]
...
(gdb) where
#0  0x0000000000000050 in ?? ()
#1  0x000055555567b399 in PutObjectRequestShutdownCallback(void*) ()
#2  0x0000555555a3bf1f in s_s3_meta_request_destroy ()
#3  0x0000555555b282c9 in aws_ref_count_release ()
#4  0x0000555555a38aa6 in aws_s3_client_update_meta_requests_threaded ()
#5  0x0000555555a39741 in s_s3_client_process_work_default ()
#6  0x0000555555b28a82 in s_run_all ()
#7  0x0000555555a83914 in s_main_loop ()
#8  0x0000555555b27002 in thread_fn ()
#9  0x00007ffff7f11609 in start_thread (arg=<optimized out>) at pthread_create.c:477
#10 0x00007ffff7e38293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

If I change #define USE_AWS_CRT 0, it always works.

[mgraczyk]

Notice that you don't even need a lambda for this, the problem is actually much worse. The code takes the address of a temporary std::function object, which clears it's pointer during destruction.

[wmorgan6796]

@KaibaLopez Whats the status of the PR linked to this issue? I've been facing this issue for a few days and it breaks with expectations, especially if we're doing what I'm doing (moving from S3Client -> S3CrtClient).