config: Fix ast_config_text_file_save writability check for missing files

A patch I did back in 2014 modified ast_config_text_file_save to check the
writability of the main file and include files before truncating and re-writing
them.  An unintended side-effect of this was that if a file doesn't exist,
the check fails and the write is aborted.

This patch causes ast_config_text_file_save to check the writability of the
parent directory of missing files instead of checking the file itself.  This
allows missing files to be created again.  A unit test was also added to
test_config to test saving of config files.

The regression was discovered when app_voicemail's passwordlocation=spooldir
feature stopped working.

ASTERISK-25917 #close
Reported-by: Jonathan Rose
Change-Id: Ic4dbe58c277a47b674679e49daed5fc6de349f80

diff --git a/main/config.c b/main/config.c
index 4766d2efb59735402db47f6a8535928289f38343..cfb5afbb7b572b76283a6d124296b68ef3181431 100644 (file)
--- a/main/config.c
+++ b/main/config.c
@@ -39,6 +39,8 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$")
 #include <time.h>
 #include <sys/stat.h>
 
+#include <string.h>
+#include <libgen.h>
 #include <math.h>      /* HUGE_VAL */
 
 #define AST_INCLUDE_GLOB 1
@@ -2128,6 +2130,25 @@ int config_text_file_save(const char *configfile, const struct ast_config *cfg,
        return ast_config_text_file_save(configfile, cfg, generator);
 }
 
+static int is_writable(const char *fn)
+{
+       if (access(fn, F_OK)) {
+               char *dn = dirname(ast_strdupa(fn));
+
+               if (access(dn, R_OK | W_OK)) {
+                       ast_log(LOG_ERROR, "Unable to write to directory %s (%s)\n", dn, strerror(errno));
+                       return 0;
+               }
+       } else {
+               if (access(fn, R_OK | W_OK)) {
+                       ast_log(LOG_ERROR, "Unable to write %s (%s)\n", fn, strerror(errno));
+                       return 0;
+               }
+       }
+
+       return 1;
+}
+
 int ast_config_text_file_save(const char *configfile, const struct ast_config *cfg, const char *generator)
 {
        FILE *f;
@@ -2150,20 +2171,20 @@ int ast_config_text_file_save(const char *configfile, const struct ast_config *c
        for (incl = cfg->includes; incl; incl = incl->next) {
                /* reset all the output flags in case this isn't our first time saving this data */
                incl->output = 0;
-               /* now make sure we have write access */
+
                if (!incl->exec) {
+                       /* now make sure we have write access to the include file or its parent directory */
                        make_fn(fn, sizeof(fn), incl->included_file, configfile);
-                       if (access(fn, R_OK | W_OK)) {
-                               ast_log(LOG_ERROR, "Unable to write %s (%s)\n", fn, strerror(errno));
+                       /* If the file itself doesn't exist, make sure we have write access to the directory */
+                       if (!is_writable(fn)) {
                                return -1;
                        }
                }
        }
 
-       /* now make sure we have write access to the main config file */
+       /* now make sure we have write access to the main config file or its parent directory */
        make_fn(fn, sizeof(fn), 0, configfile);
-       if (access(fn, R_OK | W_OK)) {
-               ast_log(LOG_ERROR, "Unable to write %s (%s)\n", fn, strerror(errno));
+       if (!is_writable(fn)) {
                return -1;
        }
 

diff --git a/tests/test_config.c b/tests/test_config.c
index e7ee675fc3308a893bfade67f1e2a1b386c88707..18615256abc49140e4ed11abe19a945c61262be0 100644 (file)
--- a/tests/test_config.c
+++ b/tests/test_config.c
@@ -34,6 +34,7 @@
 ASTERISK_FILE_VERSION(__FILE__, "$Revision$");
 
 #include <math.h> /* HUGE_VAL */
+#include <sys/stat.h>
 
 #include "asterisk/config.h"
 #include "asterisk/module.h"
@@ -47,6 +48,7 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$");
 #include "asterisk/logger.h"
 
 #define CONFIG_FILE "test_config.conf"
+#define CONFIG_INCLUDE_FILE "test_config_include.conf"
 
 /*
  * This builds the folowing config:
@@ -293,6 +295,77 @@ static int hook_cb(struct ast_config *cfg)
        return 0;
 }
 
+AST_TEST_DEFINE(config_save)
+{
+       enum ast_test_result_state res = AST_TEST_FAIL;
+       struct ast_flags config_flags = { 0 };
+       struct ast_config *cfg;
+       char config_filename[PATH_MAX];
+       char include_filename[PATH_MAX];
+       struct stat config_stat;
+       off_t before_save;
+
+       switch (cmd) {
+       case TEST_INIT:
+               info->name = "config_save";
+               info->category = "/main/config/";
+               info->summary = "Test config save";
+               info->description =
+                       "Test configuration save.";
+               return AST_TEST_NOT_RUN;
+       case TEST_EXECUTE:
+               break;
+       }
+
+       if (write_config_file()) {
+               ast_test_status_update(test, "Could not write initial config files\n");
+               return res;
+       }
+
+       snprintf(config_filename, PATH_MAX, "%s/%s", ast_config_AST_CONFIG_DIR, CONFIG_FILE);
+       snprintf(include_filename, PATH_MAX, "%s/%s", ast_config_AST_CONFIG_DIR, CONFIG_INCLUDE_FILE);
+
+       cfg = ast_config_load(CONFIG_FILE, config_flags);
+       if (!cfg) {
+               ast_test_status_update(test, "Could not load config\n");
+               goto out;
+       }
+
+       /* We need to re-save to get the generator header */
+       if (ast_config_text_file_save(CONFIG_FILE, cfg, "TEST")) {
+               ast_test_status_update(test, "Unable to write files\n");
+               goto out;
+       }
+
+       stat(config_filename, &config_stat);
+       before_save = config_stat.st_size;
+
+       if (!ast_include_new(cfg, CONFIG_FILE, CONFIG_INCLUDE_FILE, 0, NULL, 4, include_filename, PATH_MAX)) {
+               ast_test_status_update(test, "Could not create include\n");
+               goto out;
+       }
+
+       if (ast_config_text_file_save(CONFIG_FILE, cfg, "TEST")) {
+               ast_test_status_update(test, "Unable to write files\n");
+               goto out;
+       }
+
+       stat(config_filename, &config_stat);
+       if (config_stat.st_size <= before_save) {
+               ast_test_status_update(test, "Did not save config file with #include\n");
+               goto out;
+       }
+
+       res = AST_TEST_PASS;
+
+out:
+       ast_config_destroy(cfg);
+       unlink(config_filename);
+       unlink(include_filename);
+
+       return res;
+}
+
 AST_TEST_DEFINE(config_hook)
 {
        enum ast_test_result_state res = AST_TEST_FAIL;
@@ -934,6 +1007,7 @@ AST_TEST_DEFINE(config_options_test)
 
 static int unload_module(void)
 {
+       AST_TEST_UNREGISTER(config_save);
        AST_TEST_UNREGISTER(copy_config);
        AST_TEST_UNREGISTER(config_hook);
        AST_TEST_UNREGISTER(ast_parse_arg_test);
@@ -943,6 +1017,7 @@ static int unload_module(void)
 
 static int load_module(void)
 {
+       AST_TEST_REGISTER(config_save);
        AST_TEST_REGISTER(copy_config);
        AST_TEST_REGISTER(config_hook);
        AST_TEST_REGISTER(ast_parse_arg_test);