[PATCH v3 6/8] sequencer.c: always free() the "msgbuf" in do_pick_commit()
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jan 18, 2023, 16:09 UTC
- Message-ID
- <patch-v3-6.8-c07dc006c6d-20230118T160600Z-avarab@gmail.com>
- In-Reply-To
- <cover-v3-0.8-00000000000-20230118T160600Z-avarab@gmail.com>
In [1] the strbuf_release(&msgbuf) was moved into this do_pick_commit(), but didn't take into account the case of [2], where we'd return before the strbuf_release(&msgbuf).
Then when the "fixup" support was added in [3] this leak got worse, as in this error case we added another place where we'd "return" before reaching the strbuf_release().
This changes the behavior so that we'll call update_abort_safety_file() in these cases where we'd previously "return", but as noted in [4] "update_abort_safety_file() is a no-op when rebasing and you're changing code that is only run when rebasing.". Here "no-op" refers to the early return in update_abort_safety_file() if git_path_seq_dir() doesn't exist.
1. 452202c74b8 (sequencer: stop releasing the strbuf in write_message(), 2016-10-21) 2. f241ff0d0a9 (prepare the builtins for a libified merge_recursive(), 2016-07-26) 3. 6e98de72c03 (sequencer (rebase -i): add support for the 'fixup' and 'squash' commands, 2017-01-02) 4. https://lore.kernel.org/git/bcace50b-a4c3-c468-94a3-4fe0c62b3671@dunelm.org.uk/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- sequencer.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c index ea568d99498..2c5ed5bc5ea 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2277,8 +2277,10 @@ static int do_pick_commit(struct repository *r, reword = 1; else if (is_fixup(command)) { if (update_squash_messages(r, command, commit, - opts, item->flags)) - return -1; + opts, item->flags)) { + res = -1; + goto leave; + } flags |= AMEND_MSG; if (!final_fixup) msg_file = rebase_path_squash_msg(); @@ -2288,9 +2290,11 @@ static int do_pick_commit(struct repository *r, } else { const char *dest = git_path_squash_msg(r); unlink(dest); - if (copy_file(dest, rebase_path_squash_msg(), 0666)) - return error(_("could not rename '%s' to '%s'"), - rebase_path_squash_msg(), dest); + if (copy_file(dest, rebase_path_squash_msg(), 0666)) { + res = error(_("could not rename '%s' to '%s'"), + rebase_path_squash_msg(), dest); + goto leave; + } unlink(git_path_merge_msg(r)); msg_file = dest; flags |= EDIT_MSG; @@ -2328,7 +2332,6 @@ static int do_pick_commit(struct repository *r, free_commit_list(common); free_commit_list(remotes); } - strbuf_release(&msgbuf); /* * If the merge was clean or if it failed due to conflict, we write @@ -2402,6 +2405,7 @@ static int do_pick_commit(struct repository *r, leave: free_message(commit, &msg); free(author); + strbuf_release(&msgbuf); update_abort_safety_file(); return res;
-- 2.39.0.1225.g30a3d88132d