ThreadLocalStorage leak in openssl 3.3.1 about openssl HOT 11 CLOSED

The test seems invalid to me.

From my read of the log you've attached, appverifier is trapping on the fact that your application called FreeLibrary prior to cleaning up any thread local storage in that library. Your code only cleans up Openssl storage (via OpenSSL_cleanup) only after FreeLibrary has been called

To properly do what you want to do, in OpenSSLTlsLeakExe.cpp, you would have to:

1. call LoadLibraryW as you do now
2. call a function like GetProcAddress to lookup the address of the Openssl_cleanup function and assign it to a local function pointer
3. Call the function pointer in (2) to cleanup openssl resources
4. Unload the library with FreeLibrary

That should clean up your memory leaks.

from openssl.

i think this is not correct. First as i wrote appverifier can only detect leaks in a subbinary, this is why i design the test app like this.

But in my dll i did all conform to openssl documentation:

In the event that the application will close in a manner that will not call the registered atexit() handlers then the application should call OPENSSL_cleanup() directly

My use case is pretty simple. I have one dll which uses OpenSSL which can be updated during runtime, so all dependencies of my dll i need to be able to unload. As the calling application is not under my control and should not know anything about my dll's internas my dll should do all the cleanup

But as mentioned i think when i do it the way you suggest (very bugbrone already in the design) you simply can't detect the leak. When i do it with like this ->

Executable loads dll1_withoud_openssl_dependency and calls a function there which then loads OpenSSLTlsLeakDll get the proc address of Openssl_cleanup() calls it and than free the library OpenSSLTlsLeakDll it is still the same

From my research

1. TlsAlloc in OPENSSL_init_crypto/ossl_init_thread (line 70 init.c)
   

2. TlsAlloc in OPENSSL_init_crypto/CRYPTO_THREAD_init_local (line 73 init.c)
   

3. free of 2) in OPENSSL_Cleanup/CRYPTO_THREAD_cleanup_local (line 391 init.c)
   

4. TlsAlloc in OPENSSL_init_crypto/ossl_config_modules_free/../ossl_init_thread_start(line 126 threads_win.c) <--- leaking allocation
   

5. free of 1) in OPENSSL_Cleanup/ossl_cleanup_thread (line 442 init.c)
   

So there is in the cleanup one allocation with TLSAlloc where no TLSFree is called

from openssl.

i tried now as you suggested, still exact the same steps (& also i tried with atexit handler) I have 3 TlsAlloc and only 2 TlsFree

from openssl.

Have you read this manual page, especially the NOTES part? I believe it is relevant.
https://www.openssl.org/docs/man3.0/man3/OPENSSL_thread_stop.html

from openssl.

To me it seems a real bug, even though minor. The allocation from the stack below is not freed:
OpenSSLTlsLeakDll.dll!CRYPTO_THREAD_init_local(unsigned long * key, void(*)(void *) cleanup) Line 511 C

OpenSSLTlsLeakDll.dll!ossl_rcu_init() Line 126 C
OpenSSLTlsLeakDll.dll!CRYPTO_THREAD_run_once(long * once, void()() init) Line 501 C
OpenSSLTlsLeakDll.dll!ossl_rcu_lock_new(int num_writers) Line 145 C
OpenSSLTlsLeakDll.dll!do_init_module_list_lock() Line 102 C
OpenSSLTlsLeakDll.dll!do_init_module_list_lock_ossl_() Line 100 C
OpenSSLTlsLeakDll.dll!CRYPTO_THREAD_run_once(long * once, void()() init) Line 501 C
OpenSSLTlsLeakDll.dll!conf_modules_finish_int() Line 568 C
OpenSSLTlsLeakDll.dll!CONF_modules_unload(int all) Line 514 C
OpenSSLTlsLeakDll.dll!ossl_config_modules_free() Line 623 C
OpenSSLTlsLeakDll.dll!OPENSSL_cleanup() Line 431 C

Probably because this is happening in the cleanup and at this point OPENSSL_thread_stop() was already called.

from openssl.

Have you read this manual page, especially the NOTES part? I believe it is relevant. https://www.openssl.org/docs/man3.0/man3/OPENSSL_thread_stop.html

i read the docu and i think i understood it correct and the code is also doing it according to the documentation (as i understood). Do you have a specific part what you think is relevant?

Also when i call openssl_thread_stop on (in this case main thread) i have the same issue. I think the problem really is the ordering of the TLSFree. Perhaps the code should dealloc one of the resources later and because it is still used it reallocates it and missed the free (only an assumption). Fact is when init is called and afterwards cleanup is called (does btw not matter if i call it or atexit) there are 3 TLSAlloc done and only 2 TLSFree.

so this simple app has the same TLS issue as my example

int main()
{
std::ignore = OPENSSL_init_crypto(OPENSSL_INIT_NO_LOAD_CONFIG, nullptr);
return 1;
}

The only difference is that AppVerifier can't detect it. When i debug the code i see also 3 TLSAlloc and 2 TLSFree => TLS leak of one allocation

from openssl.

Goodness, sorry, I see the problem. Its because we're running the rcu lock init function on cleanup

Because we've not done any actual work inside openssl (specifically we never load a conf file, ossl_rcu_init never gets called on startup, and so when we go to clean up in the CONF_modules path, we wind up allocating an rcu lock, which in this code, calls ossl_rcu_init, allocating a thread key, that then never gets de-allocated.

The good news is, this is fixed however, in commit 24d16d3, where we moved the thread key allocation into the library context, cleaning it up properly on shutdown.

The above commit appears to have gone in after 3.3.1 so if you build and run against the HEAD of master, the problem should be resolved

from openssl.

i retested and can confirm that the issue is fixed in master thanks

from openssl.

@TobiasFunk excellent, thank you! I'll close this then

from openssl.

@nhorman Should this be fixed also on the 3.3 branch. IMO it should.

from openssl.

we certainly could, though the issue only occurs if you don't load a conf file on startup. Is that a frequent occurance?

On the other hand, backporting is likely easy to do right now. It doesn't apply cleanly, but its only got one small conflict, so if we want to, better now than later.

Yeah, reading that, it makes sense. I'll do the backport against this PR

from openssl.