[Asterisk-code-review] res config pgsql: Fix thread safety problems (asterisk[14])

Anonymous Coward asteriskteam at digium.com
Fri Feb 24 10:12:01 CST 2017


Anonymous Coward #1000019 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5077 )

Change subject: res_config_pgsql: Fix thread safety problems
......................................................................


res_config_pgsql: Fix thread safety problems

* A missing AST_LIST_UNLOCK() in find_table()

* The ESCAPE_STRING() macro uses pgsqlConn under the hood and we were
  not consistently locking before calling it.

* There were a handful of other places where pgsqlConn was accessed
  directly without appropriate locking.

Change-Id: Iea63f0728f76985a01e95b9912c3c5c6065836ed
---
M res/res_config_pgsql.c
1 file changed, 101 insertions(+), 37 deletions(-)

Approvals:
  George Joseph: Looks good to me, approved
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, but someone else must approve



diff --git a/res/res_config_pgsql.c b/res/res_config_pgsql.c
index 325dca8..2c51a53 100644
--- a/res/res_config_pgsql.c
+++ b/res/res_config_pgsql.c
@@ -270,6 +270,7 @@
 	}
 
 	if (database == NULL) {
+		AST_LIST_UNLOCK(&psql_tables);
 		return NULL;
 	}
 
@@ -411,6 +412,16 @@
 		return NULL;
 	}
 
+	/*
+	 * Must connect to the server before anything else as ESCAPE_STRING()
+	 * uses pgsqlConn
+	 */
+	ast_mutex_lock(&pgsql_lock);
+	if (!pgsql_reconnect(database)) {
+		ast_mutex_unlock(&pgsql_lock);
+		return NULL;
+	}
+
 	/* Get the first parameter and first value in our list of passed paramater/value pairs */
 	if (!field) {
 		ast_log(LOG_WARNING,
@@ -419,6 +430,7 @@
 			PQfinish(pgsqlConn);
 			pgsqlConn = NULL;
 		}
+		ast_mutex_unlock(&pgsql_lock);
 		return NULL;
 	}
 
@@ -436,6 +448,7 @@
 	ESCAPE_STRING(escapebuf, field->value);
 	if (pgresult) {
 		ast_log(LOG_ERROR, "PostgreSQL RealTime: detected invalid input: '%s'\n", field->value);
+		ast_mutex_unlock(&pgsql_lock);
 		return NULL;
 	}
 
@@ -454,6 +467,7 @@
 		ESCAPE_STRING(escapebuf, field->value);
 		if (pgresult) {
 			ast_log(LOG_ERROR, "PostgreSQL RealTime: detected invalid input: '%s'\n", field->value);
+			ast_mutex_unlock(&pgsql_lock);
 			return NULL;
 		}
 
@@ -461,8 +475,6 @@
 	}
 
 	/* We now have our complete statement; Lets connect to the server and execute it. */
-	ast_mutex_lock(&pgsql_lock);
-
         if (pgsql_exec(database, tablename, ast_str_buffer(sql), &result) != 0) {
 		ast_mutex_unlock(&pgsql_lock);
 		return NULL;
@@ -542,6 +554,16 @@
 	if (!(cfg = ast_config_new()))
 		return NULL;
 
+	/*
+	 * Must connect to the server before anything else as ESCAPE_STRING()
+	 * uses pgsqlConn
+	 */
+	ast_mutex_lock(&pgsql_lock);
+	if (!pgsql_reconnect(database)) {
+		ast_mutex_unlock(&pgsql_lock);
+		return NULL;
+	}
+
 	/* Get the first parameter and first value in our list of passed paramater/value pairs */
 	if (!field) {
 		ast_log(LOG_WARNING,
@@ -550,6 +572,7 @@
 			PQfinish(pgsqlConn);
 			pgsqlConn = NULL;
 		}
+		ast_mutex_unlock(&pgsql_lock);
 		ast_config_destroy(cfg);
 		return NULL;
 	}
@@ -575,6 +598,7 @@
 	ESCAPE_STRING(escapebuf, field->value);
 	if (pgresult) {
 		ast_log(LOG_ERROR, "PostgreSQL RealTime: detected invalid input: '%s'\n", field->value);
+		ast_mutex_unlock(&pgsql_lock);
 		ast_config_destroy(cfg);
 		return NULL;
 	}
@@ -595,6 +619,7 @@
 		ESCAPE_STRING(escapebuf, field->value);
 		if (pgresult) {
 			ast_log(LOG_ERROR, "PostgreSQL RealTime: detected invalid input: '%s'\n", field->value);
+			ast_mutex_unlock(&pgsql_lock);
 			ast_config_destroy(cfg);
 			return NULL;
 		}
@@ -606,10 +631,7 @@
 		ast_str_append(&sql, 0, " ORDER BY %s", initfield);
 	}
 
