[05/11] branch: fix a leak in setup_tracking

Commit Message

Rubén Justo June 11, 2023, 6:49 p.m. UTC

In bdaf1dfae7 (branch: new autosetupmerge option "simple" for matching
branches, 2022-04-29) a new exit for setup_tracking() missed the
clean-up, producing a leak.

   $ git config branch.autoSetupMerge simple
   $ git remote add local .
   $ git update-ref refs/remotes/local/foo HEAD
   $ git branch bar local/foo

   Direct leak of 384 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in string_list_append_nodup string-list.c
       ... in find_tracked_branch branch.c
       ... in for_each_remote remote.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtinbranch.c
       ... in run_builtin git.c

   Indirect leak of 24 byte(s) in 1 object(s) allocated from:
       ... in xrealloc wrapper.c
       ... in strbuf_grow strbuf.c
       ... in strbuf_add strbuf.c
       ... in match_name_with_pattern remote.c
       ... in query_refspecs remote.c
       ... in remote_find_tracking remote.c
       ... in find_tracked_branch branch.c
       ... in for_each_remote remote.c
       ... in setup_tracking branch.c
       ... in create_branch branch.c
       ... in cmd_branch builtinbranch.c
       ... in run_builtin git.c

The return introduced in bdaf1dfae7 was to avoid setting up the
tracking, but even in that case it is still necessary to do the
clean-up.  Let's do it.

Signed-off-by: Rubén Justo <[e-mail]>
---
 branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jeff King June 12, 2023, 3:26 a.m. UTC | #1

On Sun, Jun 11, 2023 at 08:49:51PM +0200, Rubén Justo wrote:

> The return introduced in bdaf1dfae7 was to avoid setting up the
> tracking, but even in that case it is still necessary to do the
> clean-up.  Let's do it.

That may be a sign that the return introduced by that commit is in the
wrong spot (i.e., could we be checking it earlier and returning before
doing the work that led to the allocations?).

But I didn't look too carefully, and I think for the purposes of your
series it is OK to simply fix the leak without digging too far.

I'll cc the author (and quote the patch below) though, as sometimes in
cases like these they may be interested in looking deeper themselves.

-Peff

> Signed-off-by: Rubén Justo <[e-mail]>
> ---
>  branch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/branch.c b/branch.c
> index a7333a4c32..ff81c2266a 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -333,7 +333,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  		if (!skip_prefix(tracking.srcs->items[0].string,
>  				 "refs/heads/", &tracked_branch) ||
>  		    strcmp(tracked_branch, new_ref))
> -			return;
> +			goto cleanup;
>  	}
>  
>  	if (tracking.srcs->nr < 1)
> -- 
> 2.40.1

Rubén Justo June 16, 2023, 10:46 p.m. UTC | #2

On 11-jun-2023 23:26:15, Jeff King wrote:
> On Sun, Jun 11, 2023 at 08:49:51PM +0200, Rubén Justo wrote:
> 
> > The return introduced in bdaf1dfae7 was to avoid setting up the
> > tracking, but even in that case it is still necessary to do the
> > clean-up.  Let's do it.
> 
> That may be a sign that the return introduced by that commit is in the
> wrong spot (i.e., could we be checking it earlier and returning before
> doing the work that led to the allocations?).
> 
> But I didn't look too carefully, and I think for the purposes of your
> series it is OK to simply fix the leak without digging too far.

Thanks.

> 
> I'll cc the author (and quote the patch below) though, as sometimes in
> cases like these they may be interested in looking deeper themselves.

Perfect.  Thank you.

Patch

diff --git a/branch.c b/branch.c
index a7333a4c32..ff81c2266a 100644
--- a/branch.c
+++ b/branch.c
@@ -333,7 +333,7 @@  static void setup_tracking(const char *new_ref, const char *orig_ref,
 		if (!skip_prefix(tracking.srcs->items[0].string,
 				 "refs/heads/", &tracked_branch) ||
 		    strcmp(tracked_branch, new_ref))
-			return;
+			goto cleanup;
 	}
 
 	if (tracking.srcs->nr < 1)