Re: Re: [BUGS] libpq does not manage SSL callbacks properly when other libraries are involved. - Mailing list pgsql-hackers

Bruce Momjian wrote:
> Bruce Momjian wrote:
>> Thanks for the review, Magnus.  I have adjusted the patch to use the
>> same mutex every time the counter is accessed, and adjusted the
>> pqsecure_destroy() call to properly decrement in the right place.
>>
>> Also, I renamed the libpq global destroy function to be clearer
>> (the function is not exported).
>
> Here is an updated version of the patch to match CVS HEAD.

I've updated it to match what's CVS HEAD now, and made some minor
modifications. Renamed destroySSL() to make it consistent with
initializeSSL(). Added and changed some comments. ssldiff.patch contains
my changes against Bruce's patch.

I also removed the #ifdef NOT_USED parts. They are in CVS history if we
need them, and they're trivial things anyway, so I think this is much
cleaner.

With this, it looks fine to me. Especially since we've seen some testing
from the PHP folks already.

//Magnus
*** src/backend/libpq/be-secure.c
--- src/backend/libpq/be-secure.c
***************
*** 88,94 **** static DH  *tmp_dh_cb(SSL *s, int is_export, int keylength);
  static int    verify_cb(int, X509_STORE_CTX *);
  static void info_cb(const SSL *ssl, int type, int args);
  static void initialize_SSL(void);
- static void destroy_SSL(void);
  static int    open_server_SSL(Port *);
  static void close_SSL(Port *);
  static const char *SSLerrmessage(void);
--- 88,93 ----
***************
*** 193,209 **** secure_initialize(void)
  }

  /*
-  *    Destroy global context
-  */
- void
- secure_destroy(void)
- {
- #ifdef USE_SSL
-     destroy_SSL();
- #endif
- }
-
- /*
   * Indicate if we have loaded the root CA store to verify certificates
   */
  bool
--- 192,197 ----
***************
*** 844,862 **** initialize_SSL(void)
  }

  /*
-  *    Destroy global SSL context.
-  */
- static void
- destroy_SSL(void)
- {
-     if (SSL_context)
-     {
-         SSL_CTX_free(SSL_context);
-         SSL_context = NULL;
-     }
- }
-
- /*
   *    Attempt to negotiate SSL connection.
   */
  static int
--- 832,837 ----
*** src/interfaces/libpq/fe-secure.c
--- src/interfaces/libpq/fe-secure.c
***************
*** 44,49 ****
--- 44,50 ----
  #endif
  #include <arpa/inet.h>
  #endif
+
  #include <sys/stat.h>

  #ifdef ENABLE_THREAD_SAFETY
***************
*** 89,108 **** static bool verify_peer_name_matches_certificate(PGconn *);
  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 int    initialize_SSL(PGconn *);
! static void destroy_SSL(void);
  static PostgresPollingStatusType open_client_SSL(PGconn *);
  static void close_SSL(PGconn *);
  static char *SSLerrmessage(void);
  static void SSLerrfree(char *buf);
- #endif

- #ifdef USE_SSL
  static bool pq_initssllib = true;
-
  static SSL_CTX *SSL_context = NULL;
  #endif

  /*
   * Macros to handle disabling and then restoring the state of SIGPIPE handling.
   * Note that DISABLE_SIGPIPE() must appear at the start of a block.
--- 90,121 ----
  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 destroySSL(void);
  static PostgresPollingStatusType open_client_SSL(PGconn *);
  static void close_SSL(PGconn *);
  static char *SSLerrmessage(void);
  static void SSLerrfree(char *buf);

  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 */
