Re: libpq thread safety

Bruce Momjian wrote:

>Your patch has been added to the PostgreSQL unapplied patches list at:
>
>    http://momjian.postgresql.org/cgi-bin/pgpatches
>
>I will try to apply it within the next 48 hours.
>
>
You are too fast: the patch was a proof of concept, not really tested
(actually quite buggy).
Attached are two patches:

- ready-sigpipe: check_sigpipe_handler skips pthread_create_key if a
signal handler was installed. This is wrong - the key is always required.
- ready-locking: locking around kerberos and openssl.

The patches pass the regression tests on i386 linux. Kerberos is
untested, ssl only partially tested due to the lack of a test setup.
I'm still not sure if the new code is the right thing for the openssl
initialization: libpq calls SSL_library_init() unconditionally. If the
calling app uses ssl, too, this might confuse openssl.

Could you replace my initial proposal with these two patches?

Btw, is it intentional that THREAD_SUPPORT is not set in src/template/linux?

--
    Manfred
Index: src/backend/libpq/md5.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/backend/libpq/md5.c,v
retrieving revision 1.22
diff -c -r1.22 md5.c
*** src/backend/libpq/md5.c    29 Nov 2003 19:51:49 -0000    1.22
--- src/backend/libpq/md5.c    14 Mar 2004 10:46:54 -0000
***************
*** 271,277 ****
  static void
  bytesToHex(uint8 b[16], char *s)
  {
!     static char *hex = "0123456789abcdef";
      int            q,
                  w;

--- 271,277 ----
  static void
  bytesToHex(uint8 b[16], char *s)
  {
!     static const char *hex = "0123456789abcdef";
      int            q,
                  w;

Index: src/interfaces/libpq/fe-auth.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.89
diff -c -r1.89 fe-auth.c
*** src/interfaces/libpq/fe-auth.c    7 Jan 2004 18:56:29 -0000    1.89
--- src/interfaces/libpq/fe-auth.c    14 Mar 2004 10:46:55 -0000
***************
*** 590,595 ****
--- 590,596 ----

          case AUTH_REQ_KRB4:
  #ifdef KRB4
+             pglock_thread();
              if (pg_krb4_sendauth(PQerrormsg, conn->sock,
                                 (struct sockaddr_in *) & conn->laddr.addr,
                                 (struct sockaddr_in *) & conn->raddr.addr,
***************
*** 597,604 ****
--- 598,607 ----
              {
                  snprintf(PQerrormsg, PQERRORMSG_LENGTH,
                      libpq_gettext("Kerberos 4 authentication failed\n"));
+                 pgunlock_thread();
                  return STATUS_ERROR;
              }
+             pgunlock_thread();
              break;
  #else
              snprintf(PQerrormsg, PQERRORMSG_LENGTH,
***************
*** 608,620 ****
--- 611,626 ----

          case AUTH_REQ_KRB5:
  #ifdef KRB5
+             pglock_thread();
              if (pg_krb5_sendauth(PQerrormsg, conn->sock,
                                   hostname) != STATUS_OK)
              {
                  snprintf(PQerrormsg, PQERRORMSG_LENGTH,
                      libpq_gettext("Kerberos 5 authentication failed\n"));
+                 pgunlock_thread();
                  return STATUS_ERROR;
              }
+             pgunlock_thread();
              break;
  #else
              snprintf(PQerrormsg, PQERRORMSG_LENGTH,
***************
*** 722,727 ****
--- 728,734 ----
      if (authsvc == 0)
          return NULL;            /* leave original error message in place */

+     pglock_thread();
  #ifdef KRB4
      if (authsvc == STARTUP_KRB4_MSG)
          name = pg_krb4_authname(PQerrormsg);
***************
*** 759,763 ****
--- 766,771 ----

      if (name && (authn = (char *) malloc(strlen(name) + 1)))
          strcpy(authn, name);
+     pgunlock_thread();
      return authn;
  }
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.268
diff -c -r1.268 fe-connect.c
*** src/interfaces/libpq/fe-connect.c    10 Mar 2004 21:12:47 -0000    1.268
--- src/interfaces/libpq/fe-connect.c    14 Mar 2004 10:46:56 -0000
***************
*** 2902,2908 ****
  PQsetClientEncoding(PGconn *conn, const char *encoding)
  {
      char        qbuf[128];
!     static char query[] = "set client_encoding to '%s'";
      PGresult   *res;
      int            status;

--- 2902,2908 ----
  PQsetClientEncoding(PGconn *conn, const char *encoding)
  {
      char        qbuf[128];
!     static const char query[] = "set client_encoding to '%s'";
      PGresult   *res;
      int            status;

***************
*** 3162,3166 ****
--- 3162,3207 ----
      return NULL;

  #undef LINELEN
+ }
+
+ /*
+  * To keep the API consistent, the locking stubs are always provided, even
+  * if they are not required.
+  */
+
+ void
+ PQenableSSLLocks(int enable)
+ {
+ #if defined(ENABLE_THREAD_SAFETY) && defined(USE_SSL)
+     pq_usessllocks = enable;
+ #endif
+ }
+
+ static pgthreadlock_t default_threadlock;
+ static void
+ default_threadlock(int acquire)
+ {
+ #if defined(ENABLE_THREAD_SAFETY)
+     static pthread_mutex_t singlethread_lock = PTHREAD_MUTEX_INITIALIZER;
+     if (acquire)
+         pthread_mutex_lock(&singlethread_lock);
+     else
+         pthread_mutex_unlock(&singlethread_lock);
+ #endif
+ }
+
+ pgthreadlock_t *g_threadlock = default_threadlock;
+
+ pgthreadlock_t *
+ PQregisterThreadLock(pgthreadlock_t *newhandler)
+ {
+     pgthreadlock_t *prev;
+
+     prev = g_threadlock;
+     if (newhandler)
+         g_threadlock = newhandler;
+     else
+         g_threadlock = default_threadlock;
+     return prev;
  }

Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.37
diff -c -r1.37 fe-secure.c
*** src/interfaces/libpq/fe-secure.c    10 Feb 2004 15:21:24 -0000    1.37
--- src/interfaces/libpq/fe-secure.c    14 Mar 2004 10:46:56 -0000
***************
*** 135,145 ****
  static DH  *load_dh_buffer(const char *, size_t);
  static DH  *tmp_dh_cb(SSL *s, int is_export, int keylength);
  static int    client_cert_cb(SSL *, X509 **, EVP_PKEY **);
  static int    initialize_SSL(PGconn *);
  static void destroy_SSL(void);
  static PostgresPollingStatusType open_client_SSL(PGconn *);
  static void close_SSL(PGconn *);
! static const char *SSLerrmessage(void);
  #endif

  #ifdef USE_SSL
--- 135,147 ----
  static DH  *load_dh_buffer(const char *, size_t);
  static DH  *tmp_dh_cb(SSL *s, int is_export, int keylength);
  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
***************
*** 251,259 ****
              !SSL_set_app_data(conn->ssl, conn) ||
              !SSL_set_fd(conn->ssl, conn->sock))
          {
              printfPQExpBuffer(&conn->errorMessage,
                 libpq_gettext("could not establish SSL connection: %s\n"),
!                               SSLerrmessage());
              close_SSL(conn);
              return PGRES_POLLING_FAILED;
          }
--- 253,263 ----
              !SSL_set_app_data(conn->ssl, conn) ||
              !SSL_set_fd(conn->ssl, conn->sock))
          {
+             char *err = SSLerrmessage();
              printfPQExpBuffer(&conn->errorMessage,
                 libpq_gettext("could not establish SSL connection: %s\n"),
!                               err);
!             SSLerrfree(err);
              close_SSL(conn);
              return PGRES_POLLING_FAILED;
          }
***************
*** 327,334 ****
                      break;
                  }
              case SSL_ERROR_SSL:
!                 printfPQExpBuffer(&conn->errorMessage,
!                       libpq_gettext("SSL error: %s\n"), SSLerrmessage());
                  /* fall through */
              case SSL_ERROR_ZERO_RETURN:
                  SOCK_ERRNO_SET(ECONNRESET);
--- 331,342 ----
                      break;
                  }
              case SSL_ERROR_SSL:
!                 {
!                     char *err = SSLerrmessage();
!                     printfPQExpBuffer(&conn->errorMessage,
!                           libpq_gettext("SSL error: %s\n"), err);
!                     SSLerrfree(err);
!                 }
                  /* fall through */
              case SSL_ERROR_ZERO_RETURN:
                  SOCK_ERRNO_SET(ECONNRESET);
***************
*** 402,409 ****
                      break;
                  }
              case SSL_ERROR_SSL:
!                 printfPQExpBuffer(&conn->errorMessage,
!                       libpq_gettext("SSL error: %s\n"), SSLerrmessage());
                  /* fall through */
              case SSL_ERROR_ZERO_RETURN:
                  SOCK_ERRNO_SET(ECONNRESET);
--- 410,421 ----
                      break;
                  }
              case SSL_ERROR_SSL:
