[PATCH 1/3] connect.c: Fix memory leak

[PATCH 1/3] connect.c: Fix memory leak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 connect.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/connect.c b/connect.c
index ce0e121..6090211 100644
--- a/connect.c
+++ b/connect.c
@@ -739,6 +739,7 @@ struct child_process *git_connect(int fd[2], const char *url,
 
 				free(hostandport);
 				free(path);
+				free(conn);
 				return NULL;
 			} else {
 				ssh = getenv("GIT_SSH_COMMAND");
-- 
2.3.0.81.gc37f363

[PATCH 2/3] bundle.c: fix memory leak

[Stefan Beller]

The continue statements nearby also have an accompanying free(ref);

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 bundle.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/bundle.c b/bundle.c
index 2e2dbd5..534783d 100644
--- a/bundle.c
+++ b/bundle.c
@@ -342,6 +342,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 		if (e->item->type == OBJ_TAG &&
 				!is_tag_in_date_range(e->item, revs)) {
 			e->item->flags |= UNINTERESTING;
+			free(ref);
 			continue;
 		}
 
-- 
2.3.0.81.gc37f363

Re: [PATCH 2/3] bundle.c: fix memory leak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The continue statements nearby also have an accompanying free(ref);
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

I wonder what happens when dwim_ref() returned 2 or more, though.

>  bundle.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/bundle.c b/bundle.c
> index 2e2dbd5..534783d 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -342,6 +342,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
>  		if (e->item->type == OBJ_TAG &&
>  				!is_tag_in_date_range(e->item, revs)) {
>  			e->item->flags |= UNINTERESTING;
> +			free(ref);
>  			continue;
>  		}

Re: [PATCH 2/3] bundle.c: fix memory leak

[Stefan Beller]

On Tue, Mar 10, 2015 at 3:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The continue statements nearby also have an accompanying free(ref);
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> I wonder what happens when dwim_ref() returned 2 or more, though.

That should also be fixed I guess. I'll look into it.

These one liner fixes are mostly done as a side project
having fun, just doing what the code analysis tools says,
sorry for not catching that one.

Maybe instead of the reoccuring pattern

    free(ref);
    continue;

we could just have a

    goto cleanup

which goes to the end of the loop where we have
the free anyway.

Re: [PATCH 2/3] bundle.c: fix memory leak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Mar 10, 2015 at 3:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> The continue statements nearby also have an accompanying free(ref);
>>>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>
>> I wonder what happens when dwim_ref() returned 2 or more, though.
>
> That should also be fixed I guess. I'll look into it.
>
> These one liner fixes are mostly done as a side project
> having fun, just doing what the code analysis tools says,
> sorry for not catching that one.
>
> Maybe instead of the reoccuring pattern
>
>     free(ref);
>     continue;
>
> we could just have a
>
>     goto cleanup
>
> which goes to the end of the loop where we have
> the free anyway.

Yeah, I suspect that the end result may look more like that if you
cover the dwim_ref() one as well.

Thanks.

[PATCH 2/3] bundle.c: fix memory leak

[Stefan Beller]

There was one continue statement without an accompanying `free(ref)`.
Instead of adding that, replace all the free&&continue with a goto
just after writing the refs, where we'd do the free anyway and then
reloop.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 bundle.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/bundle.c b/bundle.c
index 2e2dbd5..f732c92 100644
--- a/bundle.c
+++ b/bundle.c
@@ -334,7 +334,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 		if (e->item->flags & UNINTERESTING)
 			continue;
 		if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
-			continue;
+			goto skip_write_ref;
 		if (read_ref_full(e->name, RESOLVE_REF_READING, sha1, &flag))
 			flag = 0;
 		display_ref = (flag & REF_ISSYMREF) ? e->name : ref;
@@ -342,7 +342,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 		if (e->item->type == OBJ_TAG &&
 				!is_tag_in_date_range(e->item, revs)) {
 			e->item->flags |= UNINTERESTING;
-			continue;
+			goto skip_write_ref;
 		}
 
 		/*
@@ -357,8 +357,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 		if (!(e->item->flags & SHOWN) && e->item->type == OBJ_COMMIT) {
 			warning(_("ref '%s' is excluded by the rev-list options"),
 				e->name);
-			free(ref);
-			continue;
+			goto skip_write_ref;
 		}
 		/*
 		 * If you run "git bundle create bndl v1.0..v2.0", the
@@ -388,8 +387,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 				obj->flags |= SHOWN;
 				add_pending_object(revs, obj, e->name);
 			}
-			free(ref);
-			continue;
+			goto skip_write_ref;
 		}
 
 		ref_count++;
@@ -397,6 +395,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
 		write_or_die(bundle_fd, " ", 1);
 		write_or_die(bundle_fd, display_ref, strlen(display_ref));
 		write_or_die(bundle_fd, "\n", 1);
+ skip_write_ref:
 		free(ref);
 	}
 
-- 
2.3.0.81.gc37f363

Re: [PATCH 2/3] bundle.c: fix memory leak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> There was one continue statement without an accompanying `free(ref)`.
> Instead of adding that, replace all the free&&continue with a goto
> just after writing the refs, where we'd do the free anyway and then
> reloop.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  bundle.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 2e2dbd5..f732c92 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -334,7 +334,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
>  		if (e->item->flags & UNINTERESTING)
>  			continue;
>  		if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
> -			continue;
> +			goto skip_write_ref;

Thanks.

I briefly wondered if we need to initialize ref=NULL for this to
work, because dwim_ref() does not need to clear ref when it says
"nothing matches", but the function actually clears it upfront, so
we should be OK.

> ...
> @@ -397,6 +395,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)
>  		write_or_die(bundle_fd, " ", 1);
>  		write_or_die(bundle_fd, display_ref, strlen(display_ref));
>  		write_or_die(bundle_fd, "\n", 1);
> + skip_write_ref:
>  		free(ref);
>  	}

[PATCH 3/3] builtin/help.c: fix memory leak

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/help.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/help.c b/builtin/help.c
index 6133fe4..a1f5a0a 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -456,7 +456,7 @@ static void list_common_guides_help(void)
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
 	int nongit;
-	const char *alias;
+	char *alias;
 	enum help_format parsed_help_format;
 
 	argc = parse_options(argc, argv, prefix, builtin_help_options,
@@ -499,6 +499,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	alias = alias_lookup(argv[0]);
 	if (alias && !is_git_command(argv[0])) {
 		printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
+		free(alias);
 		return 0;
 	}
 
-- 
2.3.0.81.gc37f363

Re: [PATCH 3/3] builtin/help.c: fix memory leak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/help.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/help.c b/builtin/help.c
> index 6133fe4..a1f5a0a 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -456,7 +456,7 @@ static void list_common_guides_help(void)
>  int cmd_help(int argc, const char **argv, const char *prefix)
>  {
>  	int nongit;
> -	const char *alias;
> +	char *alias;
>  	enum help_format parsed_help_format;
>  
>  	argc = parse_options(argc, argv, prefix, builtin_help_options,
> @@ -499,6 +499,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>  	alias = alias_lookup(argv[0]);
>  	if (alias && !is_git_command(argv[0])) {
>  		printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
> +		free(alias);

Hmph, does this memory belong to us, or are we peeking into the
cached data in the config cache layer?


>  		return 0;
>  	}

Re: [PATCH 3/3] builtin/help.c: fix memory leak

[Stefan Beller]

On Tue, Mar 10, 2015 at 3:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  builtin/help.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/help.c b/builtin/help.c
>> index 6133fe4..a1f5a0a 100644
>> --- a/builtin/help.c
>> +++ b/builtin/help.c
>> @@ -456,7 +456,7 @@ static void list_common_guides_help(void)
>>  int cmd_help(int argc, const char **argv, const char *prefix)
>>  {
>>       int nongit;
>> -     const char *alias;
>> +     char *alias;
>>       enum help_format parsed_help_format;
>>
>>       argc = parse_options(argc, argv, prefix, builtin_help_options,
>> @@ -499,6 +499,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>>       alias = alias_lookup(argv[0]);
>>       if (alias && !is_git_command(argv[0])) {
>>               printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
>> +             free(alias);
>
> Hmph, does this memory belong to us, or are we peeking into the
> cached data in the config cache layer?

So  alias_lookup(..) is a wrapper around git_config_string essentially
(some more git_config_functions are involved, but we eventually reach
git_config_string), where we have

    *dest = xstrdup(value);

so I think we need to free that memory, as the config cache layer doesn't
care any more.

Re: [PATCH 3/3] builtin/help.c: fix memory leak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Hmph, does this memory belong to us, or are we peeking into the
>> cached data in the config cache layer?
>
> So  alias_lookup(..) is a wrapper around git_config_string essentially
> (some more git_config_functions are involved, but we eventually reach
> git_config_string), where we have
>
>     *dest = xstrdup(value);
>
> so I think we need to free that memory, as the config cache layer doesn't
> care any more.

Ah, OK, thanks.

Re: [PATCH 1/3] connect.c: Fix memory leak

[Torsten Bögershausen]

On 2015-03-09 17.58, Stefan Beller wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  connect.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/connect.c b/connect.c
> index ce0e121..6090211 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -739,6 +739,7 @@ struct child_process *git_connect(int fd[2], const char *url,
>  
>  				free(hostandport);
>  				free(path);
> +				free(conn);
>  				return NULL;
>  			} else {
>  				ssh = getenv("GIT_SSH_COMMAND");
> 
Nice catch, thanks.
(Even if it is not sooo serious today, the current implementation of Git just dies
when connection is NULL)

Re: [PATCH 1/3] connect.c: Fix memory leak

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  connect.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/connect.c b/connect.c
> index ce0e121..6090211 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -739,6 +739,7 @@ struct child_process *git_connect(int fd[2], const char *url,
>  
>  				free(hostandport);
>  				free(path);
> +				free(conn);
>  				return NULL;

The lack of sufficient context in the patch made me wonder if merely
freeing conn is sufficient or do we need some process related
clean-up before freeing it.

It appears that this codepath is used only when debugging with
CONNECT_DIAG_URL and we didn't start any process --- we only
cleared it with a few arrays initialized to empty at this point of
the code.

So it should be safe.  I wish I didn't have to do the digging
myself, though ;-)

>  			} else {
>  				ssh = getenv("GIT_SSH_COMMAND");