Re: [PATCH] exec_cmd: system_path memory leak fix

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2014, 18:51 UTC
Message-ID: <xmqqioi5ycme.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1416750981-24446-2-git-send-email-kuleshovmail@gmail.com>

0xAX <kuleshovmail@gmail.com> writes:

> Signed-off-by: 0xAX <kuleshovmail@gmail.com>
> ---

The comment on names I've already mentioned elsewhere.

You need a better explanation than a "no log message", as you are not doing "system-path memory leak fix".

You are doing a lot more.  Perhaps the story would start like this:

    system_path(): make the callers own the returned string

    The function sometimes returns a newly allocated string and
    sometimes returns a borrowed string, the latter of which the
    callers must not free().

    The existing callers all assume that the return value belongs to
    the callee and most of them copy it with strdup() when they want
    to keep it around.  They end up leaking the returned copy when
    the callee returned a new string.
    
    Change the contract between the callers and system_path() to
    make the returned string owned by the callers; they are
    responsible for freeing it when done, but they do not have to
    make their own copy to store it away.

    This accidentally fixes some unsafe callers as well.  For
    example, ...

>  exec_cmd.c | 25 ++++++++++++++++---------
>  exec_cmd.h |  4 ++--
>  git.c      | 12 +++++++++---

Even though I said that this changes the contract between the caller and the callee and make things wasteful for some, I personally think it is going in the right direction.

The change accidentally fixes some unsafe callers. For example, the first hit from "git grep system_path" is this:

    attr.c- static const char *system_wide;
    attr.c- if (!system_wide)
    attr.c:         system_wide = system_path(ETC_GITATTRIBUTES);
    attr.c- return system_wide;

This is obviously unsafe for a volatile return value from the callee and needs to have strdup() on it, but with the patch there no longer is need for such a caller-side strdup().

But this change also introduces new bugs, I think. For example, the second hit from "git grep system_path" is this:

  builtin/help.c: strbuf_addstr(&new_path, system_path(GIT_MAN_PATH));

Now the caller owns and is responsible for freeing the returned value, but without opening the file in question in an editor or a pager we can tell immediately that there is no way this line is not adding a new memory leak.

Show 38 quoted lines

> index 698e752..08f8f80 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -6,7 +6,7 @@
>  static const char *argv_exec_path;
>  static const char *argv0_path;
>  
> -const char *system_path(const char *path)
> +char *system_path(const char *path)
>  {
>  #ifdef RUNTIME_PREFIX
>  	static const char *prefix;
> @@ -14,9 +14,10 @@ const char *system_path(const char *path)
>  	static const char *prefix = PREFIX;
>  #endif
>  	struct strbuf d = STRBUF_INIT;
> +	char *new_path = NULL;
>  
>  	if (is_absolute_path(path))
> -		return path;
> +		return strdup(path);
>  
>  #ifdef RUNTIME_PREFIX
>  	assert(argv0_path);
> @@ -32,10 +33,13 @@ const char *system_path(const char *path)
>  				"Using static fallback '%s'.\n", prefix);
>  	}
>  #endif
> -
>  	strbuf_addf(&d, "%s/%s", prefix, path);
> -	path = strbuf_detach(&d, NULL);
> -	return path;
> +	new_path = malloc((strlen(prefix) + strlen(path)) + 2);
> +	sprintf(new_path, "%s/%s", prefix, path);
> +
> +	strbuf_release(&d);
> +
> +	return new_path;

Are you duplicating what strbuf_addf() is doing on the strbuf d, manually creating the same in new_path, while discarding what the existing code you did not remove with this patch already computed?

Isn't it sufficient to add strdup(path) for the absolute case and do nothing else to this function? I have no idea what you are doing here.