From: Jonathan Rose Date: Tue, 7 Feb 2012 15:19:51 +0000 (+0000) Subject: Fix column duplication bug in module reload for cdr_pgsql. X-Git-Tag: 10.3.0-rc1~55 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=72125d4a56905c232ff0a161dfbadacbabe52957;p=thirdparty%2Fasterisk.git Fix column duplication bug in module reload for cdr_pgsql. Prior to this patch, attempts to reload cdr_pgsql.so would cause the column list to keep its current data and then add a second copy during the reload. This would cause attempts to log the CDR to the database to fail. This patch also cleans up some unnecessary null checks for ast_free and deals with a few potential locking problems. (closes issue ASTERISK-19216) Reported by: Jacek Konieczny Review: https://reviewboard.asterisk.org/r/1711/ ........ Merged revisions 354263 from http://svn.asterisk.org/svn/asterisk/branches/1.8 git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/10@354270 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- diff --git a/cdr/cdr_pgsql.c b/cdr/cdr_pgsql.c index e857baa87a..cd2a091fa1 100644 --- a/cdr/cdr_pgsql.c +++ b/cdr/cdr_pgsql.c @@ -1,7 +1,7 @@ /* * Asterisk -- An open source telephony toolkit. * - * Copyright (C) 2003 - 2006 + * Copyright (C) 2003 - 2012 * * Matthew D. Hardeman * Adapted from the MySQL CDR logger originally by James Sharp @@ -81,6 +81,7 @@ static AST_RWLIST_HEAD_STATIC(psql_columns, columns); ast_free(sql); \ ast_free(sql2); \ AST_RWLIST_UNLOCK(&psql_columns); \ + ast_mutex_unlock(&pgsql_lock); \ return -1; \ } \ } \ @@ -94,6 +95,7 @@ static AST_RWLIST_HEAD_STATIC(psql_columns, columns); ast_free(sql); \ ast_free(sql2); \ AST_RWLIST_UNLOCK(&psql_columns); \ + ast_mutex_unlock(&pgsql_lock); \ return -1; \ } \ } \ @@ -132,14 +134,10 @@ static int pgsql_log(struct ast_cdr *cdr) struct ast_str *sql = ast_str_create(maxsize), *sql2 = ast_str_create(maxsize2); char buf[257], escapebuf[513], *value; int first = 1; - + if (!sql || !sql2) { - if (sql) { - ast_free(sql); - } - if (sql2) { - ast_free(sql2); - } + ast_free(sql); + ast_free(sql2); return -1; } @@ -270,9 +268,10 @@ static int pgsql_log(struct ast_cdr *cdr) } } first = 0; - } - AST_RWLIST_UNLOCK(&psql_columns); + } + LENGTHEN_BUF1(ast_str_strlen(sql2) + 2); + AST_RWLIST_UNLOCK(&psql_columns); ast_str_append(&sql, 0, ")%s)", ast_str_buffer(sql2)); ast_verb(11, "[%s]\n", ast_str_buffer(sql)); @@ -334,44 +333,34 @@ static int pgsql_log(struct ast_cdr *cdr) return 0; } -static int unload_module(void) +/* This function should be called without holding the pgsql_columns lock */ +static void empty_columns(void) { struct columns *current; + AST_RWLIST_WRLOCK(&psql_columns); + while ((current = AST_RWLIST_REMOVE_HEAD(&psql_columns, list))) { + ast_free(current); + } + AST_RWLIST_UNLOCK(&psql_columns); +} + +static int unload_module(void) +{ ast_cdr_unregister(name); PQfinish(conn); - if (pghostname) { - ast_free(pghostname); - } - if (pgdbname) { - ast_free(pgdbname); - } - if (pgdbuser) { - ast_free(pgdbuser); - } - if (pgpassword) { - ast_free(pgpassword); - } - if (pgdbport) { - ast_free(pgdbport); - } - if (table) { - ast_free(table); - } - if (encoding) { - ast_free(encoding); - } - if (tz) { - ast_free(tz); - } + ast_free(pghostname); + ast_free(pgdbname); + ast_free(pgdbuser); + ast_free(pgpassword); + ast_free(pgdbport); + ast_free(table); + ast_free(encoding); + ast_free(tz); - AST_RWLIST_WRLOCK(&psql_columns); - while ((current = AST_RWLIST_REMOVE_HEAD(&psql_columns, list))) { - ast_free(current); - } - AST_RWLIST_UNLOCK(&psql_columns); + empty_columns(); return 0; } @@ -393,9 +382,14 @@ static int config_module(int reload) return 0; } + ast_mutex_lock(&pgsql_lock); + if (!(var = ast_variable_browse(cfg, "global"))) { ast_config_destroy(cfg); - return 0; + ast_mutex_unlock(&pgsql_lock); + ast_log(LOG_NOTICE, "cdr_pgsql configuration contains no global section, skipping module %s.\n", + reload ? "reload" : "load"); + return -1; } if (!(tmp = ast_variable_retrieve(cfg, "global", "hostname"))) { @@ -403,11 +397,10 @@ static int config_module(int reload) tmp = ""; /* connect via UNIX-socket by default */ } - if (pghostname) { - ast_free(pghostname); - } + ast_free(pghostname); if (!(pghostname = ast_strdup(tmp))) { ast_config_destroy(cfg); + ast_mutex_unlock(&pgsql_lock); return -1; } @@ -416,11 +409,10 @@ static int config_module(int reload) tmp = "asteriskcdrdb"; } - if (pgdbname) { - ast_free(pgdbname); - } + ast_free(pgdbname); if (!(pgdbname = ast_strdup(tmp))) { ast_config_destroy(cfg); + ast_mutex_unlock(&pgsql_lock); return -1; } @@ -429,11 +421,10 @@ static int config_module(int reload) tmp = "asterisk"; } - if (pgdbuser) { - ast_free(pgdbuser); - } + ast_free(pgdbuser); if (!(pgdbuser = ast_strdup(tmp))) { ast_config_destroy(cfg); + ast_mutex_unlock(&pgsql_lock); return -1; } @@ -442,11 +433,10 @@ static int config_module(int reload) tmp = ""; } - if (pgpassword) { - ast_free(pgpassword); - } + ast_free(pgpassword); if (!(pgpassword = ast_strdup(tmp))) { ast_config_destroy(cfg); + ast_mutex_unlock(&pgsql_lock); return -1; } @@ -455,11 +445,10 @@ static int config_module(int reload) tmp = "5432"; } - if (pgdbport) { - ast_free(pgdbport); - } + ast_free(pgdbport); if (!(pgdbport = ast_strdup(tmp))) { ast_config_destroy(cfg); + ast_mutex_unlock(&pgsql_lock); return -1; } @@ -468,11 +457,10 @@ static int config_module(int reload) tmp = "cdr"; } - if (table) { - ast_free(table); - } + ast_free(table); if (!(table = ast_strdup(tmp))) { ast_config_destroy(cfg); + ast_mutex_unlock(&pgsql_lock); return -1; } @@ -481,11 +469,10 @@ static int config_module(int reload) tmp = "LATIN9"; } - if (encoding) { - ast_free(encoding); - } + ast_free(encoding); if (!(encoding = ast_strdup(tmp))) { ast_config_destroy(cfg); + ast_mutex_unlock(&pgsql_lock); return -1; } @@ -493,12 +480,12 @@ static int config_module(int reload) tmp = ""; } - if (tz) { - ast_free(tz); - tz = NULL; - } + ast_free(tz); + tz = NULL; + if (!ast_strlen_zero(tmp) && !(tz = ast_strdup(tmp))) { ast_config_destroy(cfg); + ast_mutex_unlock(&pgsql_lock); return -1; } @@ -584,6 +571,7 @@ static int config_module(int reload) ast_log(LOG_ERROR, "Failed to query database columns: %s\n", pgerror); PQclear(result); unload_module(); + ast_mutex_unlock(&pgsql_lock); return AST_MODULE_LOAD_DECLINE; } @@ -592,9 +580,13 @@ static int config_module(int reload) ast_log(LOG_ERROR, "cdr_pgsql: Failed to query database columns. No columns found, does the table exist?\n"); PQclear(result); unload_module(); + ast_mutex_unlock(&pgsql_lock); return AST_MODULE_LOAD_DECLINE; } + /* Clear out the columns list. */ + empty_columns(); + for (i = 0; i < rows; i++) { fname = PQgetvalue(result, i, 0); ftype = PQgetvalue(result, i, 1); @@ -623,7 +615,9 @@ static int config_module(int reload) } else { cur->hasdefault = 0; } + AST_RWLIST_WRLOCK(&psql_columns); AST_RWLIST_INSERT_TAIL(&psql_columns, cur, list); + AST_RWLIST_UNLOCK(&psql_columns); } } PQclear(result); @@ -636,12 +630,17 @@ static int config_module(int reload) ast_config_destroy(cfg); - return ast_cdr_register(name, ast_module_info->description, pgsql_log); + ast_mutex_unlock(&pgsql_lock); + return 0; } static int load_module(void) { - return config_module(0) ? AST_MODULE_LOAD_DECLINE : 0; + if (config_module(0)) { + return AST_MODULE_LOAD_DECLINE; + } + return ast_cdr_register(name, ast_module_info->description, pgsql_log) + ? AST_MODULE_LOAD_DECLINE : 0; } static int reload(void)