Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s2n_drbg: de-allocate thread-local allocations at thread exit #3530

Closed
wants to merge 5 commits into from

Conversation

grrtrr
Copy link
Contributor

@grrtrr grrtrr commented Oct 3, 2022

s2n_init_drbgs allocates, via s2n_drbg_instantiate, dynamic memory to fields of a static, thread-local struct. For this reason, the de-allocation of these fields should be done by the same thread that created them. Else the allocations will leak.

Hence track these allocations as thread-specific data, using an allocation key and a destructor function called at thread exit.

Resolved issues: Resolves #3525.

Description of changes:

Ensure that the same thread that dynamically allocates memory to fields of the thread-local storage allocated by
s2n_drbg_instantiate (and the functions it calls) is de-allocated by the same thread.

Currently the de-allocation happens from another thread, which does not help.

Call-outs:

Please revise the use of s2n_rand_cleanup_thread with regard to s2n_drbg_wipe. The drbg de-allocation could
happen entirely in the destructor. That would render s2n_rand_cleanup_thread either empty, or it could be used
for other clean-up tasks.

Testing:

Ran the test suite. Ran tests with LSAN (before/after) to ensure that the leaks have been fixed.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

s2n_init_drbgs allocates, via s2n_drbg_instantiate, dynamic memory to fields of a static,
thread-local struct. For this reason, the de-allocation/clean-up of these fields can only
happen through the same thread that created them. Else the allocations will leak.

Hence track these allocations as thread-specific data, using an allocation key and a
destructor function that gets called at thread exit.

This resolves aws#3525 (tested via LSAN sanitizer).
@grrtrr grrtrr requested a review from a team as a code owner October 3, 2022 13:52
@zaherd zaherd requested a review from torben-hansen October 3, 2022 15:20
@dougch
Copy link
Contributor

dougch commented Oct 3, 2022

Thanks for the submission, we'll take a look.

Copy link
Contributor

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

Can you include a test the reproduces and then fixes the memory leak that you're describing?

@grrtrr
Copy link
Contributor Author

grrtrr commented Oct 4, 2022

@lrstewart - I added an asan test in 654ba7e, which reproduces the issue if the following line is commented out:

--- a/utils/s2n_random.c
+++ b/utils/s2n_random.c
@@ -136,7 +136,7 @@ S2N_RESULT s2n_get_mix_entropy(struct s2n_blob *blob)
 
 static void s2n_drbg_destructor(void *_unused_argument) {
     (void)_unused_argument;
-    s2n_result_ignore(s2n_rand_cleanup_thread());
+    //s2n_result_ignore(s2n_rand_cleanup_thread());
 }
 
 static void s2n_drbg_make_alloc_key(void)

@grrtrr grrtrr requested review from lrstewart and removed request for torben-hansen October 7, 2022 16:08
@grrtrr
Copy link
Contributor Author

grrtrr commented Dec 31, 2022

Condition can be fixed in aws-sdk-cpp use of s2n-tls

By allocating a dedicated AWS API initialization / shutdown thread,

    std::thread sdk_initializer_thread_;
    std::promise<void> initialized_;
    std::promise<void> shutdown_;

the problem can be avoided in the main SDK (aws-sdk-cpp), with a thread like this:

    sdk_initializer_thread_ = std::thread{[this]() {
        Aws::InitAPI(aws_options_);
        initialized_.set_value();  // Signal caller that AWS SDK is initialized

        shutdown_.get_future().wait(); // Block until receiving the shutdown signal
        Aws::ShutdownAPI(aws_options_);
    }};
    initialized_.get_future().wait(); // Block until SDK is intialized

To trigger API shutdown:

AWSInit::~AWSInit() {
    // Signal the AWS-SDK-initializer thread to call Aws::ShutdownAPI.
    shutdown_.set_value();
    sdk_initializer_thread_.join();
}

The main SDK may need to do this for other reasons (e.g. non-thread-safe aws-c-common API, awslabs/aws-c-common#891 (comment)).

In any way, the above code has worked for us to avoid the memory leak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

s2n_random.c: memory leaks of per-thread rand state
3 participants