[Asterisk-code-review] config: Fix ast config text file save writability check for... (asterisk[11])
George Joseph
asteriskteam at digium.com
Sun Apr 24 23:54:12 CDT 2016
George Joseph has uploaded a new change for review.
https://gerrit.asterisk.org/2693
Change subject: config: Fix ast_config_text_file_save writability check for missing files
......................................................................
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
---
M include/asterisk/utils.h
M main/config.c
M main/utils.c
M tests/test_config.c
4 files changed, 149 insertions(+), 6 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/93/2693/1
diff --git a/include/asterisk/utils.h b/include/asterisk/utils.h
index bf17876..efc95fa 100644
--- a/include/asterisk/utils.h
+++ b/include/asterisk/utils.h
@@ -726,6 +726,23 @@
void ast_enable_packet_fragmentation(int sock);
/*!
+ * \brief Extract directory name from path
+ * \since 13.9.0
+ * \param path
+ * \return directory component of path (see below) or NULL if memory allocation failed.
+ *
+ * This is a portable version of 'basename(3)'.
+ * If path is NULL, empty, or contains no '/', "." is returned.
+ * If path is exactly "/", "/" is returned.
+ * Otherwise the path up to, but not including, the final '/' is returned.
+ *
+ * The original path is not modified.
+ *
+ * In all cases, return value must be released with ast_free.
+ */
+char *ast_dirname(const char *path);
+
+/*!
* \brief Recursively create directory path
* \param path The directory path to create
* \param mode The permissions with which to try to create the directory
diff --git a/main/config.c b/main/config.c
index 4766d2e..f55f00d 100644
--- a/main/config.c
+++ b/main/config.c
@@ -2128,6 +2128,27 @@
return ast_config_text_file_save(configfile, cfg, generator);
}
+static int is_writable(const char *fn)
+{
+ if (access(fn, F_OK)) {
+ char *dn = ast_dirname(fn);
+
+ if (!dn || access(dn, R_OK | W_OK)) {
+ ast_log(LOG_ERROR, "Unable to write to directory %s (%s)\n", dn, strerror(errno));
+ ast_free(dn);
+ return 0;
+ }
+ ast_free(dn);
+ } 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 @@
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/main/utils.c b/main/utils.c
index 8c239d5..3ec26d7 100644
--- a/main/utils.c
+++ b/main/utils.c
@@ -2234,6 +2234,25 @@
#endif /* HAVE_IP_MTU_DISCOVER */
}
+char *ast_dirname(const char *path)
+{
+ char *last_sep;
+ char *dirname;
+
+ if (ast_strlen_zero(path) || !(last_sep = strrchr(path, '/'))) {
+ return ast_strdup(".");
+ }
+
+ if (strlen(path) == 1) {
+ return ast_strdup(path);
+ }
+
+ dirname = ast_strdup(path);
+ dirname[last_sep - path] = '\0';
+
+ return dirname;
+}
+
int ast_mkdir(const char *path, int mode)
{
char *ptr;
diff --git a/tests/test_config.c b/tests/test_config.c
index e7ee675..0489d72 100644
--- 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 @@
#include "asterisk/logger.h"
#define CONFIG_FILE "test_config.conf"
+#define CONFIG_INCLUDE_FILE "test_config_include.conf"
/*
* This builds the folowing config:
@@ -291,6 +293,88 @@
}
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 = { CONFIG_FLAG_FILEUNCHANGED };
+ 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);
+ stat(config_filename, &config_stat);
+ before_save = config_stat.st_size;
+
+ cfg = ast_config_load(CONFIG_FILE, config_flags);
+ if (!cfg) {
+ ast_test_status_update(test, "Could not load config\n");
+ goto out;
+ }
+
+ delete_config_file();
+
+ 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;
+ }
+ before_save = config_stat.st_size;
+
+ snprintf(include_filename, PATH_MAX, "%s/%s", ast_config_AST_CONFIG_DIR, CONFIG_INCLUDE_FILE);
+
+ chmod(include_filename, S_IRUSR|S_IRGRP|S_IROTH);
+
+ if (!ast_config_text_file_save(CONFIG_FILE, cfg, "TEST")) {
+ ast_test_status_update(test, "Should not have been able 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 correctly when include isn't writeble\n");
+ goto out;
+ }
+
+ res = AST_TEST_PASS;
+
+out:
+ ast_config_destroy(cfg);
+ chmod(include_filename, S_IRUSR | S_IWUSR);
+ delete_config_file();
+ unlink(include_filename);
+
+ return res;
}
AST_TEST_DEFINE(config_hook)
@@ -934,6 +1018,7 @@
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 +1028,7 @@
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);
--
To view, visit https://gerrit.asterisk.org/2693
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic4dbe58c277a47b674679e49daed5fc6de349f80
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: George Joseph <gjoseph at digium.com>
More information about the asterisk-code-review
mailing list