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

From: Jan Urbański <wulczer(at)wulczer(dot)org>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: libpq's multi-threaded SSL callback handling is busted
Date: 2015-04-09 19:54:48
Message-ID: 87zj6ht6ef.fsf@wulczer.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


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

Attachment Content-Type Size
libpq-crypto-no-callback-stomping-v2.patch text/x-diff 1.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ian Stakenvicius 2015-04-09 20:30:01 Revisiting Re: BUG #8532: postgres fails to start with timezone-data >=2013e
Previous Message Magnus Hagander 2015-04-09 19:40:23 Re: psql showing owner in \dT