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();
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
