Re: [PATCH v7 6/6] branch.c: use 'goto cleanup' in setup_tracking() to fix memory leaks
- From
Glen Choo <chooglen@google.com>
- Date
- Jan 28, 2022, 19:44 UTC
- Message-ID
- <kl6lh79nmyqa.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <xmqqfsp8zuyg.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 46 quoted lines
> 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).Thanks for the reminder. We had this discussion previously and the conclusion was that I would send this cleanup as a separate series [1]. Unlike this relatively obvious cleanup, I think exit codes might spark a more involved discussion.
> > * 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.
Ah yes, that is true. I'll do that. If you (or others) prefer, I can move this patch to its own series, or possibly as part of a grab bag with the exit code fix.
Show 5 quoted lines
> > Both are not what this patch introduces, and are outside the scope > of this series. > > Thanks.
[1] https://lore.kernel.org/git/211210.86ee6ldwlc.gmgdl@evledraar.gmail.com