+
+ #endif /* SSL */
+
+
  /*
   * Macros to handle disabling and then restoring the state of SIGPIPE handling.
   * Note that DISABLE_SIGPIPE() must appear at the start of a block.
***************
*** 186,192 **** void
  pqsecure_destroy(void)
  {
  #ifdef USE_SSL
!     destroy_SSL();
  #endif
  }

--- 199,205 ----
  pqsecure_destroy(void)
  {
  #ifdef USE_SSL
!     destroySSL();
  #endif
  }

***************
*** 734,739 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
--- 747,755 ----
  }

  #ifdef ENABLE_THREAD_SAFETY
+ /*
+  *    Callback functions for OpenSSL internal locking
+  */

  static unsigned long
  pq_threadidcallback(void)
***************
*** 765,818 **** pq_lockingcallback(int mode, int n, const char *file, int line)
  #endif   /* ENABLE_THREAD_SAFETY */

  /*
!  * Also see similar code in fe-connect.c, default_threadlock()
   */
  static int
  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)
          {
!             if (pthread_mutex_init(&init_mutex, NULL))
                  return -1;
          }
!         InterlockedExchange(&mutex_initlock, 0);
      }
  #endif
!     if (pthread_mutex_lock(&init_mutex))
          return -1;

!     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++)
          {
!             if (pthread_mutex_init(&pq_lockarray[i], NULL))
                  return -1;
          }

!         CRYPTO_set_locking_callback(pq_lockingcallback);
      }
! #endif
      if (!SSL_context)
      {
          if (pq_initssllib)
--- 781,854 ----
  #endif   /* ENABLE_THREAD_SAFETY */

  /*
!  * Initialize SSL system. In threadsafe mode, this includes setting
!  * up OpenSSL callback functions to do thread locking.
!  *
!  * If the caller has told us (through PQinitSSL) that he's taking care
!  * of SSL, we expect that callbacks are already set, and won't try to
!  * override it.
!  *
!  * The conn parameter is only used to be able to pass back an error
!  * message - no connection local setup is made.
   */
  static int
  init_ssl_system(PGconn *conn)
  {
  #ifdef ENABLE_THREAD_SAFETY
! #ifdef WIN32
!     /* Also see similar code in fe-connect.c, default_threadlock() */
!     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 necessary, set up an array to hold locks for OpenSSL. OpenSSL will
!          * tell us how big to make this array.
!          */
!         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 /* ENABLE_THREAD_SAFETY */
!
      if (!SSL_context)
      {
          if (pq_initssllib)
***************
*** 833,851 **** init_ssl_system(PGconn *conn)
                                err);
              SSLerrfree(err);
  #ifdef ENABLE_THREAD_SAFETY
!             pthread_mutex_unlock(&init_mutex);
  #endif
              return -1;
          }
      }
  #ifdef ENABLE_THREAD_SAFETY
!     pthread_mutex_unlock(&init_mutex);
  #endif
      return 0;
  }

  /*
!  *    Initialize global SSL context.
   */
  static int
  initialize_SSL(PGconn *conn)
--- 869,935 ----
                                err);
              SSLerrfree(err);
  #ifdef ENABLE_THREAD_SAFETY
!             pthread_mutex_unlock(&ssl_config_mutex);
  #endif
              return -1;
          }
      }
+
  #ifdef ENABLE_THREAD_SAFETY
!     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.
!  *
!  *    Callbacks are only set when we're compiled in threadsafe mode, so
!  *    we only need to remove them in this case.
!  */
! static void
! destroy_ssl_system(void)
! {
! #ifdef ENABLE_THREAD_SAFETY
!     /* Mutex is created in initialize_ssl_system() */
!     if (pthread_mutex_lock(&ssl_config_mutex))
!         return;
!
!     if (pq_initssllib)
!     {
!         if (ssl_open_connections > 0)
!             --ssl_open_connections;
!
!         if (ssl_open_connections == 0)
!         {
!             /* No connections left, unregister all callbacks */
!             CRYPTO_set_locking_callback(NULL);
!             CRYPTO_set_id_callback(NULL);
!
!             /*
!              * We don't free the lock array. If we get another connection
!              * from the same caller, we will just re-use it with the existing
!              * mutexes.
!              *
!              * This means we leak a little memory on repeated load/unload
!              * of the library.
!              */
!             free(pqlockarray);
!             pqlockarray = NULL;
!         }
!     }
!
!     pthread_mutex_unlock(&ssl_config_mutex);
! #endif
!     return;
! }
!
! /*
!  *    Initialize SSL context.
   */
  static int
  initialize_SSL(PGconn *conn)
