On Fri, Feb 19, 2021 at 02:37:06PM +0900, Michael Paquier wrote: > The attached patch implements things this way, and initializes the > crypto callbacks before sending the startup packet, before deciding if > SSL needs to be requested or not. I have played with several > threading scenarios with this patch, with and without OpenSSL, and the > numbers match in terms of callback loading and unloading (the global > counter used in fe-secure-openssl.c gets to zero).
I have done more work and much more tests with this patch, polishing things as of the attached v2. First, I don't see any performance impact or concurrency issues, using up to 200 threads with pgbench -C -n -j N -c N -f blah.sql where the SQL file includes a single meta-command like that for instance: \set a 1 This ensures that connection requests happen a maximum in concurrency, and libpq stays close to the maximum for the number of open threads. Attached is a second, simple program that I have used to stress the case of threads using both SSL and non-SSL connections in parallel to check for the consistency of the callbacks and their release, mainly across MD5 and SCRAM. Extra eyes are welcome here, though I feel comfortable with the approach taken here. -- Michael
/*
* To compile:
* gcc -o pthread_libpq pthread_libpq.c -lpthread -lpq
*/
#include <pthread.h>
#include <stdio.h>
#include "libpq-fe.h"
#define NUM_THREADS 100
#define NUM_LOOPS 100
/* this function is run by the second thread */
void *conn_thread_func(void *num_ptr)
{
/* increment x to 100 */
int num_thread;
const char *conninfo_nossl = "host=localhost sslmode=disable";
const char *conninfo_ssl = "host=localhost sslmode=require";
const char *conninfo;
PGconn *conn;
int count;
num_thread = *((int *) num_ptr);
/* Choose SSL or not SSL. This is let random, on purpose :) */
if (num_thread < NUM_THREADS / 2)
conninfo = conninfo_nossl;
else
conninfo = conninfo_ssl;
for (count = 0; count < NUM_LOOPS; count++)
{
conn = PQconnectdb(conninfo);
if (PQstatus(conn) != CONNECTION_OK)
{
fprintf(stderr, "connection on loop %d failed: %s",
count, PQerrorMessage(conn));
return NULL;
}
/* close the connection */
PQfinish(conn);
}
/* the function must return something - NULL will do */
return NULL;
}
int main()
{
int count = 0;
/* this variable is the reference to the other 100 threads */
pthread_t conn_thread[NUM_THREADS];
if (PQisthreadsafe() == 0)
{
fprintf(stderr, "libpq is not thread-safe\n");
return 1;
}
else
fprintf(stderr, "libpq is thread safe\n");
/* create a second thread which executes inc_x(&x) */
for (count = 0; count < NUM_THREADS; count++)
{
if (pthread_create(&conn_thread[count], NULL, conn_thread_func, &count))
{
fprintf(stderr, "Error creating thread\n");
return 1;
}
}
/* wait for the other threads to finish, in sync fashion */
for (count = 0; count < NUM_THREADS; count++)
{
if (pthread_join(conn_thread[count], NULL))
{
fprintf(stderr, "Error joining thread\n");
return 1;
}
}
/* all done */
return 0;
}
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 9812a14662..f23942ab2b 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2918,6 +2918,16 @@ keep_going: /* We will come back to here until there is
#ifdef USE_SSL
+ /*
+ * Enable the libcrypto callbacks before checking if SSL needs
+ * to be done. This is done before sending the startup packet
+ * as depending on the type of authentication done, like MD5
+ * or SCRAM, the callbacks would be required even without a
+ * SSL connection
+ */
+ if (pqsecure_initialize(conn, false, true) < 0)
+ goto error_return;
+
/*
* If SSL is enabled and we haven't already got encryption of
* some sort running, request SSL instead of sending the
@@ -3033,8 +3043,14 @@ keep_going: /* We will come back to here until there is
{
/* mark byte consumed */
conn->inStart = conn->inCursor;
- /* Set up global SSL state if required */
- if (pqsecure_initialize(conn) != 0)
+
+ /*
+ * Set up global SSL state if required. The crypto
+ * state has already been set if libpq took care of
+ * doing that, so there is no need to make that happen
+ * again.
+ */
+ if (pqsecure_initialize(conn, true, false) != 0)
goto error_return;
}
else if (SSLok == 'N')
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index 0fa10a23b4..dbac824702 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -85,7 +85,7 @@ static bool pq_init_crypto_lib = true;
static bool ssl_lib_initialized = false;
#ifdef ENABLE_THREAD_SAFETY
-static long ssl_open_connections = 0;
+static long crypto_open_connections = 0;
#ifndef WIN32
static pthread_mutex_t ssl_config_mutex = PTHREAD_MUTEX_INITIALIZER;
@@ -111,7 +111,7 @@ pgtls_init_library(bool do_ssl, int do_crypto)
* Disallow changing the flags while we have open connections, else we'd
* get completely confused.
*/
- if (ssl_open_connections != 0)
+ if (crypto_open_connections != 0)
return;
#endif
@@ -635,7 +635,7 @@ pq_lockingcallback(int mode, int n, const char *file, int line)
* override it.
*/
int
-pgtls_init(PGconn *conn)
+pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto)
{
#ifdef ENABLE_THREAD_SAFETY
#ifdef WIN32
@@ -684,22 +684,28 @@ pgtls_init(PGconn *conn)
}
}
- if (ssl_open_connections++ == 0)
+ if (do_crypto && !conn->crypto_loaded)
{
- /*
- * These are only required for threaded libcrypto applications,
- * but make sure we don't stomp on them if they're already set.
- */
- if (CRYPTO_get_id_callback() == NULL)
- CRYPTO_set_id_callback(pq_threadidcallback);
- if (CRYPTO_get_locking_callback() == NULL)
- CRYPTO_set_locking_callback(pq_lockingcallback);
+ if (crypto_open_connections++ == 0)
+ {
+ /*
+ * These are only required for threaded libcrypto
+ * applications, but make sure we don't stomp on them if
+ * they're already set.
+ */
+ if (CRYPTO_get_id_callback() == NULL)
+ CRYPTO_set_id_callback(pq_threadidcallback);
+ if (CRYPTO_get_locking_callback() == NULL)
+ CRYPTO_set_locking_callback(pq_lockingcallback);
+ }
+
+ conn->crypto_loaded = true;
}
}
#endif /* HAVE_CRYPTO_LOCK */
#endif /* ENABLE_THREAD_SAFETY */
- if (!ssl_lib_initialized)
+ if (!ssl_lib_initialized && do_ssl)
{
if (pq_init_ssl_lib)
{
@@ -740,10 +746,10 @@ destroy_ssl_system(void)
if (pthread_mutex_lock(&ssl_config_mutex))
return;
- if (pq_init_crypto_lib && ssl_open_connections > 0)
- --ssl_open_connections;
+ if (pq_init_crypto_lib && crypto_open_connections > 0)
+ --crypto_open_connections;
- if (pq_init_crypto_lib && ssl_open_connections == 0)
+ if (pq_init_crypto_lib && crypto_open_connections == 0)
{
/*
* No connections left, unregister libcrypto callbacks, if no one
@@ -1402,46 +1408,61 @@ pgtls_close(PGconn *conn)
{
bool destroy_needed = false;
- if (conn->ssl)
+ if (conn->ssl_in_use)
{
- /*
- * We can't destroy everything SSL-related here due to the possible
- * later calls to OpenSSL routines which may need our thread
- * callbacks, so set a flag here and check at the end.
- */
- destroy_needed = true;
+ if (conn->ssl)
+ {
+ /*
+ * We can't destroy everything SSL-related here due to the
+ * possible later calls to OpenSSL routines which may need our
+ * thread callbacks, so set a flag here and check at the end.
+ */
- SSL_shutdown(conn->ssl);
- SSL_free(conn->ssl);
- conn->ssl = NULL;
- conn->ssl_in_use = false;
- }
+ SSL_shutdown(conn->ssl);
+ SSL_free(conn->ssl);
+ conn->ssl = NULL;
+ conn->ssl_in_use = false;
- if (conn->peer)
- {
- X509_free(conn->peer);
- conn->peer = NULL;
- }
+ destroy_needed = true;
+ }
+
+ if (conn->peer)
+ {
+ X509_free(conn->peer);
+ conn->peer = NULL;
+ }
#ifdef USE_SSL_ENGINE
- if (conn->engine)
- {
- ENGINE_finish(conn->engine);
- ENGINE_free(conn->engine);
- conn->engine = NULL;
- }
+ if (conn->engine)
+ {
+ ENGINE_finish(conn->engine);
+ ENGINE_free(conn->engine);
+ conn->engine = NULL;
+ }
#endif
+ }
+ else
+ {
+ /*
+ * In the non-SSL case, just remove the crypto callbacks. This code
+ * path has no dependency on any pending SSL calls.
+ */
+ destroy_needed = true;
+ }
/*
- * This will remove our SSL locking hooks, if this is the last SSL
- * connection, which means we must wait to call it until after all SSL
- * calls have been made, otherwise we can end up with a race condition and
- * possible deadlocks.
+ * This will remove our crypto locking hooks if this is the last
+ * connection using libcrypto which means we must wait to call it until
+ * after all the potential SSL calls have been made, otherwise we can end
+ * up with a race condition and possible deadlocks.
*
* See comments above destroy_ssl_system().
*/
- if (destroy_needed)
+ if (destroy_needed && conn->crypto_loaded)
+ {
destroy_ssl_system();
+ conn->crypto_loaded = false;
+ }
}
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index c601071838..b15d8d137c 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -159,12 +159,12 @@ PQinitOpenSSL(int do_ssl, int do_crypto)
* Initialize global SSL context
*/
int
-pqsecure_initialize(PGconn *conn)
+pqsecure_initialize(PGconn *conn, bool do_ssl, bool do_crypto)
{
int r = 0;
#ifdef USE_SSL
- r = pgtls_init(conn);
+ r = pgtls_init(conn, do_ssl, do_crypto);
#endif
return r;
@@ -191,8 +191,7 @@ void
pqsecure_close(PGconn *conn)
{
#ifdef USE_SSL
- if (conn->ssl_in_use)
- pgtls_close(conn);
+ pgtls_close(conn);
#endif
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 0c9e95f1a7..b045915e6d 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -506,6 +506,11 @@ struct pg_conn
void *engine; /* dummy field to keep struct the same if
* OpenSSL version changes */
#endif
+ bool crypto_loaded; /* Track if libcrypto locking callbacks have
+ * been done for this connection. This can be
+ * removed once support for OpenSSL 1.0.2 is
+ * removed as this this locking is handled
+ * internally in OpenSSL >= 1.1.0. */
#endif /* USE_OPENSSL */
#endif /* USE_SSL */
@@ -703,7 +708,7 @@ extern int pqWriteReady(PGconn *conn);
/* === in fe-secure.c === */
-extern int pqsecure_initialize(PGconn *);
+extern int pqsecure_initialize(PGconn *, bool, bool);
extern PostgresPollingStatusType pqsecure_open_client(PGconn *);
extern void pqsecure_close(PGconn *);
extern ssize_t pqsecure_read(PGconn *, void *ptr, size_t len);
@@ -732,11 +737,13 @@ extern void pgtls_init_library(bool do_ssl, int do_crypto);
* Initialize SSL library.
*
* The conn parameter is only used to be able to pass back an error
- * message - no connection-local setup is made here.
+ * message - no connection-local setup is made here. do_ssl controls
+ * if SSL is initialized, and do_crypto does the same for the crypto
+ * part.
*
* Returns 0 if OK, -1 on failure (adding a message to conn->errorMessage).
*/
-extern int pgtls_init(PGconn *conn);
+extern int pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto);
/*
* Begin or continue negotiating a secure session.
signature.asc
Description: PGP signature
