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.
But certainly it would not be the first such instance in our code base.
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.
-Peff
