Jeff King <p...@peff.net> writes:

> We save the result of $GIT_INDEX_FILE so that we can restore it after
> setting it to a new value and running add--interactive. However, the
> pointer returned by getenv() is not guaranteed to be valid after calling
> setenv(). This _usually_ works fine, but can fail if libc needs to
> reallocate the environment block during the setenv().
>
> Let's just duplicate the string, so we know that it remains valid.
>
> In the long run it may be more robust to teach interactive_add() to take
> a set of environment variables to pass along to run-command when it
> execs add--interactive. And then we would not have to do this
> save/restore dance at all. But this is an easy fix in the meantime.
>
> Signed-off-by: Jeff King <p...@peff.net>
> ---
>  builtin/commit.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> 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...


Reply via email to