!                 {
!                     char *err = SSLerrmessage();
!                     printfPQExpBuffer(&conn->errorMessage,
!                           libpq_gettext("SSL error: %s\n"), err);
!                     SSLerrfree(err);
!                 }
                  /* fall through */
              case SSL_ERROR_ZERO_RETURN:
                  SOCK_ERRNO_SET(ECONNRESET);
***************
*** 750,758 ****
      }
      if (PEM_read_X509(fp, x509, NULL, NULL) == NULL)
      {
          printfPQExpBuffer(&conn->errorMessage,
                    libpq_gettext("could not read certificate (%s): %s\n"),
!                           fnbuf, SSLerrmessage());
          fclose(fp);
          return -1;
      }
--- 762,772 ----
      }
      if (PEM_read_X509(fp, x509, NULL, NULL) == NULL)
      {
+         char *err = SSLerrmessage();
          printfPQExpBuffer(&conn->errorMessage,
                    libpq_gettext("could not read certificate (%s): %s\n"),
!                           fnbuf, err);
!         SSLerrfree(err);
          fclose(fp);
          return -1;
      }
***************
*** 795,803 ****
      }
      if (PEM_read_PrivateKey(fp, pkey, cb, NULL) == NULL)
      {
          printfPQExpBuffer(&conn->errorMessage,
                    libpq_gettext("could not read private key (%s): %s\n"),
!                           fnbuf, SSLerrmessage());
          X509_free(*x509);
          fclose(fp);
          return -1;
--- 809,819 ----
      }
      if (PEM_read_PrivateKey(fp, pkey, cb, NULL) == NULL)
      {
+         char *err = SSLerrmessage();
          printfPQExpBuffer(&conn->errorMessage,
                    libpq_gettext("could not read private key (%s): %s\n"),
!                           fnbuf, err);
!         SSLerrfree(err);
          X509_free(*x509);
          fclose(fp);
          return -1;
***************
*** 807,815 ****
      /* verify that the cert and key go together */
      if (!X509_check_private_key(*x509, *pkey))
      {
          printfPQExpBuffer(&conn->errorMessage,
              libpq_gettext("certificate/private key mismatch (%s): %s\n"),
!                           fnbuf, SSLerrmessage());
          X509_free(*x509);
          EVP_PKEY_free(*pkey);
          return -1;
--- 823,833 ----
      /* verify that the cert and key go together */
      if (!X509_check_private_key(*x509, *pkey))
      {
+         char *err = SSLerrmessage();
          printfPQExpBuffer(&conn->errorMessage,
              libpq_gettext("certificate/private key mismatch (%s): %s\n"),
!                           fnbuf, err);
!         SSLerrfree(err);
          X509_free(*x509);
          EVP_PKEY_free(*pkey);
          return -1;
***************
*** 819,838 ****
  #endif
  }

! /*
!  *    Initialize global SSL context.
!  */
  static int
! initialize_SSL(PGconn *conn)
  {
! #ifndef WIN32
!     struct stat buf;
!     char        pwdbuf[BUFSIZ];
!     struct passwd pwdstr;
!     struct passwd *pwd = NULL;
!     char        fnbuf[2048];
! #endif

      if (!SSL_context)
      {
          SSL_library_init();
--- 837,888 ----
  #endif
  }

! #ifdef ENABLE_THREAD_SAFETY
!
! static unsigned long
! pq_threadidcallback(void)
! {
!     return (unsigned long)pthread_self();
! }
!
! static pthread_rwlock_t *pq_lockarray;
! static void
! pq_lockingcallback(int mode, int n, const char *file, int line)
! {
!     if (mode & CRYPTO_LOCK) {
!         pthread_rwlock_wrlock(&pq_lockarray[n]);
!     } else {
!         pthread_rwlock_unlock(&pq_lockarray[n]);
!     }
! }
!
! bool pq_usessllocks = true;
!
! #endif /* ENABLE_THRAD_SAFETY */
!
  static int
! init_ssl_system(PGconn *conn)
  {
! #ifdef ENABLE_THREAD_SAFETY
! static pthread_mutex_t init_mutex = PTHREAD_MUTEX_INITIALIZER;
!
!     pthread_mutex_lock(&init_mutex);
!
!     if (pq_usessllocks && pq_lockarray == NULL) {
!         int i;
!         CRYPTO_set_id_callback(pq_threadidcallback);
!
!         pq_lockarray = malloc(sizeof(pthread_rwlock_t)*CRYPTO_num_locks());
!         if (!pq_lockarray) {
!             pthread_mutex_unlock(&init_mutex);
!             return -1;
!         }
!         for (i=0;i<CRYPTO_num_locks();i++)
!             pthread_rwlock_init(&pq_lockarray[i], NULL);

+         CRYPTO_set_locking_callback(pq_lockingcallback);
+     }
+ #endif
      if (!SSL_context)
      {
          SSL_library_init();
***************
*** 840,851 ****
          SSL_context = SSL_CTX_new(TLSv1_method());
          if (!SSL_context)
          {
              printfPQExpBuffer(&conn->errorMessage,
                       libpq_gettext("could not create SSL context: %s\n"),
!                               SSLerrmessage());
              return -1;
          }
      }

  #ifndef WIN32
      if (pqGetpwuid(getuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd) == 0)
--- 890,927 ----
          SSL_context = SSL_CTX_new(TLSv1_method());
          if (!SSL_context)
          {
+             char *err = SSLerrmessage();
              printfPQExpBuffer(&conn->errorMessage,
                       libpq_gettext("could not create SSL context: %s\n"),
!                               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)
+ {
+ #ifndef WIN32
+     struct stat buf;
+     char        pwdbuf[BUFSIZ];
+     struct passwd pwdstr;
+     struct passwd *pwd = NULL;
+     char        fnbuf[2048];
+ #endif
+
+     if(!init_ssl_system(conn))
+         return -1;

  #ifndef WIN32
      if (pqGetpwuid(getuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd) == 0)
***************
*** 867,875 ****
          }
          if (!SSL_CTX_load_verify_locations(SSL_context, fnbuf, 0))
          {
              printfPQExpBuffer(&conn->errorMessage,
                                libpq_gettext("could not read root certificate list (%s): %s\n"),
!                               fnbuf, SSLerrmessage());
              return -1;
          }
      }
--- 943,953 ----
          }
          if (!SSL_CTX_load_verify_locations(SSL_context, fnbuf, 0))
          {
+             char *err = SSLerrmessage();
              printfPQExpBuffer(&conn->errorMessage,
                                libpq_gettext("could not read root certificate list (%s): %s\n"),
!                               fnbuf, err);
!             SSLerrfree(err);
              return -1;
          }
      }
***************
*** 936,945 ****
                      return PGRES_POLLING_FAILED;
                  }
              case SSL_ERROR_SSL:
!                 printfPQExpBuffer(&conn->errorMessage,
!                       libpq_gettext("SSL error: %s\n"), SSLerrmessage());
!                 close_SSL(conn);
!                 return PGRES_POLLING_FAILED;

              default:
                  printfPQExpBuffer(&conn->errorMessage,
--- 1014,1027 ----
                      return PGRES_POLLING_FAILED;
                  }
              case SSL_ERROR_SSL:
!                 {
!                     char *err = SSLerrmessage();
!                     printfPQExpBuffer(&conn->errorMessage,
!                           libpq_gettext("SSL error: %s\n"), err);
!                     SSLerrfree(err);
!                     close_SSL(conn);
!                     return PGRES_POLLING_FAILED;
!                 }

              default:
                  printfPQExpBuffer(&conn->errorMessage,
***************
*** 973,981 ****
      conn->peer = SSL_get_peer_certificate(conn->ssl);
      if (conn->peer == NULL)
      {
          printfPQExpBuffer(&conn->errorMessage,
                  libpq_gettext("certificate could not be obtained: %s\n"),
!                           SSLerrmessage());
          close_SSL(conn);
          return PGRES_POLLING_FAILED;
      }
--- 1055,1065 ----
      conn->peer = SSL_get_peer_certificate(conn->ssl);
      if (conn->peer == NULL)
      {
+         char *err = SSLerrmessage();
          printfPQExpBuffer(&conn->errorMessage,
                  libpq_gettext("certificate could not be obtained: %s\n"),
!                           err);
!         SSLerrfree(err);
          close_SSL(conn);
          return PGRES_POLLING_FAILED;
      }
***************
*** 1036,1058 ****
   * return NULL if it doesn't recognize the error code.  We don't
   * want to return NULL ever.
   */
! static const char *
  SSLerrmessage(void)
  {
      unsigned long errcode;
      const char *errreason;
!     static char errbuf[32];

      errcode = ERR_get_error();
!     if (errcode == 0)
!         return "No SSL error reported";
      errreason = ERR_reason_error_string(errcode);
!     if (errreason != NULL)
!         return errreason;
!     snprintf(errbuf, sizeof(errbuf), "SSL error code %lu", errcode);
      return errbuf;
  }

  /*
   *    Return pointer to SSL object.
   */
--- 1120,1159 ----
   * return NULL if it doesn't recognize the error code.  We don't
   * want to return NULL ever.
   */
! static char ssl_nomem[] = "Out of memory allocating error description";
! #define SSL_ERR_LEN    128
!
! static char *
  SSLerrmessage(void)
  {
      unsigned long errcode;
      const char *errreason;
!     char *errbuf;

+     errbuf = malloc(SSL_ERR_LEN);
+     if (!errbuf)
+         return ssl_nomem;
      errcode = ERR_get_error();
!     if (errcode == 0) {
!         strcpy(errbuf, "No SSL error reported");
!         return errbuf;
!     }
      errreason = ERR_reason_error_string(errcode);
!     if (errreason != NULL) {
!         strncpy(errbuf, errreason, SSL_ERR_LEN-1);
!         errbuf[SSL_ERR_LEN-1] = '\0';
!         return errbuf;
!     }
!     snprintf(errbuf, SSL_ERR_LEN, "SSL error code %lu", errcode);
      return errbuf;
  }

+ static void
+ SSLerrfree(char *buf)
+ {
+     if (buf != ssl_nomem)
+         free(buf);
+ }
  /*
   *    Return pointer to SSL object.
   */
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.102
diff -c -r1.102 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h    9 Jan 2004 02:02:43 -0000    1.102
--- src/interfaces/libpq/libpq-fe.h    14 Mar 2004 10:46:57 -0000
***************
*** 274,279 ****
--- 274,293 ----
                       PQnoticeProcessor proc,
                       void *arg);

+ /*
+  *     Used to set callback that prevents concurrent access to
+  *     non-thread safe functions that libpq needs.
+  *     The default implementation uses a libpq internal mutex.
+  *     Only required for multithreaded apps that use kerberos
+  *     both within their app and for postgresql connections.
+  */
+ typedef void (pgthreadlock_t)(int acquire);
+
+ extern pgthreadlock_t * PQregisterThreadLock(pgthreadlock_t *newhandler);
+
+ void
+ PQenableSSLLocks(int enable);
+
  /* === in fe-exec.c === */

  /* Simple synchronous query */
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.85
diff -c -r1.85 libpq-int.h
*** src/interfaces/libpq/libpq-int.h    5 Mar 2004 01:53:59 -0000    1.85
--- src/interfaces/libpq/libpq-int.h    14 Mar 2004 10:46:57 -0000
***************
*** 359,364 ****
--- 359,374 ----
  extern int pqPacketSend(PGconn *conn, char pack_type,
               const void *buf, size_t buf_len);

+ #ifdef ENABLE_THREAD_SAFETY
+ extern pgthreadlock_t *g_threadlock;
+ #define pglock_thread() g_threadlock(true);
+ #define pgunlock_thread() g_threadlock(false);
+ #else
+ #define pglock_thread() ((void)0)
+ #define pgunlock_thread() ((void)0)
+ #endif
+
+
  /* === in fe-exec.c === */

  extern void pqSetResultError(PGresult *res, const char *msg);
***************
*** 448,453 ****
--- 458,464 ----
  #ifdef ENABLE_THREAD_SAFETY
  extern void check_sigpipe_handler(void);
  extern pthread_key_t thread_in_send;
+ extern bool pq_usessllocks;
  #endif

  /*
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.37
diff -c -r1.37 fe-secure.c
*** src/interfaces/libpq/fe-secure.c    10 Feb 2004 15:21:24 -0000    1.37
--- src/interfaces/libpq/fe-secure.c    14 Mar 2004 08:31:48 -0000
***************
*** 1077,1096 ****
      pqsigfunc pipehandler;

      /*
       *    If the app hasn't set a SIGPIPE handler, define our own
       *    that ignores SIGPIPE on libpq send() and does SIG_DFL
       *    for other SIGPIPE cases.
       */
      pipehandler = pqsignalinquire(SIGPIPE);
      if (pipehandler == SIG_DFL)    /* not set by application */
-     {
-         /*
-          *    Create key first because the signal handler might be called
-          *    right after being installed.
-          */
-         pthread_key_create(&thread_in_send, NULL);
          pqsignal(SIGPIPE, sigpipe_handler_ignore_send);
-     }
  }

  /*
--- 1077,1096 ----
      pqsigfunc pipehandler;

      /*
+      *     Always create the key for SIGPIPE handling - PQinSend needs
+      *     it. Create it first because the signal handler might be called
+      *    right after being installed.
+      */
+     pthread_key_create(&thread_in_send, NULL);
+
+     /*
       *    If the app hasn't set a SIGPIPE handler, define our own
       *    that ignores SIGPIPE on libpq send() and does SIG_DFL
       *    for other SIGPIPE cases.
       */
      pipehandler = pqsignalinquire(SIGPIPE);
      if (pipehandler == SIG_DFL)    /* not set by application */
          pqsignal(SIGPIPE, sigpipe_handler_ignore_send);
  }

  /*