Re: Re: [BUGS] libpq does not manage SSL callbacks properly when other libraries are involved.

Bruce Momjian wrote:
> Yes, my defines were very messed up;  updated version attached.
>
Hi,

I've not done a review of this patch, however I did backport it to 8.3
(as attached in unified diff). The patch wasn't made for PG purposes, so
it's not in context diff. I tested the backported patch and the issues I
was experiencing with the initial bug report have stopped.  So the fix
works for the initial reported problem.

Will this be back patched when it's committed?


Regards

Russell
diff -uNr postgresql-8.3.3/src/interfaces/libpq/fe-secure.c postgresql-8.3.3.new/src/interfaces/libpq/fe-secure.c
--- postgresql-8.3.3/src/interfaces/libpq/fe-secure.c    2008-01-29 13:03:39.000000000 +1100
+++ postgresql-8.3.3.new/src/interfaces/libpq/fe-secure.c    2008-11-13 20:57:40.000000000 +1100
@@ -142,12 +142,10 @@
 #define ERR_pop_to_mark()    ((void) 0)
 #endif

-#ifdef NOT_USED
-static int    verify_peer(PGconn *);
-#endif
 static int    verify_cb(int ok, X509_STORE_CTX *ctx);
 static int    client_cert_cb(SSL *, X509 **, EVP_PKEY **);
 static int    init_ssl_system(PGconn *conn);
+static void destroy_ssl_system(void);
 static int    initialize_SSL(PGconn *);
 static void destroy_SSL(void);
 static PostgresPollingStatusType open_client_SSL(PGconn *);
@@ -156,11 +154,19 @@
 static void SSLerrfree(char *buf);
 #endif

-#ifdef USE_SSL
 static bool pq_initssllib = true;

 static SSL_CTX *SSL_context = NULL;
+#ifdef ENABLE_THREAD_SAFETY
+static int ssl_open_connections = 0;
+
+#ifndef WIN32
+static pthread_mutex_t ssl_config_mutex = PTHREAD_MUTEX_INITIALIZER;
+#else
+static pthread_mutex_t ssl_config_mutex = NULL;
+static long win32_ssl_create_mutex = 0;
 #endif
+#endif    /* ENABLE_THREAD_SAFETY */

 /*
  * Macros to handle disabling and then restoring the state of SIGPIPE handling.
@@ -839,40 +845,53 @@
 init_ssl_system(PGconn *conn)
 {
 #ifdef ENABLE_THREAD_SAFETY
-#ifndef WIN32
-    static pthread_mutex_t init_mutex = PTHREAD_MUTEX_INITIALIZER;
-#else
-    static pthread_mutex_t init_mutex = NULL;
-    static long mutex_initlock = 0;
-
-    if (init_mutex == NULL)
-    {
-        while (InterlockedExchange(&mutex_initlock, 1) == 1)
-             /* loop, another thread own the lock */ ;
-        if (init_mutex == NULL)
-            pthread_mutex_init(&init_mutex, NULL);
-        InterlockedExchange(&mutex_initlock, 0);
-    }
-#endif
-    pthread_mutex_lock(&init_mutex);
-
-    if (pq_initssllib && pq_lockarray == NULL)
-    {
-        int            i;
-
-        CRYPTO_set_id_callback(pq_threadidcallback);
-
-        pq_lockarray = malloc(sizeof(pthread_mutex_t) * CRYPTO_num_locks());
-        if (!pq_lockarray)
-        {
-            pthread_mutex_unlock(&init_mutex);
-            return -1;
-        }
-        for (i = 0; i < CRYPTO_num_locks(); i++)
-            pthread_mutex_init(&pq_lockarray[i], NULL);
-
-        CRYPTO_set_locking_callback(pq_lockingcallback);
-    }
+#ifdef WIN32
+  if (ssl_config_mutex == NULL)
+  {
+      while (InterlockedExchange(&win32_ssl_create_mutex, 1) == 1)
+           /* loop, another thread own the lock */ ;
+      if (ssl_config_mutex == NULL)
+      {
+          if (pthread_mutex_init(&ssl_config_mutex, NULL))
+              return -1;
+      }
+      InterlockedExchange(&win32_ssl_create_mutex, 0);
+  }
+#endif
+   if (pthread_mutex_lock(&ssl_config_mutex))
+       return -1;
+
+   if (pq_initssllib)
+   {
+       if (pq_lockarray == NULL)
+       {
+           int i;
+
+           pq_lockarray = malloc(sizeof(pthread_mutex_t) * CRYPTO_num_locks());
+           if (!pq_lockarray)
+           {
+               pthread_mutex_unlock(&ssl_config_mutex);
+               return -1;
+           }
+           for (i = 0; i < CRYPTO_num_locks(); i++)
+           {
+               if (pthread_mutex_init(&pq_lockarray[i], NULL))
+               {
+                   free(pq_lockarray);
+                   pq_lockarray = NULL;
+                   pthread_mutex_unlock(&ssl_config_mutex);
+                   return -1;
+               }
+           }
+       }
+
+       if (ssl_open_connections++ == 0)
+       {
+           /* These are only required for threaded SSL applications */
+           CRYPTO_set_id_callback(pq_threadidcallback);
+           CRYPTO_set_locking_callback(pq_lockingcallback);
+       }
+    }
 #endif
     if (!SSL_context)
     {
@@ -894,18 +913,61 @@
                               err);
             SSLerrfree(err);
 #ifdef ENABLE_THREAD_SAFETY
