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

Alvaro Herrera 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).
>
> There's a problem in this patch which is that it is inconsistent in its
> use of the ENABLE_THREAD_SAFETY symbol.  init_ssl_system() is only going
> to keep the refcount in the threaded compile; but the safeguards are
> needed even when threading is not enabled.  Moreover,

Actually, CRYPTO_set_locking_callback() and CRYPTO_set_id_callbac() are
needed only for threaded SSL programs;  I have added a comments
mentioning that.

> destroy_ssl_system() is locking thread mutexes outside
> ENABLE_THREAD_SAFETY which is going to cause non-threaded builds to
> fail.

Yes, my defines were very messed up;  updated version attached.

> As a suggestion, I'd recommend not fooling around with backend files
> when you're only modifying libpq.  It enlarges the patch without
> benefit.  I think that patch should be committed separately.

OK, I will do that, though the backend change is being made to be
consistent with the front end.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: src/backend/libpq/be-secure.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/libpq/be-secure.c,v
retrieving revision 1.85
diff -c -c -r1.85 be-secure.c
*** src/backend/libpq/be-secure.c    24 Oct 2008 12:24:35 -0000    1.85
--- src/backend/libpq/be-secure.c    7 Nov 2008 23:16:28 -0000
***************
*** 88,94 ****
  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 ----
***************
*** 191,206 ****
      return 0;
  }

  /*
   *    Destroy global context
   */
  void
  secure_destroy(void)
  {
- #ifdef USE_SSL
-     destroy_SSL();
- #endif
  }

  /*
   *    Attempt to negotiate secure session.
--- 190,204 ----
      return 0;
  }

+ #ifdef NOT_USED
  /*
   *    Destroy global context
   */
  void
  secure_destroy(void)
  {
  }
+ #endif

  /*
   *    Attempt to negotiate secure session.
***************
*** 805,815 ****
      }
  }

  /*
!  *    Destroy global SSL context.
   */
  static void
! destroy_SSL(void)
  {
      if (SSL_context)
      {
--- 803,814 ----
      }
  }

+ #ifdef NOT_USED
  /*
!  *    Destroy global SSL context
   */
  static void
! destroy_global_SSL(void)
  {
      if (SSL_context)
      {
***************
*** 817,822 ****
--- 816,822 ----
          SSL_context = NULL;
      }
  }
+ #endif

  /*
   *    Attempt to negotiate SSL connection.
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.106
diff -c -c -r1.106 fe-secure.c
*** src/interfaces/libpq/fe-secure.c    24 Oct 2008 12:29:11 -0000    1.106
--- src/interfaces/libpq/fe-secure.c    7 Nov 2008 23:16:31 -0000
***************
*** 44,49 ****
--- 44,50 ----
  #endif
  #include <arpa/inet.h>
  #endif
+
  #include <sys/stat.h>

  #ifdef ENABLE_THREAD_SAFETY
***************
*** 57,72 ****
  #ifdef USE_SSL
  #include <openssl/ssl.h>
  #include <openssl/bio.h>
  #if (SSLEAY_VERSION_NUMBER >= 0x00907000L)
  #include <openssl/conf.h>
  #endif
  #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
  #include <openssl/engine.h>
  #endif
- #endif   /* USE_SSL */
-
-
- #ifdef USE_SSL

  #ifndef WIN32
  #define USER_CERT_FILE        ".postgresql/postgresql.crt"
--- 58,70 ----
  #ifdef USE_SSL
  #include <openssl/ssl.h>
  #include <openssl/bio.h>
+
  #if (SSLEAY_VERSION_NUMBER >= 0x00907000L)
  #include <openssl/conf.h>
  #endif
  #if (SSLEAY_VERSION_NUMBER >= 0x00907000L) && !defined(OPENSSL_NO_ENGINE)
  #include <openssl/engine.h>
  #endif

  #ifndef WIN32
  #define USER_CERT_FILE        ".postgresql/postgresql.crt"
***************
*** 87,112 ****
  #define ERR_pop_to_mark()    ((void) 0)
  #endif

- #ifdef NOT_USED
- static int    verify_peer_name_matches_certificate(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 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.
--- 85,122 ----
  #define ERR_pop_to_mark()    ((void) 0)
  #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 *);
  static void close_SSL(PGconn *);
  static char *SSLerrmessage(void);
  static void SSLerrfree(char *buf);
+
+ #ifdef NOT_USED
+ static int    verify_peer_name_matches_certificate(PGconn *);
  #endif

  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    /* USE_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.
***************
*** 760,805 ****
  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)
--- 770,821 ----
  init_ssl_system(PGconn *conn)
  {
  #ifdef ENABLE_THREAD_SAFETY
! #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)
***************
*** 822,839 ****
                                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
--- 838,898 ----
                                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.
+  */
+ 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
***************
*** 897,907 ****
      return 0;
  }

  /*
!  *    Destroy global SSL context.
   */
  static void
! destroy_SSL(void)
  {
      if (SSL_context)
      {
--- 956,973 ----
      return 0;
  }

+ static void
+ destroy_SSL(void)
+ {
+     destroy_ssl_system();
+ }
+
+ #ifdef NOT_USED
  /*
!  *    Destroy global SSL context
   */
  static void
! destroy_global_SSL(void)
  {
      if (SSL_context)
      {
***************
*** 909,914 ****
--- 975,981 ----
          SSL_context = NULL;
      }
  }
+ #endif

  /*
   *    Attempt to negotiate SSL connection.
***************
*** 1028,1033 ****
--- 1095,1101 ----
          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();