[PATCH v2 0/9] sequencer API & users: fix widespread leaks

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@gmail.com/

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@gmail.com>
     
 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@gmail.com>
     
    @@ 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@dunelm.org.uk/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ 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()
-- 
2.39.0.1205.g2ca064edc27