Re: [PATCH 04/11] branch: fix a leak in dwim_and_setup_tracking

From: Jeff King <peff@peff.net>
Date: Jun 12, 2023, 03:21 UTC
Message-ID: <20230612032106.GD306352@coredump.intra.peff.net>
In-Reply-To: <a6fec1ab-4006-2896-a2d9-dc0d8fa1c132@gmail.com>

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

> In e89f151db1 (branch: move --set-upstream-to behavior to
> dwim_and_setup_tracking(), 2022-01-28) the string returned by
> dwim_branch_start() was not freed, resulting in a memory leak.

Yep, looks good. One small comment:

Show 14 quoted lines

> diff --git a/branch.c b/branch.c
> index ba3914adf5..a7333a4c32 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref,
>  			     const char *orig_ref, enum branch_track track,
>  			     int quiet)
>  {
> -	char *real_orig_ref;
> +	char *real_orig_ref = NULL;
>  	dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL);
>  	setup_tracking(new_ref, real_orig_ref, track, quiet);
> +	free(real_orig_ref);
>  }

Adding the NULL-initialization signals that we expect that real_orig_ref might sometimes _not_ be filled in by dwim_branch_start(). But I think it always will be; and if it weren't, that would make passing it to setup_tracking() rather suspicious.

So I don't think the NULL initialization is wrong, and certainly it is more defensive. But I find it a little misleading.

-Peff