Re: [PATCH v7 6/6] branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks

From: Junio C Hamano <gitster@pobox.com>
To: Glen Choo <chooglen@google.com>
Cc: git@vger.kernel.org, "Jonathan Tan" <jonathantanmy@google.com>,
	"Josh Steadmon" <steadmon@google.com>,
	"Emily Shaffer" <emilyshaffer@google.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Philippe Blain" <levraiphilippeblain@gmail.com>
Subject: Re: [PATCH v7 6/6] branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks
Date: Thu, 27 Jan 2022 14:15:35 -0800	[thread overview]
Message-ID: <xmqqfsp8zuyg.fsf@gitster.g> (raw)
In-Reply-To: <20220124204442.39353-7-chooglen@google.com> (Glen Choo's message of "Mon, 24 Jan 2022 12:44:42 -0800")

Glen Choo <chooglen@google.com> writes:

> Signed-off-by: Glen Choo <chooglen@google.com>
> ---
>  branch.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index be33fe09fa..1e9a585633 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -239,7 +239,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  	if (track != BRANCH_TRACK_INHERIT)
>  		for_each_remote(find_tracked_branch, &tracking);
>  	else if (inherit_tracking(&tracking, orig_ref))
> -		return;
> +		goto cleanup;
>  
>  	if (!tracking.matches)
>  		switch (track) {
> @@ -249,7 +249,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  		case BRANCH_TRACK_INHERIT:
>  			break;
>  		default:
> -			return;
> +			goto cleanup;
>  		}
>  
>  	if (tracking.matches > 1)
> @@ -262,6 +262,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
>  				tracking.remote, tracking.srcs) < 0)
>  		exit(-1);
>  
> +cleanup:
>  	string_list_clear(tracking.srcs, 0);

Makes sense.  There is no other exit paths out of the function after
the tracking_srcs variable gets initialized, so this should be
covering everything.

Two tangential findings:

 * I see exit(-1) in the precontext of the final hunk.  We probably
   would want to fix it, as negative argument to exit(3) is
   misleading (the standard makes it clear that only the least
   significant 8 bits matter, so it is not that bad).

 * At the end, what is cleared is tracking.srcs, but because it is a
   pointer to the real resource we allocated on our stack, it would
   be cleaner to pass &tracking_srcs instead.

Both are not what this patch introduces, and are outside the scope
of this series.

Thanks.