[Asterisk-code-review] res config pgsql.c: Fix deadlock loading realtime configurat... (asterisk[11])

[Joshua Colp]

Joshua Colp has submitted this change and it was merged.

Change subject: res_config_pgsql.c: Fix deadlock loading realtime configuration.
......................................................................


res_config_pgsql.c: Fix deadlock loading realtime configuration.

On v13, loading several thousand PJSIP endpoints on Asterisk start causes
a deadlock most of the time.

Thanks to mdu113 for discovering that there was a call to pgsql_exec() not
protected by the pgsql_lock reentrancy lock.

{quote}
I believe a code path exists that attempts to use pgsql connection without
locking pgsql_lock.  I believe what happens during that deadlock that I
see is two concurrent threads are both attempting to send query to pgsql,
one of the thread is using a code path without locking pgsql_lock.  If
they managed to send queries at the same time, it seems postgres ignores
one of the queries and replies only to the one of them.  If it happens so
that the thread holding the lock didn't receive the reply it will wait for
it (and hold the lock) forever (or at least for very long time), thus
completely blocking all access to db.
{quote}

* Added missing reentrancy locking around pgsql_exec() in find_table().

* Moved unlock of pgsql_lock in unload_module() to avoid locking inversion
between the psql_tables list lock and the pgsql_lock.

ASTERISK-25455 #close
Reported by:  mdu113
Patches:
      res_config_pgsql.c-connlock2.diff (license #5543) patch uploaded by mdu113

Change-Id: Id9e7cdf8a3b65ff19964b0cf942ace567938c4e2
---
M res/res_config_pgsql.c
1 file changed, 5 insertions(+), 3 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  Richard Mudgett: Looks good to me, but someone else must approve
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/res/res_config_pgsql.c b/res/res_config_pgsql.c
index be2d4ed..bce7394 100644
--- a/res/res_config_pgsql.c
+++ b/res/res_config_pgsql.c
@@ -335,7 +335,9 @@
 		ast_str_set(&sql, 0, "SELECT a.attname, t.typname, a.attlen, a.attnotnull, d.adsrc, a.atttypmod FROM pg_class c, pg_type t, pg_attribute a LEFT OUTER JOIN pg_attrdef d ON a.atthasdef AND d.adrelid = a.attrelid AND d.adnum = a.attnum WHERE c.oid = a.attrelid AND a.atttypid = t.oid AND (a.attnum > 0) AND c.relname = '%s' ORDER BY c.relname, attnum", orig_tablename);
 	}
 
+	ast_mutex_lock(&pgsql_lock);
 	exec_result = pgsql_exec(database, orig_tablename, ast_str_buffer(sql), &result);
+	ast_mutex_unlock(&pgsql_lock);
 	ast_debug(1, "Query of table structure complete.  Now retrieving results.\n");
 	if (exec_result != 0) {
 		ast_log(LOG_ERROR, "Failed to query database columns for table %s\n", orig_tablename);
@@ -1371,15 +1373,15 @@
 	ast_config_engine_deregister(&pgsql_engine);
 	ast_verb(1, "PostgreSQL RealTime unloaded.\n");
 
+	/* Unlock so something else can destroy the lock. */
+	ast_mutex_unlock(&pgsql_lock);
+
 	/* Destroy cached table info */
 	AST_LIST_LOCK(&psql_tables);
 	while ((table = AST_LIST_REMOVE_HEAD(&psql_tables, list))) {
 		destroy_table(table);
 	}
 	AST_LIST_UNLOCK(&psql_tables);
-
-	/* Unlock so something else can destroy the lock. */
-	ast_mutex_unlock(&pgsql_lock);
 
 	return 0;
 }

-- 
To view, visit https://gerrit.asterisk.org/1439
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Id9e7cdf8a3b65ff19964b0cf942ace567938c4e2
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>