From: Jonathan Rose Date: Tue, 7 Feb 2012 15:04:38 +0000 (+0000) Subject: Fix column duplication bug in module reload for cdr_pgsql. X-Git-Tag: 1.8.11.0-rc1~60 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ff07b89e6395be677963eeed5e005b73aa48246b;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/ git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.8@354263 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- diff --git a/cdr/cdr_pgsql.c b/cdr/cdr_pgsql.c index 793b760811..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; } @@ -389,12 +378,18 @@ static int config_module(int reload) if ((cfg = ast_config_load(config, config_flags)) == NULL || cfg == CONFIG_STATUS_FILEINVALID) { ast_log(LOG_WARNING, "Unable to load config for PostgreSQL CDR's: %s\n", config); return -1; - } else if (cfg == CONFIG_STATUS_FILEUNCHANGED) + } else if (cfg == CONFIG_STATUS_FILEUNCHANGED) { 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"))) { @@ -402,10 +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; } @@ -414,10 +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; } @@ -426,10 +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; } @@ -438,10 +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; } @@ -450,10 +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; } @@ -462,10 +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; } @@ -474,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; } @@ -486,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; } @@ -577,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; } @@ -585,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); @@ -616,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); @@ -629,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)