-
 	/* We now have our complete statement; Lets connect to the server and execute it. */
-	ast_mutex_lock(&pgsql_lock);
-
 	if (pgsql_exec(database, table, ast_str_buffer(sql), &result) != 0) {
 		ast_mutex_unlock(&pgsql_lock);
 		ast_config_destroy(cfg);
@@ -706,6 +728,16 @@
 		return -1;
 	}
 
+	/*
+	 * Must connect to the server before anything else as ESCAPE_STRING()
+	 * uses pgsqlConn
+	 */
+	ast_mutex_lock(&pgsql_lock);
+	if (!pgsql_reconnect(database)) {
+		ast_mutex_unlock(&pgsql_lock);
+		return -1;
+	}
+
 	/* Get the first parameter and first value in our list of passed paramater/value pairs */
 	if (!field) {
 		ast_log(LOG_WARNING,
@@ -714,6 +746,7 @@
 			PQfinish(pgsqlConn);
 			pgsqlConn = NULL;
 		}
+		ast_mutex_unlock(&pgsql_lock);
 		release_table(table);
 		return -1;
 	}
@@ -727,6 +760,7 @@
 
 	if (!column) {
 		ast_log(LOG_ERROR, "PostgreSQL RealTime: Updating on column '%s', but that column does not exist within the table '%s'!\n", field->name, tablename);
+		ast_mutex_unlock(&pgsql_lock);
 		release_table(table);
 		return -1;
 	}
@@ -737,6 +771,7 @@
 	ESCAPE_STRING(escapebuf, field->value);
 	if (pgresult) {
 		ast_log(LOG_ERROR, "PostgreSQL RealTime: detected invalid input: '%s'\n", field->value);
+		ast_mutex_unlock(&pgsql_lock);
 		release_table(table);
 		return -1;
 	}
@@ -751,6 +786,7 @@
 		ESCAPE_STRING(escapebuf, field->value);
 		if (pgresult) {
 			ast_log(LOG_ERROR, "PostgreSQL RealTime: detected invalid input: '%s'\n", field->value);
+			ast_mutex_unlock(&pgsql_lock);
 			release_table(table);
 			return -1;
 		}
@@ -762,6 +798,7 @@
 	ESCAPE_STRING(escapebuf, lookup);
 	if (pgresult) {
 		ast_log(LOG_ERROR, "PostgreSQL RealTime: detected invalid input: '%s'\n", lookup);
+		ast_mutex_unlock(&pgsql_lock);
 		return -1;
 	}
 
@@ -770,8 +807,6 @@
 	ast_debug(1, "PostgreSQL RealTime: Update SQL: %s\n", ast_str_buffer(sql));
 
 	/* We now have our complete statement; Lets connect to the server and execute it. */
-	ast_mutex_lock(&pgsql_lock);
-
 	if (pgsql_exec(database, tablename, ast_str_buffer(sql), &result) != 0) {
 		ast_mutex_unlock(&pgsql_lock);
 		return -1;
@@ -838,12 +873,23 @@
 		return -1;
 	}
 
