openssl_id_func breaks OpenSSL thread safety requirements on non-Windows platforms

[criticalsection@…]

In order to be thread safe OpenSSL requires call backs that provide:

1) Thread level locking (e.g. mutexes)

2) A unique id for each thread

The Boost ASIO SSL support provides 1) but only a partial implementation for 2). The openssl_id_func() supplied to CRYPTO_set_id_callback uses GetCurrentThreadId on Windows but on other platforms uses this:

void* id = instance()->thread_id_;

if (id == 0)

instance()->thread_id_ = id = &id; Ugh.

BOOST_ASSERT(sizeof(unsigned long) >= sizeof(void*)); return reinterpret_cast<unsigned long>(id);

The is initially based on the address of the local variable id that should be different in each thread but may also vary in a given thread depending on the call stack.

However, the value seems to be assigned to a shared instance so that all threads will see the same value.

I recently discovered that some thread safety problems on iOS were being caused by this code.

I worked around this problem by calling CRYPTO_set_id_callback again after creating an boost::asio::ssl::context to overwrite the callback with one like this for non Windows platforms:

unsigned long my_openssl_id_func() {

return pthread_mach_thread_np(pthread_self() );

}

After this change the Boost SSL ASIO library suffered no more re-entrancy and locking issues.

I suggest that a solution could incorporate some or all of the following:

-Remove the existing non Windows implementation as it doesn't seem to meet the OpenSSL requirements -Provide a pthread based implementation in openssl_id_func when PTHREADS are available

-A runtime assert/exception if a suitable implementation has not been provided -Provide more control over OpenSSL initialization to the user of Boost ASIO so that aspects such as this can be explicitly handled.

[criticalsection@…]

Sorry, on further testing I'm sure if that was the cause of the problem I was seeing.

It might not actually have caused a lack of thread safety in OpenSSL but it doesn't really look right and probably needs updating.

I have downgraded the severity to Cosmetic

[chris_kohlhoff]

The instance()->thread_id_ member is a thread-local variable.

Please open a new ticket if you can reproduce the issue with a small, self-contained test case. Thanks.

[anonymous]

Sorry, I didn't notice it was thread local. As each thread has its own stack I guess the address of &id is guaranteed to be local.

Perhaps, the "Ugh." comment is misleading then. It could just say 'Take address of location on the thread's stack that will be unique per thread".

BTW, thanks for a great library!