On Thu, Feb 18, 2021 at 11:04:05AM +0900, Michael Paquier wrote: > We have the code in place to properly initialize the crypto locking in > libpq with ENABLE_THREAD_SAFETY, but the root of the issue is that the > SSL and crypto initializations are grouped together. What we need to > do here is to split those phases of the initialization so as non-SSL > connections can use the crypto part properly, as pqsecure_initialize > gets only called now when libpq negotiates SSL with the postmaster. > It seems to me that we should make sure of a proper reset of the > crypto part within pqDropConnection(), while the initialization needs > to happen in PQconnectPoll().
So, I have tried a couple of things with a debug build of OpenSSL 1.0.2 at hand (two locks for the crypto and SSL initializations but SSL_new() grabs some random bytes that need the same callback to be set or the state of the threads is messed up, some global states to control the load), and the simplest solution I have come up with is to control in each pg_conn if the crypto callbacks have been initialized or not so as we avoid multiple inits and/or drops of the state for a single connection. I have arrived at this conclusion after hunting down cases with pqDropConnection() which would could be called multiple times, particularly if there are connection attempts to multiple hosts. 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). -- Michael
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index db71fea169..1c05fa44a8 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2882,6 +2882,13 @@ keep_going: /* We will come back to here until there is
}
#endif
+ /*
+ * Enable the crypto callbacks before checking if SSL needs
+ * to be done and before sending the startup packet.
+ */
+ if (pqsecure_initialize(conn, false, true) < 0)
+ goto error_return;
+
#ifdef USE_SSL
/*
@@ -2999,8 +3006,12 @@ 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, 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..8b6f860d22 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,16 +684,21 @@ 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 */
@@ -701,17 +706,20 @@ pgtls_init(PGconn *conn)
if (!ssl_lib_initialized)
{
- if (pq_init_ssl_lib)
+ if (do_ssl)
{
+ if (pq_init_ssl_lib)
+ {
#ifdef HAVE_OPENSSL_INIT_SSL
- OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL);
+ OPENSSL_init_ssl(OPENSSL_INIT_LOAD_CONFIG, NULL);
#else
- OPENSSL_config(NULL);
- SSL_library_init();
- SSL_load_error_strings();
+ OPENSSL_config(NULL);
+ SSL_library_init();
+ SSL_load_error_strings();
#endif
+ }
+ ssl_lib_initialized = true;
}
- ssl_lib_initialized = true;
}
#ifdef ENABLE_THREAD_SAFETY
@@ -740,10 +748,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
@@ -1400,48 +1408,68 @@ open_client_SSL(PGconn *conn)
void
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;
+ bool destroy_needed = false;
- SSL_shutdown(conn->ssl);
- SSL_free(conn->ssl);
- conn->ssl = NULL;
- conn->ssl_in_use = false;
- }
+ 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.
+ */
- if (conn->peer)
- {
- X509_free(conn->peer);
- conn->peer = NULL;
- }
+ SSL_shutdown(conn->ssl);
+ SSL_free(conn->ssl);
+ conn->ssl = NULL;
+ conn->ssl_in_use = false;
+
+ 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
- /*
- * 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.
- *
- * See comments above destroy_ssl_system().
- */
- if (destroy_needed)
- destroy_ssl_system();
+ /*
+ * This will remove our crypto 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.
+ *
+ * See comments above destroy_ssl_system().
+ */
+ if (destroy_needed && conn->crypto_loaded)
+ {
+ destroy_ssl_system();
+ conn->crypto_loaded = false;
+ }
+ }
+ else
+ {
+ /*
+ * In the non-SSL case, just remove the crypto callbacks if they
+ * are set. This will be done once this is the last connection
+ * to handle.
+ */
+ if (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 ce36aabd25..fd42885153 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -485,6 +485,12 @@ 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 */
@@ -682,7 +688,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);
@@ -715,7 +721,7 @@ extern void pgtls_init_library(bool do_ssl, int do_crypto);
*
* 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