+	/*
+	 * Must connect to the server before anything else as ESCAPE_STRING()
+	 * uses pgsqlConn
+	 */
+	ast_mutex_lock(&pgsql_lock);
+	if (!pgsql_reconnect(database)) {
+		ast_mutex_unlock(&pgsql_lock);
+		return -1;
+	}
+
 	ast_str_set(&sql, 0, "UPDATE %s SET", tablename);
 	ast_str_set(&where, 0, " WHERE");
 
 	for (field = lookup_fields; field; field = field->next) {
 		if (!find_column(table, field->name)) {
 			ast_log(LOG_ERROR, "Attempted to update based on criteria column '%s' (%s@%s), but that column does not exist!\n", field->name, tablename, database);
+			ast_mutex_unlock(&pgsql_lock);
 			release_table(table);
 			return -1;
 		}
@@ -851,6 +897,7 @@
 		ESCAPE_STRING(escapebuf, field->value);
 		if (pgresult) {
 			ast_log(LOG_ERROR, "PostgreSQL RealTime: detected invalid input: '%s'\n", field->value);
+			ast_mutex_unlock(&pgsql_lock);
 			release_table(table);
 			return -1;
 		}
@@ -865,6 +912,7 @@
 			PQfinish(pgsqlConn);
 			pgsqlConn = NULL;
 		}
+		ast_mutex_unlock(&pgsql_lock);
 		release_table(table);
 		return -1;
 	}
@@ -881,6 +929,7 @@
 		ESCAPE_STRING(escapebuf, field->value);
 		if (pgresult) {
 			ast_log(LOG_ERROR, "PostgreSQL RealTime: detected invalid input: '%s'\n", field->value);
+			ast_mutex_unlock(&pgsql_lock);
 			release_table(table);
 			return -1;
 		}
@@ -894,7 +943,7 @@
 
 	ast_debug(1, "PostgreSQL RealTime: Update SQL: %s\n", ast_str_buffer(sql));
 
-	/* We now have our complete statement; connect to the server and execute it. */
+	/* We now have our complete statement; Lets connect to the server and execute it. */
         if (pgsql_exec(database, tablename, ast_str_buffer(sql), &result) != 0) {
 		ast_mutex_unlock(&pgsql_lock);
 	        return -1;
@@ -939,6 +988,16 @@
 		return -1;
 	}
 
+	/*
+	 * Must connect to the server before anything else as ESCAPE_STRING()
+	 * uses pgsqlConn
+	 */
+	ast_mutex_lock(&pgsql_lock);
+	if (!pgsql_reconnect(database)) {
+		ast_mutex_unlock(&pgsql_lock);
+		return -1;
+	}
+
 	/* Get the first parameter and first value in our list of passed paramater/value pairs */
 	if (!field) {
 		ast_log(LOG_WARNING,
@@ -947,12 +1006,6 @@
 			PQfinish(pgsqlConn);
 			pgsqlConn = NULL;
 		}
-		return -1;
-	}
-
-	/* Must connect to the server before anything else, as the escape function requires the connection handle.. */
-	ast_mutex_lock(&pgsql_lock);
-	if (!pgsql_reconnect(database)) {
 		ast_mutex_unlock(&pgsql_lock);
 		return -1;
 	}
@@ -973,6 +1026,7 @@
 
 	ast_debug(1, "PostgreSQL RealTime: Insert SQL: %s\n", ast_str_buffer(sql1));
 
+	/* We now have our complete statement; Lets connect to the server and execute it. */
         if (pgsql_exec(database, table, ast_str_buffer(sql1), &result) != 0) {
 		ast_mutex_unlock(&pgsql_lock);
 	        return -1;
@@ -1016,27 +1070,27 @@
 		return -1;
 	}
 
-	/* Get the first parameter and first value in our list of passed paramater/value pairs */
-	/*newparam = va_arg(ap, const char *);
-	newval = va_arg(ap, const char *);
-	if (!newparam || !newval) {*/
-	if (ast_strlen_zero(keyfield) || ast_strlen_zero(lookup))  {
-		ast_log(LOG_WARNING,
-				"PostgreSQL RealTime: Realtime destroy requires at least 1 parameter and 1 value to search on.\n");
-		if (pgsqlConn) {
-			PQfinish(pgsqlConn);
-			pgsqlConn = NULL;
-		};
-		return -1;
-	}
-
-	/* Must connect to the server before anything else, as the escape function requires the connection handle.. */
+	/*
+	 * Must connect to the server before anything else as ESCAPE_STRING()
+	 * uses pgsqlConn
+	 */
 	ast_mutex_lock(&pgsql_lock);
 	if (!pgsql_reconnect(database)) {
 		ast_mutex_unlock(&pgsql_lock);
 		return -1;
 	}
 
