From: Eric Sunshine <sunshine@sunshineco.com>
Date: Wed, 03 Feb 2016 01:47:14 GMT
Subject: Re: [PATCH v3 4/4] restore_env(): free the saved environment variable once we are done
Message-ID: <CAPig+cT8PhJU=9aS8NvkN9wx6imtACOS9rkgDJeJpN=CGikd7Q@mail.gmail.com>
In-Reply-To: <xmqqio26pt91.fsf_-_@gitster.mtv.corp.google.com>

On Tue, Feb 2, 2016 at 6:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Just like we free orig_cwd, which is the value of the original
> working directory saved in save_env_before_alias(), once we are
> done with it, the contents of orig_env[] array, saved in the
> save_env_before_alias() function should be freed; otherwise,
> the second and subsequent calls to save/restore pair will leak
> the memory allocated in save_env_before_alias().
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/git.c b/git.c
> @@ -54,10 +54,12 @@ static void restore_env(int external_alias)
>                 if (external_alias &&
>                     !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))
>                         continue;
> -               if (orig_env[i])
> +               if (orig_env[i]) {
>                         setenv(env_names[i], orig_env[i], 1);
> -               else
> +                       free(orig_env[i]);

Now that this is "well-protected"[1] against incorrect nesting, you
don't worry about the dangling pointers in static orig_env[], right?
(The same for the dangling pointer in static 'orig_cwd' after being
freed a bit earlier in this function, correct?)

[1]: via assert()

> +               } else {
>                         unsetenv(env_names[i]);
> +               }
>         }
>  }