Re: [PATCH] git.c: two memory leak fixes
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 15, 2015, 02:18 UTC
- Message-ID
- <xmqq3857vw9b.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1426358613-30180-1-git-send-email-kuleshovmail@gmail.com>
Alexander Kuleshov <kuleshovmail@gmail.com> writes:
Show 6 quoted lines
> This patch provides fixes for two minor memory leaks in the > handle_alias function: > > 1. We allocate memory for alias_argv with the xmalloc function call, > after run_command_v_opt function will be executed we no need in this > variable anymore, so let's free it.
This is technically correct, but do we really care about it?
We have finished running the command and all that is left is either to exit(ret) or to die(); either will let the operating system clear the memory together with the entire process for us.
The "normal exit" codepath still holds a live "alias_string" when it calls exist(ret), but you do not free it even after this patch, which is an inconsistent stance to take. I think it is OK not to bother freeing alias_string just before exit(), and I would apply the same reasoning to alias_argv[], too.
> 2. Memory allocated for alias_string variable in the alias_lookup function, > need to free it.
I think it is wrong to free alias_string after passing it to split_cmdline() and while you still need to use the new_argv.
Reading the split_cmdline() implementation again, the new argv which is an array of pointers is newly allocated, but I think the pointers in the array point into the cmdline parameter. From the caller's point of view, new_argv[0] points at the beginning of alias_string, new_argv[1] points at the beginning of the second "word" of alias_string, etc., all of which will become invalid if you free alias_string, no?
Show 25 quoted lines
> Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
> ---
> git.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/git.c b/git.c
> index 086fac1..501e5bd 100644
> --- a/git.c
> +++ b/git.c
> @@ -252,6 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)
> alias_argv[argc] = NULL;
>
> ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
> + free(alias_argv);
> if (ret >= 0) /* normal exit */
> exit(ret);
>
> @@ -259,6 +260,7 @@ static int handle_alias(int *argcp, const char ***argv)
> alias_command, alias_string + 1);
> }
> count = split_cmdline(alias_string, &new_argv);
> + free(alias_string);
> if (count < 0)
> die("Bad alias.%s string: %s", alias_command,
> split_cmdline_strerror(count));