On Thu 10-02-11 14:49:05, Junio C Hamano wrote:
> Libor Pechacek <lpechacek@xxxxxxx> 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?
Yes, sure and not only there. A few lines below there is the compilation of
the value regex. Freeing the memory allocated for key storage obviously needs
a little bit more work and I'd like to post the fix as a separate patch.
> > }
> > + } 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.
Negligence on my part, thanks for catching it.
> > @@ -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.
Agree. It would be yet better to get rid this weird transformation at all.
> 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.
Agree and not only that. The exit codes are inconsistent between "set" and
"get" operation[1]. IMHO it's also questionable to feed exit(3) with negative
values.
Would you be in favor of getting the exit codes consistent and documented?
> Here is a minimum fix-up on top of your patch.
Squashed into my patch, with the exception of the memleak fix, which I'm going
to post separately.
> Thanks.
Thanks for the opportunity to contribute. :)
Libor
[1] $ ./git-config x.x x [; echo $?
error: invalid pattern: [
6
$ ./git-config --get x.x [; echo $?
Invalid pattern: [
255
--
Libor Pechacek
SUSE L3 Team, Prague
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html