From d707a8ccd87d2a3d307c7bd19b1d30d3cb9b3ca5 Mon Sep 17 00:00:00 2001 From: Stephen Frost Date: Mon, 23 Sep 2013 08:33:41 -0400 Subject: [PATCH] Fix SSL deadlock risk in libpq In libpq, we set up and pass to OpenSSL callback routines to handle locking. When we run out of SSL connections, we try to clean things up by de-registering the hooks. Unfortunately, we had a few calls into the OpenSSL library after these hooks were de-registered during SSL cleanup which lead to deadlocking. This moves the thread callback cleanup to be after all SSL-cleanup related OpenSSL library calls. I've been unable to reproduce the deadlock with this fix. In passing, also move the close_SSL call to be after unlocking our ssl_config mutex when in a failure state. While it looks pretty unlikely to be an issue, it could have resulted in deadlocks if we ended up in this code path due to something other than SSL_new failing. Thanks to Heikki for pointing this out. Back-patch to all supported versions; note that the close_SSL issue only goes back to 9.0, so that hunk isn't included in the 8.4 patch. Initially found and reported by Vesa-Matti J Kari; many thanks to both Heikki and Andres for their help running down the specific issue and reviewing the patch. --- src/interfaces/libpq/fe-secure.c | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c index 2ff229c683..417e44a961 100644 --- a/src/interfaces/libpq/fe-secure.c +++ b/src/interfaces/libpq/fe-secure.c @@ -283,10 +283,11 @@ pqsecure_open_client(PGconn *conn) libpq_gettext("could not establish SSL connection: %s\n"), err); SSLerrfree(err); - close_SSL(conn); #ifdef ENABLE_THREAD_SAFETY pthread_mutex_unlock(&ssl_config_mutex); #endif + close_SSL(conn); + return PGRES_POLLING_FAILED; } #ifdef ENABLE_THREAD_SAFETY @@ -1538,15 +1539,23 @@ open_client_SSL(PGconn *conn) static void close_SSL(PGconn *conn) { + bool destroy_needed = false; + if (conn->ssl) { DECLARE_SIGPIPE_INFO(spinfo); + /* + * 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; + DISABLE_SIGPIPE(conn, spinfo, (void) 0); SSL_shutdown(conn->ssl); SSL_free(conn->ssl); conn->ssl = NULL; - pqsecure_destroy(); /* We have to assume we got EPIPE */ REMEMBER_EPIPE(spinfo, true); RESTORE_SIGPIPE(conn, spinfo); @@ -1566,6 +1575,17 @@ close_SSL(PGconn *conn) 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) + pqsecure_destroy(); } /* -- 2.40.0