Re: libpq's multi-threaded SSL callback handling is busted - Mailing list pgsql-hackers

Jan Urbański writes:

> Andres Freund writes:
>
>> On 2015-02-12 09:31:27 +0100, Jan Urbański wrote:
>>> That doesn't solve the problem of the Python deadlock, where you're not at
>>> leisure to call a C function at the beginning of your module.
>>
>> We could just never unload the hooks...
>
> That's what we did before 4e816286533dd34c10b368487d4079595a3e1418 :) And it
> got changed after http://www.postgresql.org/message-id/48620925.6070806@pws.com.au
>
>>
>>> > * If there's already callbacks set: Remember that fact and don't
>>> >   overwrite. In the next major version: warn.
>>>
>>> So yeah, that was my initial approach - check if callbacks are set, don't do
>>> the dance if they are. It felt like a crutch, though, and racy at that. There's
>>> no atomic way to test-and-set those callbacks. The window for racyness is
>>> small, though.
>>
>> If you do that check during library initialization instead of every
>> connection it shouldn't be racy - if that part is run in a multithreaded
>> fashion you're doing something crazy.
>
> Yes, that's true. The problem is that there's no real libpq initialisation
> function. The docs say that:
>
> "If your application initializes libssl and/or libcrypto libraries and libpq is
> built with SSL support, you should call PQinitOpenSSL"
>
> So most apps will just not bother. The moment you know you'll need SSL is only
> when you get an 'S' message from the server...

For the sake of discussion, here's a patch to prevent stomping on
previously-set callbacks, racy as it looks.

FWIW, it does fix the Python deadlock and doesn't cause the PHP segfault...

J
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index a32af34..54312a5 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -710,6 +710,9 @@ verify_peer_name_matches_certificate(PGconn *conn)
  *    Callback functions for OpenSSL internal locking
  */

+#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. */
 static unsigned long
 pq_threadidcallback(void)
 {
@@ -720,6 +723,7 @@ pq_threadidcallback(void)
      */
     return (unsigned long) pthread_self();
 }
+#endif   /* OPENSSL_VERSION_NUMBER < 0x10000000 */

 static pthread_mutex_t *pq_lockarray;

@@ -806,9 +810,14 @@ 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 OPENSSL_VERSION_NUMBER < 0x10000000
+            if (CRYPTO_get_id_callback() == NULL)
+                CRYPTO_set_id_callback(pq_threadidcallback);
+#endif
+            if (CRYPTO_get_locking_callback() == NULL)
+                CRYPTO_set_locking_callback(pq_lockingcallback);
         }
     }
 #endif   /* ENABLE_THREAD_SAFETY */
@@ -885,9 +894,14 @@ 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 OPENSSL_VERSION_NUMBER < 0x10000000
+        if (CRYPTO_get_id_callback() == pq_threadidcallback)
+            CRYPTO_set_id_callback(NULL);
+#endif
+        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