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
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index a32af34..02c9177 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -806,9 +806,12 @@ pgtls_init(PGconn *conn)
if (ssl_open_connections++ == 0)
{
- /* These are only required for threaded libcrypto applications */
- CRYPTO_set_id_callback(pq_threadidcallback);
- CRYPTO_set_locking_callback(pq_lockingcallback);
+ /* These are only required for threaded libcrypto applications, but
+ * make sure we don't stomp on them if they're already set. */
+ if (CRYPTO_get_id_callback() == NULL)
+ CRYPTO_set_id_callback(pq_threadidcallback);
+ if (CRYPTO_get_locking_callback() == NULL)
+ CRYPTO_set_locking_callback(pq_lockingcallback);
}
}
#endif /* ENABLE_THREAD_SAFETY */
@@ -885,9 +888,12 @@ destroy_ssl_system(void)
if (pq_init_crypto_lib && ssl_open_connections == 0)
{
- /* No connections left, unregister libcrypto callbacks */
- CRYPTO_set_locking_callback(NULL);
- CRYPTO_set_id_callback(NULL);
+ /* No connections left, unregister libcrypto callbacks, if no one
+ * registered different ones in the meantime. */
+ if (CRYPTO_get_id_callback() == pq_threadidcallback)
+ CRYPTO_set_id_callback(NULL);
+ if (CRYPTO_get_locking_callback() == pq_lockingcallback)
+ CRYPTO_set_locking_callback(NULL);
/*
* We don't free the lock array or the SSL_context. If we get another
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
