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

From: Libor Pechacek <lpechacek@suse.cz>
Date: Feb 11, 2011, 18:52 UTC
Message-ID: <20110211185205.GB7406@localhost.suse.cz>
In-Reply-To: <7v7hd733q6.fsf@alter.siamese.dyndns.org>

On Thu 10-02-11 14:49:05, Junio C Hamano wrote:

Show 16 quoted lines

> 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.

Show 10 quoted lines

> >  		}
> > +	} 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.

Show 8 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.

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