***************
*** 932,948 **** initialize_SSL(PGconn *conn)
      return 0;
  }

- /*
-  *    Destroy global SSL context.
-  */
  static void
! destroy_SSL(void)
  {
!     if (SSL_context)
!     {
!         SSL_CTX_free(SSL_context);
!         SSL_context = NULL;
!     }
  }

  /*
--- 1016,1025 ----
      return 0;
  }

  static void
! destroySSL(void)
  {
!     destroy_ssl_system();
  }

  /*
***************
*** 1061,1066 **** close_SSL(PGconn *conn)
--- 1138,1144 ----
          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();
*** src/backend/libpq/be-secure.c
--- src/backend/libpq/be-secure.c
***************
*** 831,851 **** initialize_SSL(void)
      }
  }

- #ifdef NOT_USED
- /*
-  *    Destroy global SSL context
-  */
- static void
- destroy_global_SSL(void)
- {
-     if (SSL_context)
-     {
-         SSL_CTX_free(SSL_context);
-         SSL_context = NULL;
-     }
- }
- #endif
-
  /*
   *    Attempt to negotiate SSL connection.
   */
--- 831,836 ----
*** src/interfaces/libpq/fe-secure.c
--- src/interfaces/libpq/fe-secure.c
***************
*** 92,98 **** 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 *);
  static void close_SSL(PGconn *);
  static char *SSLerrmessage(void);
--- 92,98 ----
  static int    init_ssl_system(PGconn *conn);
  static void destroy_ssl_system(void);
  static int    initialize_SSL(PGconn *);
! static void destroySSL(void);
  static PostgresPollingStatusType open_client_SSL(PGconn *);
  static void close_SSL(PGconn *);
  static char *SSLerrmessage(void);
***************
*** 199,205 **** void
  pqsecure_destroy(void)
  {
  #ifdef USE_SSL
!     destroy_SSL();
  #endif
  }

--- 199,205 ----
  pqsecure_destroy(void)
  {
  #ifdef USE_SSL
!     destroySSL();
  #endif
  }

***************
*** 747,752 **** client_cert_cb(SSL *ssl, X509 **x509, EVP_PKEY **pkey)
--- 747,755 ----
  }

  #ifdef ENABLE_THREAD_SAFETY
+ /*
+  *    Callback functions for OpenSSL internal locking
+  */

  static unsigned long
  pq_threadidcallback(void)