+	/* Get the first parameter and first value in our list of passed paramater/value pairs */
+	if (ast_strlen_zero(keyfield) || ast_strlen_zero(lookup))  {
+		ast_log(LOG_WARNING,
+				"PostgreSQL RealTime: Realtime destroy requires at least 1 parameter and 1 value to search on.\n");
+		if (pgsqlConn) {
+			PQfinish(pgsqlConn);
+			pgsqlConn = NULL;
+		}
+		ast_mutex_unlock(&pgsql_lock);
+		return -1;
+	}
 
 	/* Create the first part of the query using the first parameter/value pairs we just extracted
 	   If there is only 1 set, then we have our query. Otherwise, loop thru the list and concat */
@@ -1052,10 +1106,11 @@
 
 	ast_debug(1, "PostgreSQL RealTime: Delete SQL: %s\n", ast_str_buffer(sql));
 
-        if (pgsql_exec(database, table, ast_str_buffer(sql), &result) != 0) {
+	/* We now have our complete statement; Lets connect to the server and execute it. */
+	if (pgsql_exec(database, table, ast_str_buffer(sql), &result) != 0) {
 		ast_mutex_unlock(&pgsql_lock);
-	        return -1;
-        }
+		return -1;
+	}
 
 	numrows = atoi(PQcmdTuples(result));
 	ast_mutex_unlock(&pgsql_lock);
@@ -1268,7 +1323,7 @@
 				ast_debug(1, "About to run ALTER query on table '%s' to add column '%s'\n", tablename, elm);
 
 			        if (pgsql_exec(database, tablename, ast_str_buffer(sql), &result) != 0) {
-						ast_mutex_unlock(&pgsql_lock);
+					ast_mutex_unlock(&pgsql_lock);
 				        return -1;
 			        }
 
@@ -1395,6 +1450,7 @@
 
 	ast_mutex_lock(&pgsql_lock);
 
+	/* XXX: Why would we do this before we're ready to establish a new connection? */
 	if (pgsqlConn) {
 		PQfinish(pgsqlConn);
 		pgsqlConn = NULL;
@@ -1502,13 +1558,18 @@
 
 	/* mutex lock should have been locked before calling this function. */
 
-	if (pgsqlConn && PQstatus(pgsqlConn) != CONNECTION_OK) {
+	if (pgsqlConn) {
+		if (PQstatus(pgsqlConn) == CONNECTION_OK) {
+			/* We're good? */
+			return 1;
+		}
+
 		PQfinish(pgsqlConn);
 		pgsqlConn = NULL;
 	}
 
 	/* DB password can legitimately be 0-length */
-	if ((!pgsqlConn) && (!ast_strlen_zero(dbhost) || !ast_strlen_zero(dbsock)) && !ast_strlen_zero(dbuser) && !ast_strlen_zero(my_database)) {
+	if ((!ast_strlen_zero(dbhost) || !ast_strlen_zero(dbsock)) && !ast_strlen_zero(dbuser) && !ast_strlen_zero(my_database)) {
 		struct ast_str *conn_info = ast_str_create(128);
 
 		if (!conn_info) {
@@ -1608,7 +1669,7 @@
 	char connection_info[256];
 	char credentials[100] = "";
 	char buf[376]; /* 256+100+"Connected to "+" for "+NULL */
-	int ctimesec = time(NULL) - connect_time;
+	int is_connected = 0, ctimesec = time(NULL) - connect_time;
 
 	switch (cmd) {
 	case CLI_INIT:
@@ -1634,8 +1695,11 @@
 	if (!ast_strlen_zero(dbuser))
 		snprintf(credentials, sizeof(credentials), " with username %s", dbuser);
 
+	ast_mutex_lock(&pgsql_lock);
+	is_connected = (pgsqlConn && PQstatus(pgsqlConn) == CONNECTION_OK);
+	ast_mutex_unlock(&pgsql_lock);
 
-	if (pgsqlConn && PQstatus(pgsqlConn) == CONNECTION_OK) {
+	if (is_connected) {
 		snprintf(buf, sizeof(buf), "Connected to %s%s for ", connection_info, credentials);
 		ast_cli_print_timestr_fromseconds(a->fd, ctimesec, buf);
 		return CLI_SUCCESS;

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iea63f0728f76985a01e95b9912c3c5c6065836ed
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>



More information about the asterisk-code-review mailing list