Re: [PATCH 2/3] setup_path(): Free temporary buffer

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2011, 20:14 UTC
Message-ID: <7v7hc1cvdt.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1300130318-11279-3-git-send-email-cmn@elego.de>

Carlos Martín Nieto <cmn@elego.de> writes:

Show 7 quoted lines

> Make sure the pointer git_exec_path() returns is in the heap and free
> it after it's no longer needed.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
>  exec_cmd.c |    9 ++++++---
>  1 files changed, 6 insertions(+), 3 deletions(-)

The proposed log message does not explain half of what this patch does, if I am reading the patch correctly. Here is what I would consider as a fair description:

    Subject: git_exec_path: always return a free-able string

    git_exec_path() returns a string that the callers do not have to free
    in most cases, but when it calls into system_path() and ends up going
    into runtime-prefix codepath, it allocates an extra string on the
    heap, which ends up leaking because the callers are not allowed to
    free it.

    In order to allow the callers to clean up in all cases, change the API
    of git_exec_path() to always return a string on heap.  This requires
    all the callers to free it.

    Update only one caller setup_path() to follow the new API, but leave
    other callers to leak even in normal cases.  The caller in git.c exits
    immediately after using it, so we don't care about the leak there
    anyway.  Also help.c has a few calls to it but the number of calls to
    the function is small and bounded, so the leak is small and we don't
    care.

And then reviewers can agree or disagree if the small leaks in git.c (just one string allocation that immediately is followed by exit after its use) and help.c (in list_commands() and load_commands_list(), neither of which is called millions of times anyway) are OK to accept.

I tend to think they are Ok, but then I also tend to think one leak of exec-path return value in setup_path() is perfectly fine for the same reason, so in that sense I don't see a point in this patch...

Show 33 quoted lines

> diff --git a/exec_cmd.c b/exec_cmd.c
> index 38545e8..c16c3d4 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -73,11 +73,11 @@ const char *git_exec_path(void)
>  	const char *env;
>  
>  	if (argv_exec_path)
> -		return argv_exec_path;
> +		return xstrdup(argv_exec_path);
>  
>  	env = getenv(EXEC_PATH_ENVIRONMENT);
>  	if (env && *env) {
> -		return env;
> +		return xstrdup(env);
>  	}
>  
>  	return system_path(GIT_EXEC_PATH);
> @@ -99,10 +99,13 @@ void setup_path(void)
>  {
>  	const char *old_path = getenv("PATH");
>  	struct strbuf new_path = STRBUF_INIT;
> +	char *exec_path = (char *) git_exec_path();
>  
> -	add_path(&new_path, git_exec_path());
> +	add_path(&new_path, exec_path);
>  	add_path(&new_path, argv0_path);
>  
> +	free(exec_path);
> +
>  	if (old_path)
>  		strbuf_addstr(&new_path, old_path);
>  	else