From: Jeff King <peff@peff.net>
Date: Thu, 15 Nov 2012 16:11:50 GMT
Subject: Re: [PATCH] config: don't segfault when given --path with a missing value
Message-ID: <20121115161149.GB6157@sigill.intra.peff.net>
In-Reply-To: <20121115160847.GA6157@sigill.intra.peff.net>

On Thu, Nov 15, 2012 at 08:08:49AM -0800, Jeff King wrote:

> That is definitely the right thing to do. But do we also need to take
> note of the error for later? After this code:
> 
> >  	} else if (types == TYPE_PATH) {
> > -		git_config_pathname(&vptr, key_, value_);
> > -		must_free_vptr = 1;
> > +		must_free_vptr = !git_config_pathname(&vptr, key_, value_);
> 
> We don't have any clue that nothing got written into vptr. Which means
> it still points at the stack buffer "value", which contains
> uninitialized bytes. We will later try to print it, thinking it has the
> expanded path in it.
> 
> Do we need something like:
> 
>   if (!git_config_pathname(&vptr, key_, value_))
>           must_free_vptr = 1;
>   else
>           vptr = "";

Hmm, actually, we should probably propagate the error (I was thinking
for some reason this was in the listing code, but it is really about
getting a specific variable, and that variable does not have a sane
format. We'll already have printed the non-bool error, so we should
probably die. So more like:

  if (git_config_pathname(&vptr, key_, value_) < 0)
          return -1;
  must_free_vptr = 1;

-Peff