@matheusbsbs / matheus-bagliano / commits / a5792e9d09

sequencer.c: always free() the "msgbuf" in do_pick_commit()

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Feb 6, 2023 at 20:08 UTC a5792e9d0929f30ece6b8cb679cc58fc700f7d09

1 file changed +10 -6

sequencer.c

+10 -6

@@ -2277,8 +2277,10 @@ static int do_pick_commit(struct repository *r,

2277 reword = 1;

2278 else if (is_fixup(command)) {

2279 if (update_squash_messages(r, command, commit,

2280 - opts, item->flags))

2281 - return -1;

2280 + opts, item->flags)) {

2281 + res = -1;

2282 + goto leave;

2283 + }

2284 flags |= AMEND_MSG;

2285 if (!final_fixup)

2286 msg_file = rebase_path_squash_msg();

@@ -2288,9 +2290,11 @@ static int do_pick_commit(struct repository *r,

2290 } else {

2291 const char *dest = git_path_squash_msg(r);

2292 unlink(dest);

2291 - if (copy_file(dest, rebase_path_squash_msg(), 0666))

2292 - return error(_("could not rename '%s' to '%s'"),

2293 - rebase_path_squash_msg(), dest);

2293 + if (copy_file(dest, rebase_path_squash_msg(), 0666)) {

2294 + res = error(_("could not rename '%s' to '%s'"),

2295 + rebase_path_squash_msg(), dest);

2296 + goto leave;

2297 + }

2298 unlink(git_path_merge_msg(r));

2299 msg_file = dest;

2300 flags |= EDIT_MSG;

@@ -2328,7 +2332,6 @@ static int do_pick_commit(struct repository *r,

2332 free_commit_list(common);

2333 free_commit_list(remotes);

2334 }

2331 - strbuf_release(&msgbuf);

2335

2336 /*

2337 * If the merge was clean or if it failed due to conflict, we write

@@ -2402,6 +2405,7 @@ fast_forward_edit:

2405 leave:

2406 free_message(commit, &msg);

2407 free(author);

2408 + strbuf_release(&msgbuf);

2409 update_abort_safety_file();

2410

2411 return res;