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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: May 15, 2025, 10:08 UTC
Message-ID: <935822b9-33e5-457e-95a7-64058777b52a@gmail.com>
In-Reply-To: <pull.1964.git.git.1747226641249.gitgitgadget@gmail.com>

On 14/05/2025 13:44, Lidong Yan via GitGitGadget wrote:

Show 8 quoted lines

> From: Lidong Yan <502024330056@smail.nju.edu.cn>
> 
> In sequencer.c:update_squash_messages, `repo_logmsg_reencode` returns
> either an allocated reencode string or commit buffer if no encode is
> needed. To free `repo_logmsg_reencode` result, `repo_unuse_commit_buffer`
> should be used. However, when encountering the error("unknown command..."),
> the absence of `repo_unuse_commit_buffer` results in a memory leak. I
> think we should add a `repo_unuse_commit_buffer` before return.

Looking at the code, if we reaching that call to error() is a programming error as we should only call update_squash_messages() if command is TODO_FIXUP or TODO_SQUASH so I think we'd be better to replace error(...) with BUG(...) which calls abort() which means we don't care if there is a leak or not.

Thanks

Phillip

Show 37 quoted lines

> Signed-off-by: Lidong Yan <502024330056@smail.nju.edu.cn>
> ---
>      sequencer: fix memory leak if update_squash_messages() failed
>      
>      In sequencer.c:update_squash_messages, repo_logmsg_reencode returns
>      either an allocated reencode string or commit buffer if no encode is
>      needed. To free repo_logmsg_reencode result, repo_unuse_commit_buffer
>      should be used. However, when encountering the error("unknown
>      command..."), the absence of repo_unuse_commit_buffer results in a
>      memory leak. I think we should add a repo_unuse_commit_buffer before
>      return.
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1964%2Fbrandb97%2Ffix-sequencer-leak-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1964/brandb97/fix-sequencer-leak-v1
> Pull-Request: https://github.com/git/git/pull/1964
> 
>   sequencer.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index b5c4043757e..f288a303eaa 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2138,8 +2138,10 @@ static int update_squash_messages(struct repository *r,
>   		strbuf_addstr(&buf, "\n\n");
>   		strbuf_add_commented_lines(&buf, body, strlen(body),
>   					   comment_line_str);
> -	} else
> +	} else {
> +		repo_unuse_commit_buffer(r, commit, message);
>   		return error(_("unknown command: %d"), command);
> +	}
>   	repo_unuse_commit_buffer(r, commit, message);
>   
>   	if (!res)
> 
> base-commit: 6f84262c44a89851c3ae5a6e4c1a9d06b2068d75