Re: [PATCH v7 6/6] branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 27, 2022, 22:15 UTC
- Message-ID
- <xmqqfsp8zuyg.fsf@gitster.g>
- In-Reply-To
- <20220124204442.39353-7-chooglen@google.com>
Glen Choo <chooglen@google.com> writes:
Show 33 quoted lines
> 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.