Re: [PATCH 2/6] commit: copy saved getenv() result

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 15, 2019, 14:05 UTC
Message-ID: <nycvar.QRO.7.76.6.1901151502130.41@tvgsbejvaqbjf.bet>
In-Reply-To: <20190112102635.GA16633@sigill.intra.peff.net>

Hi Peff,

On Sat, 12 Jan 2019, Jeff King wrote:

Show 37 quoted lines

> On Fri, Jan 11, 2019 at 07:07:15PM -0800, Junio C Hamano wrote:
> 
> > > diff --git a/builtin/commit.c b/builtin/commit.c
> > > index 004b816635..7d2e0b61e5 100644
> > > --- a/builtin/commit.c
> > > +++ b/builtin/commit.c
> > > @@ -351,7 +351,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
> > >  		if (write_locked_index(&the_index, &index_lock, 0))
> > >  			die(_("unable to create temporary index"));
> > >  
> > > -		old_index_env = getenv(INDEX_ENVIRONMENT);
> > > +		old_index_env = xstrdup_or_null(getenv(INDEX_ENVIRONMENT));
> > >  		setenv(INDEX_ENVIRONMENT, get_lock_file_path(&index_lock), 1);
> > >  
> > >  		if (interactive_add(argc, argv, prefix, patch_interactive) != 0)
> > > @@ -361,6 +361,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
> > >  			setenv(INDEX_ENVIRONMENT, old_index_env, 1);
> > >  		else
> > >  			unsetenv(INDEX_ENVIRONMENT);
> > > +		FREE_AND_NULL(old_index_env);
> > >  
> > >  		discard_cache();
> > >  		read_cache_from(get_lock_file_path(&index_lock));
> > 
> > Even though it is not wrong per-se to assign a NULL to the
> > now-no-longer-referenced variable, I do not quite get why it is
> > free-and-null, not a straight free.  This may be a taste-thing,
> > though.
> > 
> > Even if a future update needs to make it possible to access
> > old_index_env somewhere in the block after discard_cache() gets
> > called, we would need to push down the free (or free-and-null) to
> > prolong its lifetime a bit anyway, so...
> 
> My thinking was that if we simply call free(), then the variable is left
> as a dangling pointer for the rest of the function, making it easy to
> accidentally use-after-free.

FWIW I thought that was your reasoning (and did not think of asking you about it) and totally agree with it.

It is *too* easy not to realize that the `free()` call needs to be moved, but a segmentation fault is a very strong indicator that it should be moved.

> But certainly it would not be the first such instance in our code base.

Just because a lot of our code has grown historically does not mean that we need to add code that shares the same shortcomings. FREE_AND_NULL() was not available for a long time, after all, so it is understandable that we did not use it back then. But it is available now, so we no longer have an excuse to add less defensive code.

> In theory a static analyzer should easily be able to figure out such a
> problem, too, so maybe it is not worth being defensive about.

How often do you run a static analyzer?

My point being: if we can prevent future mistakes easily, and it does not add too much code churn, why not just do it. No need to rely on fancy stuff that might not even be available on your preferred platform.

Thanks, Dscho