[Asterisk-code-review] config: Fix ast config text file save2 writability check fo... (asterisk[master])
Joshua Colp
asteriskteam at digium.com
Wed Apr 27 14:55:56 CDT 2016
Joshua Colp has submitted this change and it was merged.
Change subject: config: Fix ast_config_text_file_save2 writability check for missing files
......................................................................
config: Fix ast_config_text_file_save2 writability check for missing files
A patch I did back in 2014 modified ast_config_text_file_save2 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_save2 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
---
M main/config.c
M tests/test_config.c
2 files changed, 103 insertions(+), 6 deletions(-)
Approvals:
Mark Michelson: Looks good to me, approved
Jonathan R. Rose: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, but someone else must approve; Verified
diff --git a/main/config.c b/main/config.c
index dad3b66..5992703 100644
--- a/main/config.c
+++ b/main/config.c
@@ -36,6 +36,9 @@
#include "asterisk/paths.h" /* use ast_config_AST_CONFIG_DIR */
#include "asterisk/network.h" /* we do some sockaddr manipulation here */
+
+#include <string.h>
+#include <libgen.h>
#include <time.h>
#include <sys/stat.h>
@@ -2512,6 +2515,25 @@
return ast_config_text_file_save2(configfile, cfg, generator, CONFIG_SAVE_FLAG_PRESERVE_EFFECTIVE_CONTEXT);
}
+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_save2(const char *configfile, const struct ast_config *cfg, const char *generator, uint32_t flags)
{
FILE *f;
@@ -2534,20 +2556,20 @@
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 fe64a07..eb5409b 100644
--- a/tests/test_config.c
+++ b/tests/test_config.c
@@ -34,6 +34,7 @@
ASTERISK_REGISTER_FILE();
#include <math.h> /* HUGE_VAL */
+#include <sys/stat.h>
#include "asterisk/config.h"
#include "asterisk/module.h"
@@ -49,6 +50,7 @@
#include "asterisk/format_cap.h"
#define CONFIG_FILE "test_config.conf"
+#define CONFIG_INCLUDE_FILE "test_config_include.conf"
/*
* This builds the folowing config:
@@ -879,6 +881,77 @@
}
ast_config_destroy(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)
@@ -1734,6 +1807,7 @@
static int unload_module(void)
{
+ AST_TEST_UNREGISTER(config_save);
AST_TEST_UNREGISTER(config_basic_ops);
AST_TEST_UNREGISTER(config_filtered_ops);
AST_TEST_UNREGISTER(config_template_ops);
@@ -1748,6 +1822,7 @@
static int load_module(void)
{
+ AST_TEST_REGISTER(config_save);
AST_TEST_REGISTER(config_basic_ops);
AST_TEST_REGISTER(config_filtered_ops);
AST_TEST_REGISTER(config_template_ops);
--
To view, visit https://gerrit.asterisk.org/2692
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic4dbe58c277a47b674679e49daed5fc6de349f80
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Jonathan R. Rose <jonathan.rose at motorolasolutions.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
More information about the asterisk-code-review
mailing list