From: Phillip Wood Date: Thu, 15 May 2025 10:08:36 GMT Subject: Re: [PATCH] sequencer: fix memory leak if `update_squash_messages()` failed Message-ID: <935822b9-33e5-457e-95a7-64058777b52a@gmail.com> In-Reply-To: On 14/05/2025 13:44, Lidong Yan via GitGitGadget wrote: > 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 > 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