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