Re: libpq's multi-threaded SSL callback handling is busted

Peter Eisentraut writes:

> On 2/12/15 7:28 AM, Jan Urbański wrote:
>> +#if OPENSSL_VERSION_NUMBER < 0x10000000
>> +/* OpenSSL 1.0.0 deprecates the CRYPTO_set_id_callback function and provides a
>> + * default implementation, so there's no need for our own. */
>
> I have some additional concerns about this.  It is true that OpenSSL
> 1.0.0 deprecates CRYPTO_set_id_callback(), but it replaces it with
> CRYPTO_THREADID_set_callback().  There is no indication that you don't
> need to set a callback anymore.  The man page
> (https://www.openssl.org/docs/crypto/threads.html) still says you need
> to set two callbacks, and points to the new interface.

Truly, no good deed can ever go unpunished.

I spent around an hour tracking down why setting both callbacks
for OpenSSL >= 1.0.0 brought back the PHP segfaults. Turns out, in OpenSSL
there's *no way* to unregister a callback registered with
CRYPTO_THREADID_set_callback()!

Observe: https://github.com/openssl/openssl/blob/35a1cc90bc1795e8893c11e442790ee7f659fffb/crypto/thr_id.c#L174-L180

Once you set a callback, game over - unloading your library will cause a
segfault. I cannot express how joyful I felt when I discovered it.

> I suggest you remove this part from your patch and submit a separate
> patch for consideration if you want to.

At this point I will propose an even simpler patch (attached). I gave up on
trying to use the more modern CRYPTO_THREADID_* functions.

Right now, HEAD libpq won't compile against OpenSSL compiled with
OPENSSL_NO_DEPRECATED, which I think is fine, given the lack of complaints. So
let's just keep using the older, saner functions and ignore the THREADID crap.

By the way, might I take the opportunity to breach the subject of backpatching
this change, should it get accepted?

Cheers,
Jan