Fix app_voicemail Segfault And A Few Memory Leaks

The original report was that app_voicemail would crash.  This was caused by
ast_config_load() returning CONFIG_STATUS_FILEINVALID but no checks being
performed for that return status.  After adding the initial patch to fix this
issue, Jaco Kroon (jkroon) added some fixes to memory leaks he had discovered.

During review, Walter Doekes (wdoekes) suggested adding a helper function in
order to determine if we had a valid configuration or not.

This patch does the following:

* Creates a helper function to check if the configuration is valid

* Adds calls to the new helper function where appropiate

* Fixes memory leaks where the code returned without running
  ast_config_destroy() on the configuration that was loaded

(closes issue ASTERISK-21302)
Reported by: Jaco Kroon
Tested by: Jaco Kroon, Michael L. Young
Patches:
    asterisk-11.3.0-app_voicemail-ast_config-fixes.patch
                                                       Jaco Kroon (license 5671)
    asterisk-21302-valid_cfg_and_mem_leaks_v3-1.8.diff
                                                 Michael L. Young (license 5026)

Review: https://reviewboard.asterisk.org/r/2443/
........

Merged revisions 385551 from http://svn.asterisk.org/svn/asterisk/branches/1.8
........

Merged revisions 385557 from http://svn.asterisk.org/svn/asterisk/branches/11

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

diff --git a/apps/app_voicemail.c b/apps/app_voicemail.c
index 0f4c688..1d53a67 100644 (file)
--- a/apps/app_voicemail.c
+++ b/apps/app_voicemail.c
@@ -1719,6 +1719,14 @@ static int reset_user_pw(const char *context, const char *mailbox, const char *n
        return res;
 }
 
+/*!
+ * \brief Check if configuration file is valid
+ */
+static inline int valid_config(const struct ast_config *cfg)
+{
+       return cfg && cfg != CONFIG_STATUS_FILEINVALID;
+}
+
 /*! 
  * \brief The handler for the change password option.
  * \param vmu The voicemail user to work with.
@@ -1755,7 +1763,7 @@ static void vm_change_password(struct ast_vm_user *vmu, const char *newpassword)
                }
                /* Fall-through */
        case OPT_PWLOC_VOICEMAILCONF:
