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 e857baa87a7f9e93471f76a6ca708d98a99ee488..cd2a091fa119fddfa48441718e107a17214a3c91 100644 (file)
--- 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 <mhardemn@papersoft.com>
  * 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)