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

[Libor Pechacek <lpechacek@suse.cz>]

On Thu 10-02-11 14:49:05, Junio C Hamano wrote:
> 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?

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