Re: [PATCH] Git segmentation faults if submodule path is empty.

From: Jeff King <peff@peff.net>
Date: Aug 16, 2013, 13:09 UTC
Message-ID: <20130816130957.GB20138@sigill.intra.peff.net>
In-Reply-To: <520DCB4B.6090309@web.de>

On Fri, Aug 16, 2013 at 08:48:43AM +0200, Jens Lehmann wrote:

Show 8 quoted lines

> > This patch addresses the issue by returning from the function if
> > 'value' is null before the call to xstrdup is made.
> 
> Hmm, I'm not sure silently ignoring the misconfiguration is the best
> way to go. A submodule config having a path setting without a value
> is broken (while a submodule setting without a subsection configures
> something else, so the "|| !name" below is fine). So I believe we
> should complain to the user when "value" is NULL.

Yeah, the usual behavior is to catch it and return config_error_nonbool (because a key without a value is an implicit-true boolean). Most code does this via git_config_string, but what the submodule code is doing is too tricky and custom to use that stock function.

Show 5 quoted lines

> On the other hand this should only happen for the three options we do
> parse, as some users (e.g. git-submodule.sh) use other configurations
> for which a missing value may be fine. Please see the "lacks value"
> errors in read_merge_config() of ll-merge.c for an example of how to
> deal with that.

Those spots in ll-merge should probably be using config_error_nonbool, if only for consistency with the rest of the code base.

> > -	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name)
> > +	if (parse_config_key(var, "submodule", &name, &namelen, &key) < 0 || !name || !value)
> >  		return 0;

I think this is also the wrong place to make the check, anyway. It is saying that all values of submodule.X.Y must be non-NULL. But that is not true. The submodule.X.fetchRecurseSubmodules option can be a boolean, and:

  [submodule "foo"]
    fetchRecurseSubmodules

is perfectly valid (and is broken by this patch).

-Peff