Re: [PATCH] reduce_heads: fix memory leaks
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Nov 2, 2017, 10:45 UTC
- Message-ID
- <CAN0heSpgUBy4a6iok4MoDqJ__hZtrXvfApcxjHuS0vOBKSuShg@mail.gmail.com>
- In-Reply-To
- <xmqq7ev9s7bp.fsf@gitster.mtv.corp.google.com>
On 2 November 2017 at 04:11, Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
> Martin Ågren <martin.agren@gmail.com> writes:
>
>> diff --git a/builtin/merge-base.c b/builtin/merge-base.c
>> index 6dbd167d3..b1b7590c4 100644
>> --- a/builtin/merge-base.c
>> +++ b/builtin/merge-base.c
>> @@ -59,6 +59,8 @@ static int handle_independent(int count, const char **args)
>> commit_list_insert(get_commit_reference(args[i]), &revs);
>>
>> result = reduce_heads(revs);
>> + free_commit_list(revs);
>> +
>> if (!result)
>> return 1;
>
> The post-context of this hunk continues like so:
>
> while (result) {
> printf("%s\n", oid_to_hex(&result->item->object.oid));
> result = result->next;
> }
> return 0;
> }
>
> and we end up leaking "result". This function is directly called from
> cmd_merge_base() and its value is returned to main(), so leaking it
> is not that a grave offence, but that excuse applies equally well to
> revs.Good catch. I even have a patch to address the leak of `result`, except I seem to have sorted it into another pile. For this series I just grepped for "reduce_heads" and didn't stop to think about using UNLEAK, nor about the leaking of `result`.
> I can see you are shooting for minimum change in this patch, but if > we were writing this code in a codebase where reduce_heads_replace() > is already available, I would imagine that we wouldn't use two separate > variables, perhaps?
The way my other patch addresses the leaking of `result` is that it rewrites the loop to avoid losing the original value of `result`, so that it can be UNLEAK-ed at the very end. (That makes it obvious where the leak happens, compared to adding an UNLEAK a few lines up.) If I do `reduce_heads_replace(&revs)`, I'll need to touch the loop anyway, and then I could probably just as well UNLEAK a little while at it.
I'll get to this within the next couple of days, then I'll see what it looks like.
Thanks for your feedback.