s2n_random.c: memory leaks of per-thread rand state #3525
Comments
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).
|
Thanks for reaching out! I think I'm missing something here. Are you / aws-c-io not calling s2n_cleanup from each thread? Or are you saying that allocations that should be cleaned up in s2n_rand_cleanup_thread are actually only being cleaned up in s2n_rand_cleanup? |
|
I understand the intention, that The current reality is that I have included a trace with |
|
I agree with the notion that if S2N allocates something in thread local storage, that it should register a thread exit handler to clean that up. It should not be the responsibility of the consumer in this case. |
|
I recently made changes in this area (#3512) and it was suggested (in #3446) that reference counting the calls to s2n_init and s2n_cleanup would allow the library to clean up better. I can see in that case, it would be possible for a different thread to call s2n_cleanup than the one that calls s2n_init and that would become acceptable, whereas with now s2n requires the main thread to be used in cleanup to actually cleanup. Would that be a better solution? |
|
Per current documentation, each consumer using S2N needs to call s2n_cleanup on each thread that uses S2N. The first thread that calls s2n_init is considered to be the "main thread" (whether it is the originating process thread is irrelevant). It performs global initialization, and also performs thread local storage initialization (stuff for rand gen). Each subsequent thread that needs it will also allocate thread local storage. The consumer is responsible for ensuring s2n_cleanup is called on every thread. When s2n_cleanup is called on any thread but the first thread that called s2n_init (i.e., the "main thread"), thread local storage is cleaned up. When s2n_cleanup is called on the main thread, both thread local storage and global state are cleaned up. There's no requirement that these happen in any sort of order, they simply all need to happen. Your issue does not sound like it is an S2N library issue, as S2N makes no attempt to handle cleanup on its own. Whatever is consuming the S2N library is not honoring the current API contract, if it calls a single s2n_init on one thread and then calls a single s2n_cleanup on another thread per your example above. Perhaps this issue needs to be taken up with aws_c_io, aws-cpp-sdk core, or perhaps your code is not using the same thread to initialize and clean up the SDK? For my statement one post up about reference counting, that is unnecessary given the current behavior. For my statement two posts up about the responsibility, I think it would be better if S2N used a thread exit handler to clean up the drbg that is creates in thread local storage from an API contract perspective, removing the burden from the consumer. However that's not what's there today. |
|
@jeking3 - I traced this all the way back to the AWS C++ SDK where init/clean-up is done via Aws::InitAPI 140004202379008
# ... application code running
Aws::ShutdownAPI 140004272230400Both As a consequence, the same constraints ("s2n_init/s2n_cleanup must be called from exactly the same thread") also hold true for We ran into this in an application that initializes the SDK only if cloud calls are needed. As a consequence, we can not do dynamic initialization, due to the s2n constraint. Question: is it at all possible to register a thread-exit cleanup handler to prevent external users (C++ SDK in this case) from running into this issue? |
…be called from the same thread Running InitAPI() and ShutdownAPI() from different threads can cause subtle problems, see e.g. - aws/s2n-tls#3525 - awslabs/aws-c-common#891 Add a note for users to use the same thread.
…be called from the same thread Running InitAPI() and ShutdownAPI() from different threads can cause subtle problems, see e.g. - aws/s2n-tls#3525 - awslabs/aws-c-common#891 Add a note for users to use the same thread.
…be called from the same thread Running InitAPI() and ShutdownAPI() from different threads can cause subtle problems, see e.g. - aws/s2n-tls#3525 - awslabs/aws-c-common#891 Add a note for users to use the same thread.
Problem description
s2nassigns allocated memory to a per-thread data structure, which is not cleaned up by the thread that created it.We discovered this issue (reproducible on each run) when using ASAN/LeakSanitizer.
Backtrace
Analysis
These 4 allocations happen on the following per-thread structure:
The first 2 allocations of 152B each (for
public/private_drbg) allocate thectx:The remaining 2 allocations result from initializations:
A de-allocation function exists:
The problem is that the allocation is per-thread, while the de-allocation is only in the main thread:
Similar for
aws-c-io/source/io.c:aws_io_library_clean_up() => aws_tls_clean_up_static_state():Where
s2n_cleanupisConclusion
The allocations to
s2n_per_thread_rand_stateare assigned to the thread-local storage wheres2n_init_drbgs/s2n_ensure_initialized_drbgsare run. The corresponding clean-up happens from a different (main) thread, hence do not free the original allocations.To illustrate, here is a call trace where the value of
pthread_self()is printed (from the above run):The text was updated successfully, but these errors were encountered: