Commit e51e29e84c16f7a5d9079c2dc645301a54fdb172

Patrick Steinhardt 2017-11-12T13:59:47

config_parse: have `git_config_parse` own entry value and name The function `git_config_parse` uses several callbacks to pass data along to the caller as it parses the file. One design shortcoming here is that strings passed to those callbacks are expected to be freed by them, which is really confusing. Fix the issue by changing memory ownership here. Instead of expecting the `on_variable` callbacks to free memory for `git_config_parse`, just do it inside of `git_config_parse`. While this obviously requires a bit more memory allocation churn due to having to copy both name and value at some places, this shouldn't be too much of a burden.

diff --git a/src/config_file.c b/src/config_file.c
index 90255c9..84f6dd1 100644
--- a/src/config_file.c
+++ b/src/config_file.c
@@ -1022,8 +1022,8 @@ static int parse_conditional_include(git_config_parser *reader,
 static int read_on_variable(
 	git_config_parser *reader,
 	const char *current_section,
-	char *var_name,
-	char *var_value,
+	const char *var_name,
+	const char *var_value,
 	const char *line,
 	size_t line_len,
 	void *data)
@@ -1031,24 +1031,26 @@ static int read_on_variable(
 	diskfile_parse_state *parse_data = (diskfile_parse_state *)data;
 	git_buf buf = GIT_BUF_INIT;
 	git_config_entry *entry;
+	const char *c;
 	int result = 0;
 
 	GIT_UNUSED(line);
 	GIT_UNUSED(line_len);
 
-	git__strtolower(var_name);
-	git_buf_printf(&buf, "%s.%s", current_section, var_name);
-	git__free(var_name);
+	git_buf_puts(&buf, current_section);
+	git_buf_putc(&buf, '.');
+	for (c = var_name; *c; c++)
+		git_buf_putc(&buf, git__tolower(*c));
 
 	if (git_buf_oom(&buf)) {
-		git__free(var_value);
+		git_buf_free(&buf);
 		return -1;
 	}
 
 	entry = git__calloc(1, sizeof(git_config_entry));
 	GITERR_CHECK_ALLOC(entry);
 	entry->name = git_buf_detach(&buf);
-	entry->value = var_value;
+	entry->value = var_value ? git__strdup(var_value) : NULL;
 	entry->level = parse_data->level;
 	entry->include_depth = parse_data->depth;
 
@@ -1065,7 +1067,6 @@ static int read_on_variable(
 		result = parse_conditional_include(reader, parse_data,
 						   entry->name, entry->value);
 
-
 	return result;
 }
 
@@ -1249,8 +1250,8 @@ static int write_on_section(
 static int write_on_variable(
 	git_config_parser *reader,
 	const char *current_section,
-	char *var_name,
-	char *var_value,
+	const char *var_name,
+	const char *var_value,
 	const char *line,
 	size_t line_len,
 	void *data)
@@ -1279,9 +1280,6 @@ static int write_on_variable(
 	if (has_matched && write_data->preg != NULL)
 		has_matched = (regexec(write_data->preg, var_value, 0, NULL, 0) == 0);
 
-	git__free(var_name);
-	git__free(var_value);
-
 	/* If this isn't the name/value we're looking for, simply dump the
 	 * existing data back out and continue on.
 	 */
diff --git a/src/config_parse.c b/src/config_parse.c
index 66ac432..ffbc466 100644
--- a/src/config_parse.c
+++ b/src/config_parse.c
@@ -463,7 +463,7 @@ int git_config_parse(
 	void *data)
 {
 	git_parse_ctx *ctx;
-	char *current_section = NULL, *var_name, *var_value;
+	char *current_section = NULL, *var_name = NULL, *var_value = NULL;
 	int result = 0;
 
 	ctx = &parser->ctx;
@@ -508,7 +508,10 @@ int git_config_parse(
 		default: /* assume variable declaration */
 			if ((result = parse_variable(parser, &var_name, &var_value)) == 0 && on_variable) {
 				result = on_variable(parser, current_section, var_name, var_value, line_start, line_len, data);
+				git__free(var_name);
+				git__free(var_value);
 			}
+
 			break;
 		}
 
diff --git a/src/config_parse.h b/src/config_parse.h
index d14a8e6..6650b87 100644
--- a/src/config_parse.h
+++ b/src/config_parse.h
@@ -36,8 +36,8 @@ typedef int (*git_config_parser_section_cb)(
 typedef int (*git_config_parser_variable_cb)(
 	git_config_parser *parser,
 	const char *current_section,
-	char *var_name,
-	char *var_value,
+	const char *var_name,
+	const char *var_value,
 	const char *line,
 	size_t line_len,
 	void *data);