Re: [PATCH] sequencer: fix memory leak if `todo_list_rearrange_squash()` failed

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2025, 16:37 UTC
Message-ID: <xmqqecwp3k5h.fsf@gitster.g>
In-Reply-To: <52e0a717-f7f0-401d-b621-f1f6e028e595@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 12 quoted lines

> On 14/05/2025 14:53, Lidong Yan via GitGitGadget wrote:
>> From: Lidong Yan <502024330056@smail.nju.edu.cn>
>> In sequencer.c:todo_list_rearrange_squash, if it fails, memory
>> allocated in `next`, `tail`, `subjects` and `subject2item` will leak.
>> Jump to cleanup label before return could fix this leak problem.
>
> You could mention that you're adding the cleanup label.
>
> I suspect reaching this condition is a bug as well because we should
> only rearrange the todo list when the rebase starts. However I'm not
> 100% sure that it is impossible to trigger this condition so lets free
> it as you suggest. The code changes look good.

I wonder if there is a way (sort of using BUG("") but not as severe as killing the program) for us to mark "we do not expect this code path to be taken" and ask static analysis or fuzzers to disprove such assertions.

The message being an error(), without a silent return doing nothing, smells like a good enough sign, at least to me, that the original author intended not to call this helper function on a list that was already rearranged, so turning this into BUG("") may be something we would want to consider doing in the longer term.

Show 8 quoted lines

>> @@ -6626,8 +6627,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>>   		}
>>     		if (is_fixup(item->command)) {
>> -			clear_commit_todo_item(&commit_todo);
>> -			return error(_("the script was already rearranged."));
>> +			ret = error(_("the script was already rearranged."));
>> +			goto cleanup;
>>   		}

Thanks.