Access violation possible race condition and memory/handle leak on global mutexes in x509_issuer_cache.c

[testuser220]

In x509_issuer_cache.c, there is a global mutex on the certificate cache:

static pthread_mutex_t x509_issuer_tree_mutex = PTHREAD_MUTEX_INITIALIZER;

This mutex is allocated and initialized with a handle upon the first call that requires this mutex be acquired from any function that works with the certificate cache, i.e. x509_issuer_cache_find(), etc.

However, the access to the cache can happen upon initiating the SSL connections in many threads at the same time.

In this case, there will be a race condition: several threads will initialize this mutex at the same time. This will leads to A/V as several threads will then gain exclusive access to the certificate cache, and then it will be a memory leak because only one (last) mutex will be stored in this variable. Which is never freed anyway, which is another issue, besides the memory leak, there is also a handle leak, as the handle allocated for the mutex is never closed with the operating system.

Suggestions:

1. Initialize this global variable in SSL_library_init
2. Free this global variable in OPENSSL_cleanup(), i.e. in the x509_issuer_cache_free, which is only called from OPENSSL_cleanup()

The same issue actually applies to other global mutexes that are allocated on demand.

[botovq]

Thanks for the report. I agree, although the initialization should probably happen in OPENSSL_init_crypto_internal() (which is called from SSL_library_init()). @bob-beck can you take a look, please?

[botovq]

On second thought, this is just nonsense:

https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_mutex_init.html

[testuser220]

Please check out pthread.c in compat folder where it defines posix functions under Windows.

So this bug is clearly present under Windows build.

[testuser220]

It may not have an A/V race condition because of the Interlocked Exchange operation, but it surely has leaks. I have Visual Studio debug printout for this.

[botovq]

@testuser220 thanks for the follow-up. That there are leaks due to the compat implementation and due to missing cleanup in OpenSSL_cleanup(), I can believe.