Jeff King <peff@xxxxxxxx> 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 <peff@xxxxxxxx>
> ---
> 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...
