Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit b37c90f

Browse files
committed
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.
1 parent b882246 commit b37c90f

File tree

1 file changed

+22
-2
lines changed

1 file changed

+22
-2
lines changed

src/interfaces/libpq/fe-secure.c

+22-2
Original file line numberDiff line numberDiff line change
@@ -282,10 +282,11 @@ pqsecure_open_client(PGconn *conn)
282282
libpq_gettext("could not establish SSL connection: %s\n"),
283283
err);
284284
SSLerrfree(err);
285-
close_SSL(conn);
286285
#ifdef ENABLE_THREAD_SAFETY
287286
pthread_mutex_unlock(&ssl_config_mutex);
288287
#endif
288+
close_SSL(conn);
289+
289290
return PGRES_POLLING_FAILED;
290291
}
291292
#ifdef ENABLE_THREAD_SAFETY
@@ -1537,15 +1538,23 @@ open_client_SSL(PGconn *conn)
15371538
static void
15381539
close_SSL(PGconn *conn)
15391540
{
1541+
bool destroy_needed = false;
1542+
15401543
if (conn->ssl)
15411544
{
15421545
DECLARE_SIGPIPE_INFO(spinfo);
15431546

1547+
/*
1548+
* We can't destroy everything SSL-related here due to the possible
1549+
* later calls to OpenSSL routines which may need our thread
1550+
* callbacks, so set a flag here and check at the end.
1551+
*/
1552+
destroy_needed = true;
1553+
15441554
DISABLE_SIGPIPE(conn, spinfo, (void) 0);
15451555
SSL_shutdown(conn->ssl);
15461556
SSL_free(conn->ssl);
15471557
conn->ssl = NULL;
1548-
pqsecure_destroy();
15491558
/* We have to assume we got EPIPE */
15501559
REMEMBER_EPIPE(spinfo, true);
15511560
RESTORE_SIGPIPE(conn, spinfo);
@@ -1565,6 +1574,17 @@ close_SSL(PGconn *conn)
15651574
conn->engine = NULL;
15661575
}
15671576
#endif
1577+
1578+
/*
1579+
* This will remove our SSL locking hooks, if this is the last SSL
1580+
* connection, which means we must wait to call it until after all
1581+
* SSL calls have been made, otherwise we can end up with a race
1582+
* condition and possible deadlocks.
1583+
*
1584+
* See comments above destroy_ssl_system().
1585+
*/
1586+
if (destroy_needed)
1587+
pqsecure_destroy();
15681588
}
15691589

15701590
/*

0 commit comments

Comments
 (0)