-            pthread_mutex_unlock(&init_mutex);
+            pthread_mutex_unlock(&ssl_config_mutex);
 #endif
             return -1;
         }
     }
 #ifdef ENABLE_THREAD_SAFETY
-    pthread_mutex_unlock(&init_mutex);
+    pthread_mutex_unlock(&ssl_config_mutex);
 #endif
     return 0;
 }

 /*
+ *    This function is needed because if the libpq library is unloaded
+ *    from the application, the callback functions will no longer exist when
+ *    SSL used by other parts of the system.  For this reason,
+ *    we unregister the SSL callback functions when the last libpq
+ *    connection is closed.
+ */
+static void
+destroy_ssl_system(void)
+{
+#ifdef ENABLE_THREAD_SAFETY
+  /* Assume mutex is already created */
+  if (pthread_mutex_lock(&ssl_config_mutex))
+      return;
+
+  if (pq_initssllib)
+  {
+      /*
+       *  We never free pq_lockarray, which means we leak memory on
+       *  repeated loading/unloading of this library.
+       */
+
+      if (ssl_open_connections > 0)
+          --ssl_open_connections;
+
+      if (ssl_open_connections == 0)
+      {
+          /*
+           *  We need to unregister the SSL callbacks on last connection
+           *  close because the libpq shared library might be unloaded,
+           *  and once it is, callbacks must be removed to prevent them
+           *  from being called by other SSL code.
+           */
+          CRYPTO_set_locking_callback(NULL);
+          CRYPTO_set_id_callback(NULL);
+      }
+  }
+
+  pthread_mutex_unlock(&ssl_config_mutex);
+#endif
+  return;
+}
+
+/*
  *    Initialize global SSL context.
  */
 static int
@@ -969,18 +1031,26 @@
     return 0;
 }

+static void
+destroy_SSL(void)
+{
+  destroy_ssl_system();
+}
+
+#ifdef NOT_USED
 /*
- *    Destroy global SSL context.
+ * Destroy global SSL context
  */
 static void
-destroy_SSL(void)
+destroy_global_SSL(void)
 {
-    if (SSL_context)
+    if (SSL_context)
     {
         SSL_CTX_free(SSL_context);
         SSL_context = NULL;
     }
 }
+#endif

 /*
  *    Attempt to negotiate SSL connection.
@@ -1124,6 +1194,7 @@
         SSL_shutdown(conn->ssl);
         SSL_free(conn->ssl);
         conn->ssl = NULL;
+        pqsecure_destroy();
         /* We have to assume we got EPIPE */
         REMEMBER_EPIPE(true);
         RESTORE_SIGPIPE();