Re: [PATCH v4] Sanity-check config variable names

[Junio C Hamano <gitster@pobox.com>]

Libor Pechacek <lpechacek@suse.cz> writes:

> diff --git a/builtin/config.c b/builtin/config.c
> index ca4a0db..dd17029 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -168,17 +167,30 @@ static int get_value(const char *key_, const char *regex_)
> ...
>  		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));
>  		if (regcomp(key_regexp, key, REG_EXTENDED)) {
>  			fprintf(stderr, "Invalid key pattern: %s\n", key_);
>  			goto free_strings;

This is not a new issue introduced by this series, but isn't this goto
leaking "key" by jumping over free(key) later in the function but before
its target?

>  		}
> +	} else {
> +		if (git_config_parse_key(key_, &key, NULL))
> +			goto free_strings;

This seemingly has the same issue but is worse than that.  You allocate
and overwrite "key" in git_config_parse_key(), so by calling the function
after allocating key in the caller, you immediately leak it.  The new copy
allocated inside the callee is freed at its end upon error return, so
jumping over free(key) in the caller does not leak, though.

> @@ -1124,59 +1187,22 @@ int git_config_set(const char *key, const char *value)
> ...
> +	ret = -git_config_parse_key(key, &store.key, &store.baselen);
> +	if (ret)
>  		goto out_free;

This '-' is very easy to miss; I'd rather see it spelled out with an
explanation.

But looking at the bigger picture, don't you think that an internal
function like git_config_set() should return negative on error, and
we should make it the caller's responsibility to turn it to a value
suitable for feeding exit(3)?  It obviously is a separate topic.


Here is a minimum fix-up on top of your patch.
Thanks.

diff --git a/builtin/config.c b/builtin/config.c
index dd17029..6754da8 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -166,8 +166,6 @@ static int get_value(const char *key_, const char *regex_)
 			system_wide = git_etc_gitconfig();
 	}
 
-	key = xstrdup(key_);
-
 	if (use_key_regexp) {
 		char *tl;
 
@@ -176,6 +174,8 @@ static int get_value(const char *key_, const char *regex_)
 		 * work for more complex patterns like "^[^.]*Foo.*bar".
 		 * Perhaps we should deprecate this altogether someday.
 		 */
+
+		key = xstrdup(key_);
 		for (tl = key + strlen(key) - 1;
 		     tl >= key && *tl != '.';
 		     tl--)
@@ -186,6 +186,7 @@ static int get_value(const char *key_, const char *regex_)
 		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));
 		if (regcomp(key_regexp, key, REG_EXTENDED)) {
 			fprintf(stderr, "Invalid key pattern: %s\n", key_);
+			free(key);
 			goto free_strings;
 		}
 	} else {
diff --git a/config.c b/config.c
index fde91f5..f758734 100644
--- a/config.c
+++ b/config.c
@@ -1141,7 +1141,8 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 			dot = 1;
 		/* Leave the extended basename untouched.. */
 		if (!dot || i > baselen) {
-			if (!iskeychar(c) || (i == baselen+1 && !isalpha(c))) {
+			if (!iskeychar(c) ||
+			    (i == baselen + 1 && !isalpha(c))) {
 				error("invalid key: %s", key);
 				goto out_free_ret_1;
 			}
@@ -1197,7 +1198,8 @@ int git_config_set_multivar(const char *key, const char *value,
 	else
 		config_filename = git_pathdup("config");
 
-	ret = -git_config_parse_key(key, &store.key, &store.baselen);
+	/* parse-key returns negative; flip the sign to feed exit(3) */
+	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
 	if (ret)
 		goto out_free;