This series fixes various widespread leaks in the sequencer and its
users (rebase, revert, cherry-pick). As a result 18 tests become
leak-free in their entirety.
See the v1 for a longer general summary:
https://lore.kernel.org/git/cover-00.10-00000000000-20221230T071741Z-avarab@xxxxxxxxx/
Changes since v1:
* I think this addresses all the outstanding feedback, thanks all.
* Most significantly, the replay_opts_release() is now moved out of
sequencer_remove_state() as suggested.
* There's a prep change for renaming "squash_onto_name", per the
discussion in v1.
* Just do "goto leave" rather than being paranoid and introdungi
"goto cleanup", thanks Phillip!
* Various other small changes, see the range-diff.
A branch & passing CI for this are at:
https://github.com/avar/git/tree/avar/leak-fixes-sequencer-rebase-2
Ævar Arnfjörð Bjarmason (9):
rebase: use "cleanup" pattern in do_interactive_rebase()
sequencer.c: split up sequencer_remove_state()
rebase & sequencer API: fix get_replay_opts() leak in "rebase"
builtin/revert.c: move free-ing of "revs" to replay_opts_release()
builtin/rebase.c: rename "squash_onto_name" to "to_free"
builtin/rebase.c: fix "options.onto_name" leak
sequencer.c: always free() the "msgbuf" in do_pick_commit()
builtin/rebase.c: free() "options.strategy_opts"
commit.c: free() revs.commit in get_fork_point()
builtin/rebase.c | 27 +++++++++-------
builtin/revert.c | 8 ++---
commit.c | 1 +
sequencer.c | 43 ++++++++++++++++----------
sequencer.h | 1 +
t/t3405-rebase-malformed.sh | 1 +
t/t3412-rebase-root.sh | 1 +
t/t3416-rebase-onto-threedots.sh | 1 +
t/t3419-rebase-patch-id.sh | 1 +
t/t3423-rebase-reword.sh | 1 +
t/t3425-rebase-topology-merges.sh | 2 ++
t/t3431-rebase-fork-point.sh | 1 +
t/t3432-rebase-fast-forward.sh | 1 +
t/t3437-rebase-fixup-options.sh | 1 +
t/t3438-rebase-broken-files.sh | 2 ++
t/t3501-revert-cherry-pick.sh | 1 +
t/t3502-cherry-pick-merge.sh | 1 +
t/t3503-cherry-pick-root.sh | 1 +
t/t3506-cherry-pick-ff.sh | 1 +
t/t3511-cherry-pick-x.sh | 1 +
t/t7402-submodule-rebase.sh | 1 +
t/t9106-git-svn-commit-diff-clobber.sh | 1 -
t/t9164-git-svn-dcommit-concurrent.sh | 1 -
23 files changed, 64 insertions(+), 36 deletions(-)
Range-diff against v1:
1: f3a4ed79c7d ! 1: d0a0524f3d4 rebase: use "cleanup" pattern in do_interactive_rebase()
@@ Commit message
rebase: use "cleanup" pattern in do_interactive_rebase()
Use a "goto cleanup" pattern in do_interactive_rebase(). This
- eliminates some duplicated free() code added in 0609b741a43 (rebase
- -i: combine rebase--interactive.c with rebase.c, 2019-04-17), and sets
- us up for a subsequent commit which'll make further use of the
- "cleanup" label.
+ eliminates some duplicated free() code added in 53bbcfbde7c (rebase
+ -i: implement the main part of interactive rebase as a builtin,
+ 2018-09-27), and sets us up for a subsequent commit which'll make
+ further use of the "cleanup" label.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
2: 4994940a0a9 ! 2: c4eaa8dfef4 sequencer.c: split up sequencer_remove_state()
@@ Commit message
function, which will be adjusted and called independent of the other
code in sequencer_remove_state() in a subsequent commit.
- The only functional changes here are:
-
- * Changing the "int" to a "size_t", which is the correct type, as
- "xopts_nr" is a "size_t".
-
- * Calling the free() before the "if (is_rebase_i(opts) && ...)",
- which is OK, and makes a subsequent change smaller.
+ The only functional change here is changing the "int" to a "size_t",
+ which is the correct type, as "xopts_nr" is a "size_t".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
@@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
struct strbuf buf = STRBUF_INIT;
- int i, ret = 0;
+ int ret = 0;
-+
-+ replay_opts_release(opts);
if (is_rebase_i(opts) &&
strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
@@ sequencer.c: int sequencer_remove_state(struct replay_opts *opts)
- free(opts->xopts[i]);
- free(opts->xopts);
- strbuf_release(&opts->current_fixups);
--
++ replay_opts_release(opts);
+
strbuf_reset(&buf);
strbuf_addstr(&buf, get_dir(opts));
- if (remove_dir_recursively(&buf, 0))
3: 3e9c4df61fe ! 3: f06f565ceaf rebase & sequencer API: fix get_replay_opts() leak in "rebase"
@@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts)
break;
}
case ACTION_EDIT_TODO:
+@@ builtin/rebase.c: static int finish_rebase(struct rebase_options *opts)
+
+ replay.action = REPLAY_INTERACTIVE_REBASE;
+ ret = sequencer_remove_state(&replay);
++ replay_opts_release(&replay);
+ } else {
+ strbuf_addstr(&dir, opts->state_dir);
+ if (remove_dir_recursively(&dir, 0))
+@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
+
+ replay.action = REPLAY_INTERACTIVE_REBASE;
+ ret = sequencer_remove_state(&replay);
++ replay_opts_release(&replay);
+ } else {
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, options.state_dir);
+
+ ## builtin/revert.c ##
+@@ builtin/revert.c: int cmd_revert(int argc, const char **argv, const char *prefix)
+ if (opts.revs)
+ release_revisions(opts.revs);
+ free(opts.revs);
++ replay_opts_release(&opts);
+ return res;
+ }
+
+@@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
+ free(opts.revs);
+ if (res < 0)
+ die(_("cherry-pick failed"));
++ replay_opts_release(&opts);
+ return res;
+ }
## sequencer.c ##
@@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
@@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
-static void replay_opts_release(struct replay_opts *opts)
+void replay_opts_release(struct replay_opts *opts)
{
-- free(opts->gpg_sign);
-- free(opts->reflog_action);
-- free(opts->default_strategy);
-- free(opts->strategy);
-+ FREE_AND_NULL(opts->gpg_sign);
-+ FREE_AND_NULL(opts->reflog_action);
-+ FREE_AND_NULL(opts->default_strategy);
-+ FREE_AND_NULL(opts->strategy);
+ free(opts->gpg_sign);
+ free(opts->reflog_action);
+@@ sequencer.c: static void replay_opts_release(struct replay_opts *opts)
+ free(opts->strategy);
for (size_t i = 0; i < opts->xopts_nr; i++)
free(opts->xopts[i]);
-- free(opts->xopts);
+ opts->xopts_nr = 0;
-+ FREE_AND_NULL(opts->xopts);
+ free(opts->xopts);
strbuf_release(&opts->current_fixups);
}
+@@ sequencer.c: int sequencer_remove_state(struct replay_opts *opts)
+ }
+ }
+- replay_opts_release(opts);
+-
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, get_dir(opts));
+ if (remove_dir_recursively(&buf, 0))
## sequencer.h ##
@@ sequencer.h: int sequencer_pick_revisions(struct repository *repo,
@@ t/t3412-rebase-root.sh: Tests if git rebase --root --onto <newparent> can rebase
log_with_names () {
+ ## t/t3419-rebase-patch-id.sh ##
+@@ t/t3419-rebase-patch-id.sh: test_description='git rebase - test patch id computation'
+ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ scramble () {
+
## t/t3423-rebase-reword.sh ##
@@
@@ t/t3423-rebase-reword.sh
. "$TEST_DIRECTORY"/lib-rebase.sh
+ ## t/t3425-rebase-topology-merges.sh ##
+@@
+ #!/bin/sh
+
+ test_description='rebase topology tests with merges'
++
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+ . "$TEST_DIRECTORY"/lib-rebase.sh
+
+
## t/t3437-rebase-fixup-options.sh ##
@@ t/t3437-rebase-fixup-options.sh: to the "fixup" command that works with "fixup!", "fixup -C" works with
"amend!" upon --autosquash.
@@ t/t3438-rebase-broken-files.sh
test_expect_success 'set up conflicting branches' '
+ ## t/t3501-revert-cherry-pick.sh ##
+@@ t/t3501-revert-cherry-pick.sh: test_description='test cherry-pick and revert with renames
+ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ test_expect_success setup '
+
+ ## t/t3502-cherry-pick-merge.sh ##
+@@ t/t3502-cherry-pick-merge.sh: test_description='cherry picking and reverting a merge
+ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ test_expect_success setup '
+
+ ## t/t3503-cherry-pick-root.sh ##
+@@ t/t3503-cherry-pick-root.sh: test_description='test cherry-picking (and reverting) a root commit'
+ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ test_expect_success setup '
+
+ ## t/t3506-cherry-pick-ff.sh ##
+@@ t/t3506-cherry-pick-ff.sh: test_description='test cherry-picking with --ff option'
+ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ test_expect_success setup '
+
+ ## t/t3511-cherry-pick-x.sh ##
+@@
+
+ test_description='Test cherry-pick -x and -s'
+
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ pristine_detach () {
+
## t/t7402-submodule-rebase.sh ##
@@
4: 1e4e504c533 < -: ----------- builtin/revert.c: refactor run_sequencer() return pattern
5: e2895bb9795 < -: ----------- builtin/revert.c: fix common leak by using replay_opts_release()
6: 21eea8eb802 ! 4: e83bdfab046 builtin/revert.c: move free-ing of "revs" to replay_opts_release()
@@ builtin/revert.c: int cmd_revert(int argc, const char **argv, const char *prefix
- if (opts.revs)
- release_revisions(opts.revs);
- free(opts.revs);
-+ replay_opts_release(&opts);
+ replay_opts_release(&opts);
return res;
}
-
@@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
opts.action = REPLAY_PICK;
sequencer_init_config(&opts);
@@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *p
- if (opts.revs)
- release_revisions(opts.revs);
- free(opts.revs);
-+ replay_opts_release(&opts);
if (res < 0)
die(_("cherry-pick failed"));
- return res;
+ replay_opts_release(&opts);
## sequencer.c ##
@@ sequencer.c: void replay_opts_release(struct replay_opts *opts)
opts->xopts_nr = 0;
- FREE_AND_NULL(opts->xopts);
+ free(opts->xopts);
strbuf_release(&opts->current_fixups);
+ if (opts->revs)
+ release_revisions(opts->revs);
-+ FREE_AND_NULL(opts->revs);
++ free(opts->revs);
}
int sequencer_remove_state(struct replay_opts *opts)
-: ----------- > 5: 4fea2b77c6d builtin/rebase.c: rename "squash_onto_name" to "to_free"
7: 484ebbfd6d1 ! 6: 898bb7698fc builtin/rebase.c: fix "options.onto_name" leak
@@ Commit message
In [1] we started saving away the earlier xstrdup()'d
"options.onto_name" assignment to free() it, but when [2] added this
- "keep_base" branch it didn't free() the already assigned
- "squash_onto_name" before re-assigning to "options.onto_name". Let's
- do that, and fix the memory leak.
+ "keep_base" branch it didn't free() the already assigned value before
+ re-assigning to "options.onto_name". Let's do that, and fix the memory
+ leak.
1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
strbuf_addstr(&buf, "...");
strbuf_addstr(&buf, branch_name);
- options.onto_name = xstrdup(buf.buf);
-+ free(squash_onto_name);
-+ options.onto_name = squash_onto_name = xstrdup(buf.buf);
++ free(to_free);
++ options.onto_name = to_free = xstrdup(buf.buf);
} else if (!options.onto_name)
options.onto_name = options.upstream_name;
if (strstr(options.onto_name, "...")) {
8: d607dbac38e ! 7: fb38dc873f9 sequencer.c: always free() the "msgbuf" in do_pick_commit()
@@ Commit message
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().
+ in this error case 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.
+ 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)
@@ Commit message
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@xxxxxxxxxxxxx/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
@@ sequencer.c: static int do_pick_commit(struct repository *r,
- return -1;
+ opts, item->flags)) {
+ res = -1;
-+ goto cleanup;
++ goto leave;
+ }
flags |= AMEND_MSG;
if (!final_fixup)
@@ sequencer.c: static int do_pick_commit(struct repository *r,
+ if (copy_file(dest, rebase_path_squash_msg(), 0666)) {
+ res = error(_("could not rename '%s' to '%s'"),
+ rebase_path_squash_msg(), dest);
-+ goto cleanup;
++ goto leave;
+ }
unlink(git_path_merge_msg(r));
msg_file = dest;
@@ sequencer.c: static int do_pick_commit(struct repository *r,
/*
* If the merge was clean or if it failed due to conflict, we write
@@ sequencer.c: 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);
+ update_abort_safety_file();
return res;
- }
9: cd0489a2384 ! 8: d4b0e2a5c83 builtin/rebase.c: free() "options.strategy_opts"
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
free(options.strategy);
+ free(options.strategy_opts);
strbuf_release(&options.git_format_patch_opt);
- free(squash_onto_name);
+ free(to_free);
string_list_clear(&exec, 0);
10: eb3678b4667 = 9: fd9c7a5547c commit.c: free() revs.commit in get_fork_point()