***************
*** 778,790 **** pq_lockingcallback(int mode, int n, const char *file, int line)
  #endif   /* ENABLE_THREAD_SAFETY */

  /*
!  * Also see similar code in fe-connect.c, default_threadlock()
   */
  static int
  init_ssl_system(PGconn *conn)
  {
  #ifdef ENABLE_THREAD_SAFETY
  #ifdef WIN32
      if (ssl_config_mutex == NULL)
      {
          while (InterlockedExchange(&win32_ssl_create_mutex, 1) == 1)
--- 781,802 ----
  #endif   /* ENABLE_THREAD_SAFETY */

  /*
!  * Initialize SSL system. In threadsafe mode, this includes setting
!  * up OpenSSL callback functions to do thread locking.
!  *
!  * If the caller has told us (through PQinitSSL) that he's taking care
!  * of SSL, we expect that callbacks are already set, and won't try to
!  * override it.
!  *
!  * The conn parameter is only used to be able to pass back an error
!  * message - no connection local setup is made.
   */
  static int
  init_ssl_system(PGconn *conn)
  {
  #ifdef ENABLE_THREAD_SAFETY
  #ifdef WIN32
+     /* Also see similar code in fe-connect.c, default_threadlock() */
      if (ssl_config_mutex == NULL)
      {
          while (InterlockedExchange(&win32_ssl_create_mutex, 1) == 1)
***************
*** 802,811 **** init_ssl_system(PGconn *conn)

      if (pq_initssllib)
      {
          if (pq_lockarray == NULL)
          {
              int i;
!
              pq_lockarray = malloc(sizeof(pthread_mutex_t) * CRYPTO_num_locks());
              if (!pq_lockarray)
              {
--- 814,827 ----

      if (pq_initssllib)
      {
+         /*
+          * If necessary, set up an array to hold locks for OpenSSL. OpenSSL will
+          * tell us how big to make this array.
+          */
          if (pq_lockarray == NULL)
          {
              int i;
!
              pq_lockarray = malloc(sizeof(pthread_mutex_t) * CRYPTO_num_locks());
              if (!pq_lockarray)
              {
***************
*** 823,829 **** init_ssl_system(PGconn *conn)
                  }
              }
          }
!
          if (ssl_open_connections++ == 0)
          {
              /* These are only required for threaded SSL applications */
--- 839,845 ----
                  }
              }
          }
!
          if (ssl_open_connections++ == 0)
          {
              /* These are only required for threaded SSL applications */
***************
*** 858,863 **** init_ssl_system(PGconn *conn)
--- 874,880 ----
              return -1;
          }
      }
+
  #ifdef ENABLE_THREAD_SAFETY
      pthread_mutex_unlock(&ssl_config_mutex);
  #endif
***************
*** 870,904 **** init_ssl_system(PGconn *conn)
   *    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);
          }
      }

--- 887,925 ----
   *    SSL used by other parts of the system.  For this reason,
   *    we unregister the SSL callback functions when the last libpq
   *    connection is closed.
+  *
+  *    Callbacks are only set when we're compiled in threadsafe mode, so
+  *    we only need to remove them in this case.
   */
  static void
  destroy_ssl_system(void)
  {
  #ifdef ENABLE_THREAD_SAFETY
!     /* Mutex is created in initialize_ssl_system() */
      if (pthread_mutex_lock(&ssl_config_mutex))
          return;

      if (pq_initssllib)
      {
          if (ssl_open_connections > 0)
              --ssl_open_connections;

          if (ssl_open_connections == 0)
          {
!             /* No connections left, unregister all callbacks */
              CRYPTO_set_locking_callback(NULL);
              CRYPTO_set_id_callback(NULL);
+
+             /*
+              * We don't free the lock array. If we get another connection
+              * from the same caller, we will just re-use it with the existing
+              * mutexes.
+              *
+              * This means we leak a little memory on repeated load/unload
+              * of the library.
+              */
+             free(pqlockarray);
+             pqlockarray = NULL;
          }
      }

***************
*** 908,914 **** destroy_ssl_system(void)
  }

  /*
!  *    Initialize global SSL context.
   */
  static int
  initialize_SSL(PGconn *conn)
--- 929,935 ----
  }

  /*
!  *    Initialize SSL context.
   */
  static int
  initialize_SSL(PGconn *conn)
***************
*** 996,1021 **** initialize_SSL(PGconn *conn)
  }

  static void
! destroy_SSL(void)
  {
      destroy_ssl_system();
  }

- #ifdef NOT_USED
- /*
-  *    Destroy global SSL context
-  */
- static void
- destroy_global_SSL(void)
- {
-     if (SSL_context)
-     {
-         SSL_CTX_free(SSL_context);
-         SSL_context = NULL;
-     }
- }
- #endif
-
  /*
   *    Attempt to negotiate SSL connection.
   */
--- 1017,1027 ----
  }

  static void
! destroySSL(void)
  {
      destroy_ssl_system();
  }

  /*
   *    Attempt to negotiate SSL connection.
   */