[PATCH] git.c: two memory leak fixes

[PATCH] git.c: two memory leak fixes

[Alexander Kuleshov]

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.

2. Memory allocated for alias_string variable in the alias_lookup function,
need to free it.

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));
-- 
2.3.3.469.g69a3822.dirty

Re: [PATCH] git.c: two memory leak fixes

[Junio C Hamano]

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

> 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?

> 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));

Re: [PATCH] git.c: two memory leak fixes

[Alexander Kuleshov]

Hello Junio,

>>This is technically correct, but do we really care about it?

Yes, as i saw handle_alias called once, and we no need to care about 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.

Yes, right. sorry for the noise.