Re: [PATCH] config: don't segfault when given --path with a missing value

From: Jeff King <peff@peff.net>
Date: Nov 15, 2012, 16:11 UTC
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:

Show 19 quoted lines

> 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