Re: [PATCH] diff: ensure correct lifetime of external_diff_cmd

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 9, 2019, 23:10 UTC
Message-ID: <CAPig+cQKnEWb+co_NJ0UyZbXZrvx2KsbS_ZugdyjjYZcz8tjvw@mail.gmail.com>
In-Reply-To: <20190109221007.21624-1-kgybels@infogroep.be>

On Wed, Jan 9, 2019 at 5:19 PM Kim Gybels <kgybels@infogroep.be> wrote:

Show 15 quoted lines

> According to getenv(3)'s notes:
> [...]
> Since strings returned by getenv() are allowed to change on subsequent
> calls to getenv(), make sure to duplicate when caching external_diff_cmd
> from environment.
> [...]
> Signed-off-by: Kim Gybels <kgybels@infogroep.be>
> ---
> diff --git a/diff.c b/diff.c
> @@ -492,6 +492,9 @@ static const char *external_diff(void)
>         external_diff_cmd = getenv("GIT_EXTERNAL_DIFF");
>         if (!external_diff_cmd)
>                 external_diff_cmd = external_diff_cmd_cfg;
> +       else
> +               external_diff_cmd = xstrdup(external_diff_cmd);

Make sense.

Not shown in the context is that 'external_diff_cmd' is static, so this is not (in the traditional sense) leaking the dup'd string.

I do find that the logic is obscured by doing the xstrdup() in the 'else' arm; it would be easier to grok if the condition was reversed and xstrdup() done in the 'then' arm.

However, you might also consider using xstrdup_or_null(), like this:

    external_diff_cmd = xstrdup_or_null(getenv(...));
    if (!external_diff_cmd)
        ...as before...

>         done_preparing = 1;
>         return external_diff_cmd;
>  }