Fix lock usage in cdr_sqlite3_custom to avoid potential crashes during reload.
authorSean Bright <sean@malleable.com>
Mon, 22 Jun 2009 16:09:50 +0000 (16:09 +0000)
committerSean Bright <sean@malleable.com>
Mon, 22 Jun 2009 16:09:50 +0000 (16:09 +0000)
Pointed out by Russell while working on the CEL branch.

git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@202417 65c4cc65-6c06-0410-ace0-fbb531ad65f3

cdr/cdr_sqlite3_custom.c

index 84537cf..fd2d3e4 100644 (file)
@@ -61,8 +61,6 @@ static const char name[] = "cdr_sqlite3_custom";
 static sqlite3 *db = NULL;
 
 static char table[80];
-
-/*! XXX \bug Handling of this variable has crash potential on reload */
 static char *columns;
 
 struct values {
@@ -72,7 +70,7 @@ struct values {
 
 static AST_LIST_HEAD_STATIC(sql_values, values);
 
-static int free_config(void);
+static void free_config(void);
 
 static int load_column_config(const char *tmp)
 {
@@ -168,11 +166,8 @@ static int load_config(int reload)
                free_config();
        }
 
-       ast_mutex_lock(&lock);
-
        if (!(mappingvar = ast_variable_browse(cfg, "master"))) {
                /* Nothing configured */
-               ast_mutex_unlock(&lock);
                ast_config_destroy(cfg);
                return -1;
        }
@@ -187,7 +182,6 @@ static int load_config(int reload)
 
        /* Columns */
        if (load_column_config(ast_variable_retrieve(cfg, "master", "columns"))) {
-               ast_mutex_unlock(&lock);
                ast_config_destroy(cfg);
                free_config();
                return -1;
@@ -195,7 +189,6 @@ static int load_config(int reload)
 
        /* Values */
        if (load_values_config(ast_variable_retrieve(cfg, "master", "values"))) {
-               ast_mutex_unlock(&lock);
                ast_config_destroy(cfg);
                free_config();
                return -1;
@@ -203,18 +196,15 @@ static int load_config(int reload)
 
        ast_verb(3, "cdr_sqlite3_custom: Logging CDR records to table '%s' in 'master.db'\n", table);
 
-       ast_mutex_unlock(&lock);
        ast_config_destroy(cfg);
 
        return 0;
 }
 
-static int free_config(void)
+static void free_config(void)
 {
        struct values *value;
 
-       ast_mutex_lock(&lock);
-
        if (db) {
                sqlite3_close(db);
                db = NULL;
@@ -228,10 +218,6 @@ static int free_config(void)
        while ((value = AST_LIST_REMOVE_HEAD(&sql_values, list))) {
                ast_free(value);
        }
-
-       ast_mutex_unlock(&lock);
-
-       return 0;
 }
 
 static int sqlite3_log(struct ast_cdr *cdr)
@@ -246,6 +232,8 @@ static int sqlite3_log(struct ast_cdr *cdr)
                return 0;
        }
 
+       ast_mutex_lock(&lock);
+
        { /* Make it obvious that only sql should be used outside of this block */
                char *escaped;
                char subst_buf[2048];
@@ -257,6 +245,7 @@ static int sqlite3_log(struct ast_cdr *cdr)
                if (!dummy) {
                        ast_log(LOG_ERROR, "Unable to allocate channel for variable subsitution.\n");
                        ast_free(value_string);
+                       ast_mutex_unlock(&lock);
                        return 0;
                }
                dummy->cdr = ast_cdr_dup(cdr);
@@ -272,8 +261,6 @@ static int sqlite3_log(struct ast_cdr *cdr)
                ast_free(value_string);
        }
 
-       ast_mutex_lock(&lock);
-
        /* XXX This seems awful arbitrary... */
        for (count = 0; count < 5; count++) {
                res = sqlite3_exec(db, sql, NULL, NULL, &error);
@@ -299,10 +286,10 @@ static int sqlite3_log(struct ast_cdr *cdr)
 
 static int unload_module(void)
 {
-       free_config();
-
        ast_cdr_unregister(name);
 
+       free_config();
+
        return 0;
 }
 
@@ -313,14 +300,7 @@ static int load_module(void)
        int res;
        char *sql;
 
-       if (!load_config(0)) {
-               res = ast_cdr_register(name, desc, sqlite3_log);
-               if (res) {
-                       ast_log(LOG_ERROR, "Unable to register custom SQLite3 CDR handling\n");
-                       free_config();
-                       return AST_MODULE_LOAD_DECLINE;
-               }
-       } else {
+       if (load_config(0)) {
                return AST_MODULE_LOAD_DECLINE;
        }
 
@@ -350,12 +330,25 @@ static int load_module(void)
                }
        }
 
-       return 0;
+       res = ast_cdr_register(name, desc, sqlite3_log);
+       if (res) {
+               ast_log(LOG_ERROR, "Unable to register custom SQLite3 CDR handling\n");
+               free_config();
+               return AST_MODULE_LOAD_DECLINE;
+       }
+
+       return AST_MODULE_LOAD_SUCCESS;
 }
 
 static int reload(void)
 {
-       return load_config(1);
+       int res = 0;
+
+       ast_mutex_lock(&lock);
+       res = load_config(1);
+       ast_mutex_unlock(&lock);
+
+       return res;
 }
 
 AST_MODULE_INFO(ASTERISK_GPL_KEY, AST_MODFLAG_DEFAULT, "SQLite3 Custom CDR Module",