[asterisk-commits] res config pgsql: Fix thread safety problems (asterisk[master])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Fri Feb 24 10:20:26 CST 2017
Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/5078 )
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 b5f24b0..93126c6 100644
--- a/res/res_config_pgsql.c
+++ b/res/res_config_pgsql.c
@@ -268,6 +268,7 @@
}
if (database == NULL) {
+ AST_LIST_UNLOCK(&psql_tables);
return NULL;
}
@@ -409,6 +410,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,
@@ -417,6 +428,7 @@
PQfinish(pgsqlConn);
pgsqlConn = NULL;
}
+ ast_mutex_unlock(&pgsql_lock);
return NULL;
}
@@ -434,6 +446,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;
}
@@ -452,6 +465,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;
}
@@ -459,8 +473,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;
@@ -540,6 +552,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,
@@ -548,6 +570,7 @@
PQfinish(pgsqlConn);
pgsqlConn = NULL;
}
+ ast_mutex_unlock(&pgsql_lock);
ast_config_destroy(cfg);
return NULL;
}
@@ -573,6 +596,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;
}
@@ -593,6 +617,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;
}
@@ -604,10 +629,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);
@@ -704,6 +726,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,
@@ -712,6 +744,7 @@
PQfinish(pgsqlConn);
pgsqlConn = NULL;
}
+ ast_mutex_unlock(&pgsql_lock);
release_table(table);
return -1;
}
@@ -725,6 +758,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;
}
@@ -735,6 +769,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;
}
@@ -749,6 +784,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;
}
@@ -760,6 +796,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;
}
@@ -768,8 +805,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;
@@ -836,12 +871,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;
}
@@ -849,6 +895,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;
}
@@ -863,6 +910,7 @@
PQfinish(pgsqlConn);
pgsqlConn = NULL;
}
+ ast_mutex_unlock(&pgsql_lock);
release_table(table);
return -1;
}
@@ -879,6 +927,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;
}
@@ -892,7 +941,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;
@@ -937,6 +986,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,
@@ -945,12 +1004,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;
}
@@ -971,6 +1024,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;
@@ -1014,27 +1068,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 */
@@ -1050,10 +1104,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);
@@ -1266,7 +1321,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;
}
@@ -1393,6 +1448,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;
@@ -1500,13 +1556,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) {
@@ -1606,7 +1667,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:
@@ -1632,8 +1693,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/5078
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iea63f0728f76985a01e95b9912c3c5c6065836ed
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
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-commits
mailing list