[Asterisk-code-review] config: Fix ast config text file save2 writability check fo... (asterisk[13])

George Joseph asteriskteam at digium.com
Sun Apr 24 23:41:08 CDT 2016


George Joseph has uploaded a new change for review.

  https://gerrit.asterisk.org/2691

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 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/91/2691/1

diff --git a/include/asterisk/utils.h b/include/asterisk/utils.h
index c7a4737..06d9a27 100644
--- a/include/asterisk/utils.h
+++ b/include/asterisk/utils.h
@@ -775,6 +775,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 2a6baa1..7b424de 100644
--- a/main/config.c
+++ b/main/config.c
@@ -2512,6 +2512,27 @@
 	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 = 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_save2(const char *configfile, const struct ast_config *cfg, const char *generator, uint32_t flags)
 {
 	FILE *f;
@@ -2534,20 +2555,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 95eda11..add1d4d 100644
--- a/main/utils.c
+++ b/main/utils.c
@@ -2100,6 +2100,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 40de265..739ed39 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"
@@ -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,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_save2(CONFIG_FILE, cfg, "TEST", CONFIG_SAVE_FLAG_PRESERVE_EFFECTIVE_CONTEXT)) {
+		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)
@@ -1734,6 +1818,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 +1833,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/2691
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic4dbe58c277a47b674679e49daed5fc6de349f80
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <gjoseph at digium.com>



More information about the asterisk-code-review mailing list