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

[Phillip Wood <phillip.wood123@gmail.com>]

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.

Out of interest how are you finding these leaks?

Thanks

Phillip

> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
>      sequencer: fix memory leak if todo_list_rearrange_squash() failed
>      
>      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.
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1965%2Fbrandb97%2Ffix-sequencer-todo-leak-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1965/brandb97/fix-sequencer-todo-leak-v1
> Pull-Request: https://github.com/git/git/pull/1965
> 
>   sequencer.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index b5c4043757e..5fb7b68a7ab 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6596,6 +6596,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>   	char **subjects;
>   	struct commit_todo_item commit_todo;
>   	struct todo_item *items = NULL;
> +	int ret = 0;
>   
>   	init_commit_todo_item(&commit_todo);
>   	/*
> @@ -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;
>   		}
>   
>   		repo_parse_commit(the_repository, item->commit);
> @@ -6729,6 +6730,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>   		todo_list->items = items;
>   	}
>   
> +cleanup:
>   	free(next);
>   	free(tail);
>   	for (i = 0; i < todo_list->nr; i++)
> @@ -6738,7 +6740,7 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>   
>   	clear_commit_todo_item(&commit_todo);
>   
> -	return 0;
> +	return ret;
>   }
>   
>   int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
> 
> base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75