Re: PQinitSSL broken in some use casesf - Mailing list pgsql-hackers

Robert Haas wrote:
> On Fri, Feb 13, 2009 at 9:17 AM, Andrew Chernow <ac@esilo.com> wrote:
>> Should I create a patch implementing the PQinitCrypto idea?
>
> I think that would be helpful.  Seeing the code will give everyone a
> better idea of exactly what the proposed change is and whether it's
> acceptable.
>
> ...Robert
>
>

Patch attached.

One thing I noticed is the ssl_open_connections variable is ref counting
connections when pq_initssllib is true.  But, it now only affects crypto
library init and cleanup calls.  Point is, ref counting is only needed
if pq_initcryptolib is true and it should be renamed to
crypto_open_connections.  I didn't do this in the patch.  Its the same
old name and the counter is incremented if pq_initssllib or
pq_initcryptolib is true.  Please advise.

--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Index: src/interfaces/libpq/exports.txt
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/exports.txt,v
retrieving revision 1.22
diff -C6 -r1.22 exports.txt
*** src/interfaces/libpq/exports.txt    22 Sep 2008 13:55:14 -0000    1.22
--- src/interfaces/libpq/exports.txt    13 Feb 2009 17:01:22 -0000
***************
*** 149,154 ****
--- 149,155 ----
  PQinstanceData            147
  PQsetInstanceData         148
  PQresultInstanceData      149
  PQresultSetInstanceData   150
  PQfireResultCreateEvents  151
  PQconninfoParse           152
+ PQinitCrypto              153
\ No newline at end of file
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.119
diff -C6 -r1.119 fe-secure.c
*** src/interfaces/libpq/fe-secure.c    28 Jan 2009 15:06:47 -0000    1.119
--- src/interfaces/libpq/fe-secure.c    13 Feb 2009 17:01:22 -0000
***************
*** 96,107 ****
--- 96,108 ----
  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 bool pq_initcryptolib = true;
  static SSL_CTX *SSL_context = NULL;

  #ifdef ENABLE_THREAD_SAFETY
  static int ssl_open_connections = 0;

  #ifndef WIN32
***************
*** 175,186 ****
--- 176,199 ----
  #ifdef USE_SSL
      pq_initssllib = do_init;
  #endif
  }

  /*
+  *    Exported function to allow application to tell us it's already
+  *    initialized the OpenSSL Crypto library.
+  */
+ void
+ PQinitCrypto(int do_init)
+ {
+ #ifdef USE_SSL
+     pq_initcryptolib = do_init;
+ #endif
+ }
+
+ /*
   *    Initialize global context
   */
  int
  pqsecure_initialize(PGconn *conn)
  {
      int            r = 0;
***************
*** 820,831 ****
--- 833,846 ----
   * message - no connection local setup is made.
   */
  static int
  init_ssl_system(PGconn *conn)
  {
  #ifdef ENABLE_THREAD_SAFETY
+     int num_ssl_conns = 0;
+
  #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 */ ;
***************
*** 837,849 ****
          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)
--- 852,873 ----
          InterlockedExchange(&win32_ssl_create_mutex, 0);
      }
  #endif
      if (pthread_mutex_lock(&ssl_config_mutex))
          return -1;

!     /*
!      * Increment connection count if we are responsible for
!      * intiializing the SSL or Crypto library.  Currently, only
!      * crypto needs this during setup and cleanup.  That may
!      * change in the future so we always update the counter.
!      */
!     if (pq_initssllib || pq_initcryptolib)
!         num_ssl_conns = ssl_open_connections++;
!
!     if (pq_initcryptolib)
      {
          /*
           * 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)
***************
*** 865,877 ****
                      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);
          }
      }
--- 889,901 ----
                      pthread_mutex_unlock(&ssl_config_mutex);
                      return -1;
                  }
              }
          }

!         if (num_ssl_conns == 0)
          {
              /* These are only required for threaded SSL applications */
              CRYPTO_set_id_callback(pq_threadidcallback);
              CRYPTO_set_locking_callback(pq_lockingcallback);
          }
      }
***************
*** 924,955 ****
  {
  #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.
!              */
!         }
      }

      pthread_mutex_unlock(&ssl_config_mutex);
  #endif
      return;
  }
--- 948,976 ----
  {
  #ifdef ENABLE_THREAD_SAFETY
      /* Mutex is created in initialize_ssl_system() */
      if (pthread_mutex_lock(&ssl_config_mutex))
          return;

!     if ((pq_initssllib || pq_initcryptolib) && ssl_open_connections > 0)
!         --ssl_open_connections;
!
!     if (pq_initcryptolib && 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.
!          */
      }

      pthread_mutex_unlock(&ssl_config_mutex);
  #endif
      return;
  }
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.145
diff -C6 -r1.145 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h    1 Jan 2009 17:24:03 -0000    1.145
--- src/interfaces/libpq/libpq-fe.h    13 Feb 2009 17:01:22 -0000
***************
*** 299,310 ****
--- 299,313 ----
   * unencrypted connections or if any other TLS library is in use. */
  extern void *PQgetssl(PGconn *conn);

  /* Tell libpq whether it needs to initialize OpenSSL */
  extern void PQinitSSL(int do_init);

+ /* Tell libpq whether it needs to initialize the OpenSSL Crypto library. */
+ extern void PQinitCrypto(int do_init)
+
  /* Set verbosity for PQerrorMessage and PQresultErrorMessage */
  extern PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity);

  /* Enable/disable tracing */
  extern void PQtrace(PGconn *conn, FILE *debug_port);
  extern void PQuntrace(PGconn *conn);