Re: [PATCH] exec_cmd: system_path memory leak fix

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 23, 2014, 20:07 UTC
Message-ID: <CAPig+cS11fQMUkn2D=3oqac=NA2wRp9rap_XOk=Jfj4pPGWP+Q@mail.gmail.com>
In-Reply-To: <87sih9en65.fsf@gmail.com>

On Sun, Nov 23, 2014 at 2:19 PM, Alex Kuleshov <kuleshovmail@gmail.com> wrote:

Show 16 quoted lines

>
> Signed-off-by: Alex Kuleshov <kuleshovmail@gmail.com>
> ---
>  exec_cmd.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 698e752..7ed9bcc 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -13,7 +13,7 @@ const char *system_path(const char *path)
>  #else
>         static const char *prefix = PREFIX;
>  #endif
> -       struct strbuf d = STRBUF_INIT;
> +       static struct strbuf d = STRBUF_INIT;

Curious. Did the unit tests pass with this change?

In the original code, the strbuf starts out empty each time system_path() is called, and strbuf_addf() populates it. After your change, the strbuf starts empty only on the very first call, and subsequent calls append more content rather than replacing it. At the very least, to fix, you now need to invoke strbuf_reset() before invoking strbuf_addf().

Show 21 quoted lines

>         if (is_absolute_path(path))
>                 return path;
> @@ -34,8 +34,7 @@ const char *system_path(const char *path)
>  #endif
>
>         strbuf_addf(&d, "%s/%s", prefix, path);
> -       path = strbuf_detach(&d, NULL);
> -       return path;
> +       return d.buf;
>  }
>
>  const char *git_extract_argv0_path(const char *argv0)
> @@ -94,7 +93,7 @@ static void add_path(struct strbuf *out, const char *path)
>  void setup_path(void)
>  {
>         const char *old_path = getenv("PATH");
> -       struct strbuf new_path = STRBUF_INIT;
> +       static struct strbuf new_path = STRBUF_INIT;
>
>         add_path(&new_path, git_exec_path());
>         add_path(&new_path, argv0_path);

Not sure what this change is about. The last couple lines of this function are:

    setenv("PATH", new_path.buf, 1);
    strbuf_release(&new_path);

which means that the buffer held by the strbuf is being released anyhow, whether static or not.