Carlos MartÃn Nieto <cmn@xxxxxxxx> writes:
> 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@xxxxxxxx>
> ---
> 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...
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html