-               if ((cfg = ast_config_load(VOICEMAIL_CONFIG, config_flags)) && cfg != CONFIG_STATUS_FILEINVALID) {
+               if ((cfg = ast_config_load(VOICEMAIL_CONFIG, config_flags)) && valid_config(cfg)) {
                        while ((category = ast_category_browse(cfg, category))) {
                                if (!strcasecmp(category, vmu->context)) {
                                        if (!(tmp = ast_variable_retrieve(cfg, category, vmu->mailbox))) {
@@ -1784,14 +1792,17 @@ static void vm_change_password(struct ast_vm_user *vmu, const char *newpassword)
                                reset_user_pw(vmu->context, vmu->mailbox, newpassword);
                                ast_copy_string(vmu->password, newpassword, sizeof(vmu->password));
                                ast_config_text_file_save(VOICEMAIL_CONFIG, cfg, "AppVoicemail");
+                               ast_config_destroy(cfg);
                                break;
                        }
+
+                       ast_config_destroy(cfg);
                }
                /* Fall-through */
        case OPT_PWLOC_USERSCONF:
                /* check users.conf and update the password stored for the mailbox */
                /* if no vmsecret entry exists create one. */
-               if ((cfg = ast_config_load("users.conf", config_flags)) && cfg != CONFIG_STATUS_FILEINVALID) {
+               if ((cfg = ast_config_load("users.conf", config_flags)) && valid_config(cfg)) {
                        ast_debug(4, "we are looking for %s\n", vmu->mailbox);
                        for (category = ast_category_browse(cfg, NULL); category; category = ast_category_browse(cfg, category)) {
                                ast_debug(4, "users.conf: %s\n", category);
@@ -1825,6 +1836,8 @@ static void vm_change_password(struct ast_vm_user *vmu, const char *newpassword)
                                ast_copy_string(vmu->password, newpassword, sizeof(vmu->password));
                                ast_config_text_file_save("users.conf", cfg, "AppVoicemail");
                        }
+
+                       ast_config_destroy(cfg);
                }
        }
 }
@@ -4179,7 +4192,7 @@ static int store_file(const char *dir, const char *mailboxuser, const char *mail
                        res = -1;
                        break;
                }
-               if (cfg && cfg != CONFIG_STATUS_FILEINVALID) {
+               if (valid_config(cfg)) {
                        if (!(idata.context = ast_variable_retrieve(cfg, "message", "context"))) {
                                idata.context = "";
                        }
@@ -4235,7 +4248,7 @@ static int store_file(const char *dir, const char *mailboxuser, const char *mail
        if (obj) {
                ast_odbc_release_obj(obj);
        }
-       if (cfg)
+       if (valid_config(cfg))
                ast_config_destroy(cfg);
        if (fdm != MAP_FAILED)
                munmap(fdm, fdlen);
@@ -4714,7 +4727,7 @@ static void prep_email_sub_vars(struct ast_channel *ast, struct ast_vm_user *vmu
        if (strlen(fromfile) < sizeof(fromfile) - 5) {
                strcat(fromfile, ".txt");
        }
-       if (!(msg_cfg = ast_config_load(fromfile, config_flags))) {
+       if (!(msg_cfg = ast_config_load(fromfile, config_flags)) || !(valid_config(msg_cfg))) {
                ast_debug(1, "Config load for message text file '%s' failed\n", fromfile);
                return;
        }
@@ -5106,7 +5119,7 @@ static void make_email_file(FILE *p,
                        if (strlen(fromfile) < sizeof(fromfile) - 5) {
                                strcat(fromfile, ".txt");
                        }
-                       if ((msg_cfg = ast_config_load(fromfile, config_flags))) {
+                       if ((msg_cfg = ast_config_load(fromfile, config_flags)) && valid_config(msg_cfg)) {
                                if ((v = ast_variable_retrieve(msg_cfg, "message", "callerid"))) {
                                        ast_copy_string(origcallerid, v, sizeof(origcallerid));
                                }
@@ -7592,7 +7605,7 @@ static int vm_forwardoptions(struct ast_channel *chan, struct ast_vm_user *vmu,
        strncat(backup, "-bak", sizeof(backup) - strlen(backup) - 1);
        strncat(backup_textfile, "-bak.txt", sizeof(backup_textfile) - strlen(backup_textfile) - 1);
 
-       if ((msg_cfg = ast_config_load(textfile, config_flags)) && msg_cfg != CONFIG_STATUS_FILEINVALID && (duration_str = ast_variable_retrieve(msg_cfg, "message", "duration"))) {
+       if ((msg_cfg = ast_config_load(textfile, config_flags)) && valid_config(msg_cfg) && (duration_str = ast_variable_retrieve(msg_cfg, "message", "duration"))) {
                *duration = atoi(duration_str);
        } else {
                *duration = 0;
@@ -7628,7 +7641,7 @@ static int vm_forwardoptions(struct ast_channel *chan, struct ast_vm_user *vmu,
                        *duration = 0;
 
                        /* if we can't read the message metadata, stop now */
-                       if (!msg_cfg) {
+                       if (!valid_config(msg_cfg)) {
                                cmd = 0;
                                break;
                        }
@@ -7712,7 +7725,7 @@ static int vm_forwardoptions(struct ast_channel *chan, struct ast_vm_user *vmu,
                }
        }
 
-       if (msg_cfg)
+       if (valid_config(msg_cfg))
                ast_config_destroy(msg_cfg);
        if (prepend_duration)
                *duration = prepend_duration;
@@ -8463,7 +8476,7 @@ static int play_message(struct ast_channel *chan, struct ast_vm_user *vmu, struc
        snprintf(filename, sizeof(filename), "%s.txt", vms->fn);
        RETRIEVE(vms->curdir, vms->curmsg, vmu->mailbox, vmu->context);
        msg_cfg = ast_config_load(filename, config_flags);
-       if (!msg_cfg || msg_cfg == CONFIG_STATUS_FILEINVALID) {
+       if (!valid_config(msg_cfg)) {
                ast_log(LOG_WARNING, "No message attribute file?!! (%s)\n", filename);
                return 0;
        }
@@ -8541,7 +8554,7 @@ static int play_message(struct ast_channel *chan, struct ast_vm_user *vmu, struc
                }
        }
 
-       if (!msg_cfg) {
+       if (!valid_config(msg_cfg)) {
                ast_log(AST_LOG_WARNING, "No message attribute file?!! (%s)\n", filename);
                return 0;
        }
@@ -13637,7 +13650,7 @@ static void read_password_from_file(const char *secretfn, char *password, int pa
        struct ast_flags config_flags = { 0 };
 
        pwconf = ast_config_load(secretfn, config_flags);
-       if (pwconf) {
+       if (valid_config(pwconf)) {
                const char *val = ast_variable_retrieve(pwconf, "general", "password");
                if (val) {
                        ast_copy_string(password, val, passwordlen);
@@ -14124,7 +14137,7 @@ AST_TEST_DEFINE(test_voicemail_load_config)
        fputs("00000002 => 9999,Mrs. Test\n", file);
        fclose(file);
 
-       if (!(cfg = ast_config_load(config_filename, config_flags))) {
+       if (!(cfg = ast_config_load(config_filename, config_flags)) || !valid_config(cfg)) {
                res = AST_TEST_FAIL;
                goto cleanup;
        }
@@ -14454,7 +14467,7 @@ static int advanced_options(struct ast_channel *chan, struct ast_vm_user *vmu, s
        RETRIEVE(vms->curdir, vms->curmsg, vmu->mailbox, vmu->context);
        msg_cfg = ast_config_load(filename, config_flags);
        DISPOSE(vms->curdir, vms->curmsg);
-       if (!msg_cfg || msg_cfg == CONFIG_STATUS_FILEINVALID) {
+       if (!valid_config(msg_cfg)) {
                ast_log(AST_LOG_WARNING, "No message attribute file?!! (%s)\n", filename);
                return 0;
        }
@@ -14616,9 +14629,9 @@ static int advanced_options(struct ast_channel *chan, struct ast_vm_user *vmu, s
                break;
        }
 
-#ifndef IMAP_STORAGE
        ast_config_destroy(msg_cfg);
 
+#ifndef IMAP_STORAGE
        if (!res) {
                make_file(vms->fn, sizeof(vms->fn), vms->curdir, msg);
                vms->heard[msg] = 1;