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

> 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()