| Message ID | patch-08.10-d607dbac38e-20221230T071741Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sequencer API & users: fix widespread leaks | expand |
Hi Ævar On 30/12/2022 07:28, Ævar Arnfjörð Bjarmason wrote: > 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 > we added another place where we'd "return" before reaching the > strbuf_release(). This message makes it sound much worse that it is. The leak only happens if there is an error and in that case rebase bails out straight away. > Let's move it to a "cleanup" label, and use an appropriate "goto". It > may or may not be safe to combine the existing "leave" and "cleanup" > labels, but this change doesn't attempt to answer that question. Let's > instead avoid calling update_abort_safety_file() in these cases, as we > didn't do so before. It is safe as update_abort_safety_file() is a no-op when rebasing and you're changing code that is only run when rebasing. I feel this patch is adding complexity for no real gain, at least if we can avoid adding a second label it would be simpler. Best Wishes Phillip > 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) > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > sequencer.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 47367e66842..db8d789fa76 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2280,8 +2280,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 cleanup; > + } > flags |= AMEND_MSG; > if (!final_fixup) > msg_file = rebase_path_squash_msg(); > @@ -2291,9 +2293,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 cleanup; > + } > unlink(git_path_merge_msg(r)); > msg_file = dest; > flags |= EDIT_MSG; > @@ -2331,7 +2335,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 > @@ -2403,9 +2406,11 @@ static int do_pick_commit(struct repository *r, > } > > leave: > + update_abort_safety_file(); > +cleanup: > free_message(commit, &msg); > free(author); > - update_abort_safety_file(); > + strbuf_release(&msgbuf); > > return res; > }
diff --git a/sequencer.c b/sequencer.c index 47367e66842..db8d789fa76 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2280,8 +2280,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 cleanup; + } flags |= AMEND_MSG; if (!final_fixup) msg_file = rebase_path_squash_msg(); @@ -2291,9 +2293,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 cleanup; + } unlink(git_path_merge_msg(r)); msg_file = dest; flags |= EDIT_MSG; @@ -2331,7 +2335,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 @@ -2403,9 +2406,11 @@ static int do_pick_commit(struct repository *r, } leave: + update_abort_safety_file(); +cleanup: free_message(commit, &msg); free(author); - update_abort_safety_file(); + strbuf_release(&msgbuf); return res; }
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 we added another place where we'd "return" before reaching the strbuf_release(). Let's move it to a "cleanup" label, and use an appropriate "goto". It may or may not be safe to combine the existing "leave" and "cleanup" labels, but this change doesn't attempt to answer that question. Let's instead avoid calling update_abort_safety_file() in these cases, as we didn't do so before. 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) Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- sequencer.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)