Re: [PATCH v4] Sanity-check config variable names
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 10, 2011, 22:49 UTC
- Message-ID
- <7v7hd733q6.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20110130194041.GA25033@fm.suse.cz>
Libor Pechacek <lpechacek@suse.cz> writes:
Show 10 quoted lines
> 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.
Show 5 quoted lines
> @@ -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;