[PATCH 00/10] sequencer API & users: fix widespread leaks

[PATCH 00/10] sequencer API & users: fix widespread leaks

[Ævar Arnfjörð Bjarmason]

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.

The main change is the 3/10 here, where we introduce a
replay_opts_release() to free the "struct replay_opts". The rest is
then either refactorings to be able to call that destructor
(e.g. "return" to "goto cleanup"), or other miscellanious adjacent
leaks.

This is a follow-up to the discussion ending at [1], as noted there
the recent ff84d031a9d (Merge branch 'pw/rebase-no-reflog-action',
2022-11-23) ended up introducing a leak because of the disfunctional
lack of a destructor (or rather, the current logic being tied up in
sequencer_remove_state().

This can be queued and graduated independently of the other concurrent
leak series I've submitted[2]. When the two are combined we'll end up
passing more tests, i.e. both topics combined get us over the finish
line for some of them, but neither one is enough in isolation.

But that's OK, we just won't opt them into the "linux-leaks"
testing. I'll submit a follow-up similar to [3] at some later date to
mark them as passing. I think that's a better trade-off than making
these two depend on one another.

1. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/cover-v2-00.20-00000000000-20221230T020341Z-avarab@gmail.com/
3. https://lore.kernel.org/git/patch-v2-01.20-3de29c6d75f-20221230T020341Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (10):
  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: refactor run_sequencer() return pattern
  builtin/revert.c: fix common leak by using replay_opts_release()
  builtin/revert.c: move free-ing of "revs" to replay_opts_release()
  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                       | 19 +++++-----
 builtin/revert.c                       | 40 +++++++++++----------
 commit.c                               |  1 +
 sequencer.c                            | 48 +++++++++++++++++---------
 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, 82 insertions(+), 47 deletions(-)

-- 
2.39.0.1153.g589e4efe9dc

[PATCH 01/10] rebase: use "cleanup" pattern in do_interactive_rebase()

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5b..7141fd5e0c1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -256,7 +256,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)
 
 static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 {
-	int ret;
+	int ret = -1;
 	char *revisions = NULL, *shortrevisions = NULL;
 	struct strvec make_script_args = STRVEC_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
@@ -265,16 +265,12 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 
 	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
 				&revisions, &shortrevisions))
-		return -1;
+		goto cleanup;
 
 	if (init_basic_state(&replay,
 			     opts->head_name ? opts->head_name : "detached HEAD",
-			     opts->onto, &opts->orig_head->object.oid)) {
-		free(revisions);
-		free(shortrevisions);
-
-		return -1;
-	}
+			     opts->onto, &opts->orig_head->object.oid))
+		goto cleanup;
 
 	if (!opts->upstream && opts->squash_onto)
 		write_file(path_squash_onto(), "%s\n",
@@ -304,6 +300,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 			opts->autosquash, opts->update_refs, &todo_list);
 	}
 
+cleanup:
 	string_list_clear(&commands, 0);
 	free(revisions);
 	free(shortrevisions);
-- 
2.39.0.1153.g589e4efe9dc

Re: [PATCH 01/10] rebase: use "cleanup" pattern in do_interactive_rebase()

[Phillip Wood]

Hi Ævar

On 30/12/2022 07:28, Ævar Arnfjörð Bjarmason wrote:
> 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),

I read this as meaning that commit added some code to this function, but 
it fact it just copied the function unchanged from another file.

> 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>
> ---
>   builtin/rebase.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 1481c5b6a5b..7141fd5e0c1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -256,7 +256,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)
>   
>   static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   {
> -	int ret;
> +	int ret = -1;
>   	char *revisions = NULL, *shortrevisions = NULL;
>   	struct strvec make_script_args = STRVEC_INIT;
>   	struct todo_list todo_list = TODO_LIST_INIT;
> @@ -265,16 +265,12 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   
>   	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
>   				&revisions, &shortrevisions))
> -		return -1;
> +		goto cleanup;

This sort of change potentially problematic as we're free()ing things 
that were not previously free()d but revisions and shortrevisions are 
initialized to NULL before passing them to get_revision_ranges() so it 
is safe.

Looks good

Phillip

>   	if (init_basic_state(&replay,
>   			     opts->head_name ? opts->head_name : "detached HEAD",
> -			     opts->onto, &opts->orig_head->object.oid)) {
> -		free(revisions);
> -		free(shortrevisions);
> -
> -		return -1;
> -	}
> +			     opts->onto, &opts->orig_head->object.oid))
> +		goto cleanup;
>   
>   	if (!opts->upstream && opts->squash_onto)
>   		write_file(path_squash_onto(), "%s\n",
> @@ -304,6 +300,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   			opts->autosquash, opts->update_refs, &todo_list);
>   	}
>   
> +cleanup:
>   	string_list_clear(&commands, 0);
>   	free(revisions);
>   	free(shortrevisions);

[PATCH 02/10] sequencer.c: split up sequencer_remove_state()

[Ævar Arnfjörð Bjarmason]

Split off the free()-ing in sequencer_remove_state() into a utility
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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bcb662e23be..655ae9f1a72 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,10 +351,24 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
 	return buf.buf;
 }
 
+static void replay_opts_release(struct replay_opts *opts)
+{
+	free(opts->gpg_sign);
+	free(opts->reflog_action);
+	free(opts->default_strategy);
+	free(opts->strategy);
+	for (size_t i = 0; i < opts->xopts_nr; i++)
+		free(opts->xopts[i]);
+	free(opts->xopts);
+	strbuf_release(&opts->current_fixups);
+}
+
 int sequencer_remove_state(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) {
@@ -373,15 +387,6 @@ int sequencer_remove_state(struct replay_opts *opts)
 		}
 	}
 
-	free(opts->gpg_sign);
-	free(opts->reflog_action);
-	free(opts->default_strategy);
-	free(opts->strategy);
-	for (i = 0; i < opts->xopts_nr; i++)
-		free(opts->xopts[i]);
-	free(opts->xopts);
-	strbuf_release(&opts->current_fixups);
-
 	strbuf_reset(&buf);
 	strbuf_addstr(&buf, get_dir(opts));
 	if (remove_dir_recursively(&buf, 0))
-- 
2.39.0.1153.g589e4efe9dc

Re: [PATCH 02/10] sequencer.c: split up sequencer_remove_state()

[René Scharfe]

Am 30.12.22 um 08:28 schrieb Ævar Arnfjörð Bjarmason:
> Split off the free()-ing in sequencer_remove_state() into a utility
> 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".

Good, and you declare it in the for statement, which we can do now!

>
>  * Calling the free() before the "if (is_rebase_i(opts) && ...)",
>    which is OK, and makes a subsequent change smaller.

It's true that is_rebase_i() can be called after all that free()ing;
here is its definition:

	static inline int is_rebase_i(const struct replay_opts *opts)
	{
		return opts->action == REPLAY_INTERACTIVE_REBASE;
	}

But why?  Making a subsequent change smaller is just a trivial fact if
you do a part if it earlier, but that in itself is not a valid reason
for the reordering.

And I can't find that patch -- sequencer_remove_state() is not touched
again in this series.

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  sequencer.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index bcb662e23be..655ae9f1a72 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -351,10 +351,24 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>  	return buf.buf;
>  }
>
> +static void replay_opts_release(struct replay_opts *opts)
> +{
> +	free(opts->gpg_sign);
> +	free(opts->reflog_action);
> +	free(opts->default_strategy);
> +	free(opts->strategy);
> +	for (size_t i = 0; i < opts->xopts_nr; i++)
> +		free(opts->xopts[i]);
> +	free(opts->xopts);
> +	strbuf_release(&opts->current_fixups);
> +}
> +
>  int sequencer_remove_state(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) {
> @@ -373,15 +387,6 @@ int sequencer_remove_state(struct replay_opts *opts)
>  		}
>  	}
>
> -	free(opts->gpg_sign);
> -	free(opts->reflog_action);
> -	free(opts->default_strategy);
> -	free(opts->strategy);
> -	for (i = 0; i < opts->xopts_nr; i++)
> -		free(opts->xopts[i]);
> -	free(opts->xopts);
> -	strbuf_release(&opts->current_fixups);
> -
>  	strbuf_reset(&buf);
>  	strbuf_addstr(&buf, get_dir(opts));
>  	if (remove_dir_recursively(&buf, 0))

Re: [PATCH 02/10] sequencer.c: split up sequencer_remove_state()

[Phillip Wood]

On 30/12/2022 17:35, René Scharfe wrote:
> Am 30.12.22 um 08:28 schrieb Ævar Arnfjörð Bjarmason:
>> Split off the free()-ing in sequencer_remove_state() into a utility
>> 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".
> 
> Good, and you declare it in the for statement, which we can do now!
> 
>>
>>   * Calling the free() before the "if (is_rebase_i(opts) && ...)",
>>     which is OK, and makes a subsequent change smaller.
> 
> It's true that is_rebase_i() can be called after all that free()ing;
> here is its definition:
> 
> 	static inline int is_rebase_i(const struct replay_opts *opts)
> 	{
> 		return opts->action == REPLAY_INTERACTIVE_REBASE;
> 	}
> 
> But why?  Making a subsequent change smaller is just a trivial fact if
> you do a part if it earlier, but that in itself is not a valid reason
> for the reordering.

Yes I'd prefer we did not reorder here either

Best Wishes

Phillip

> And I can't find that patch -- sequencer_remove_state() is not touched
> again in this series.
> 
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   sequencer.c | 25 +++++++++++++++----------
>>   1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index bcb662e23be..655ae9f1a72 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -351,10 +351,24 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>>   	return buf.buf;
>>   }
>>
>> +static void replay_opts_release(struct replay_opts *opts)
>> +{
>> +	free(opts->gpg_sign);
>> +	free(opts->reflog_action);
>> +	free(opts->default_strategy);
>> +	free(opts->strategy);
>> +	for (size_t i = 0; i < opts->xopts_nr; i++)
>> +		free(opts->xopts[i]);
>> +	free(opts->xopts);
>> +	strbuf_release(&opts->current_fixups);
>> +}
>> +
>>   int sequencer_remove_state(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) {
>> @@ -373,15 +387,6 @@ int sequencer_remove_state(struct replay_opts *opts)
>>   		}
>>   	}
>>
>> -	free(opts->gpg_sign);
>> -	free(opts->reflog_action);
>> -	free(opts->default_strategy);
>> -	free(opts->strategy);
>> -	for (i = 0; i < opts->xopts_nr; i++)
>> -		free(opts->xopts[i]);
>> -	free(opts->xopts);
>> -	strbuf_release(&opts->current_fixups);
>> -
>>   	strbuf_reset(&buf);
>>   	strbuf_addstr(&buf, get_dir(opts));
>>   	if (remove_dir_recursively(&buf, 0))

[PATCH 03/10] rebase & sequencer API: fix get_replay_opts() leak in "rebase"

[Ævar Arnfjörð Bjarmason]

Make the recently added replay_opts_release() function non-static and
use it for freeing the "struct replay_opts" constructed by the
get_replay_opts() function in "builtin/rebase.c". See [1] for the
initial addition of get_replay_opts().

To safely call our new replay_opts_release() we'll need to change all
the free() to a FREE_AND_NULL(), and set "xopts_nr" to "0" after we
loop over it and free() it (the free() in the loop doesn't need to be
a FREE_AND_NULL()).

This is because in e.g. do_interactive_rebase() we construct a "struct
replay_opts" with "get_replay_opts()", and then call
"complete_action()". If we get far enough in that function without
encountering errors we'll call "pick_commits()" which (indirectly)
calls sequencer_remove_state() at the end.

But if we encounter errors anywhere along the way we'd punt out early,
and not free() the memory we allocated. Remembering whether we
previously called sequencer_remove_state() would be a hassle, so let's
make it safe to re-invoke replay_opts_release() instead.

I experimented with a change to be more paranoid instead, i.e. to
exhaustively check our state via an enum. We could make sure that we:

- Only allow calling "replay_opts_release()" after
  "sequencer_remove_state()", but not the other way around.

- Forbid invoking either function twice in a row.

But such paranoia isn't warranted here, let's instead take the easy
way out and FREE_AND_NULL() this.

See [2] for the initial implementation of "sequencer_remove_state()",
which assumed that it should be removing the full (including on-disk)
rebase state as a one-off.

1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
   2019-04-17)
2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
   2011-08-04)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Re the "I experimented with...", the earlier version can be seen at
https://github.com/avar/git/commit/7a150d1b7e2; It's the commit I
linked to from
https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/
(which is mentioned in the CL).

 builtin/rebase.c                       |  2 ++
 sequencer.c                            | 13 +++++++------
 sequencer.h                            |  1 +
 t/t3405-rebase-malformed.sh            |  1 +
 t/t3412-rebase-root.sh                 |  1 +
 t/t3423-rebase-reword.sh               |  1 +
 t/t3437-rebase-fixup-options.sh        |  1 +
 t/t3438-rebase-broken-files.sh         |  2 ++
 t/t7402-submodule-rebase.sh            |  1 +
 t/t9106-git-svn-commit-diff-clobber.sh |  1 -
 t/t9164-git-svn-dcommit-concurrent.sh  |  1 -
 11 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 7141fd5e0c1..91bf55be6e6 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -301,6 +301,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	}
 
 cleanup:
+	replay_opts_release(&replay);
 	string_list_clear(&commands, 0);
 	free(revisions);
 	free(shortrevisions);
@@ -343,6 +344,7 @@ static int run_sequencer_rebase(struct rebase_options *opts)
 		struct replay_opts replay_opts = get_replay_opts(opts);
 
 		ret = sequencer_continue(the_repository, &replay_opts);
+		replay_opts_release(&replay_opts);
 		break;
 	}
 	case ACTION_EDIT_TODO:
diff --git a/sequencer.c b/sequencer.c
index 655ae9f1a72..e29a97b6caa 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,15 +351,16 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
 	return buf.buf;
 }
 
-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);
 	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);
 	strbuf_release(&opts->current_fixups);
 }
 
diff --git a/sequencer.h b/sequencer.h
index 888c18aad71..3bcdfa1b586 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -158,6 +158,7 @@ int sequencer_pick_revisions(struct repository *repo,
 int sequencer_continue(struct repository *repo, struct replay_opts *opts);
 int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_skip(struct repository *repo, struct replay_opts *opts);
+void replay_opts_release(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
 #define TODO_LIST_KEEP_EMPTY (1U << 0)
diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
index 25243318618..8979bc34073 100755
--- a/t/t3405-rebase-malformed.sh
+++ b/t/t3405-rebase-malformed.sh
@@ -5,6 +5,7 @@ test_description='rebase should handle arbitrary git message'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 58371d8a547..e75b3d0e07c 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -7,6 +7,7 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 log_with_names () {
diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
index 4859bb8f722..2fab703d615 100755
--- a/t/t3423-rebase-reword.sh
+++ b/t/t3423-rebase-reword.sh
@@ -2,6 +2,7 @@
 
 test_description='git rebase interactive with rewording'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index c023fefd681..274699dadb8 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -14,6 +14,7 @@ to the "fixup" command that works with "fixup!", "fixup -C" works with
 "amend!" upon --autosquash.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh
index b92a3ce46b8..c614c4f2e4b 100755
--- a/t/t3438-rebase-broken-files.sh
+++ b/t/t3438-rebase-broken-files.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='rebase behavior when on-disk files are broken'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'set up conflicting branches' '
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index ebeca12a711..b19792b3269 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -5,6 +5,7 @@
 
 test_description='Test rebasing, stashing, etc. with submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
index 3cab0b9720a..bca496c40e0 100755
--- a/t/t9106-git-svn-commit-diff-clobber.sh
+++ b/t/t9106-git-svn-commit-diff-clobber.sh
@@ -3,7 +3,6 @@
 # Copyright (c) 2006 Eric Wong
 test_description='git svn commit-diff clobber'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'initialize repo' '
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index 1465156072e..c8e6c0733f4 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -5,7 +5,6 @@
 
 test_description='concurrent git svn dcommit'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 
-- 
2.39.0.1153.g589e4efe9dc

Re: [PATCH 03/10] rebase & sequencer API: fix get_replay_opts() leak in "rebase"

[Phillip Wood]

Hi Ævar

On 30/12/2022 07:28, Ævar Arnfjörð Bjarmason wrote:
> Make the recently added replay_opts_release() function non-static and
> use it for freeing the "struct replay_opts" constructed by the
> get_replay_opts() function in "builtin/rebase.c". See [1] for the
> initial addition of get_replay_opts().
> 
> To safely call our new replay_opts_release() we'll need to change all
> the free() to a FREE_AND_NULL(), and set "xopts_nr" to "0" after we
> loop over it and free() it (the free() in the loop doesn't need to be
> a FREE_AND_NULL()).
> 
> This is because in e.g. do_interactive_rebase() we construct a "struct
> replay_opts" with "get_replay_opts()", and then call
> "complete_action()". If we get far enough in that function without
> encountering errors we'll call "pick_commits()" which (indirectly)
> calls sequencer_remove_state() at the end.

The way to fix this is to stop calling replay_opts_release() from 
sequencer_remove_state(), then you can be sure to call 
replay_opts_release() only once by adding calls to the functions that 
initialize a "struct replay_opts". That would be much cleaner than this 
FREE_AND_NULL() hackery.

Best Wishes

Phillip

> But if we encounter errors anywhere along the way we'd punt out early,
> and not free() the memory we allocated. Remembering whether we
> previously called sequencer_remove_state() would be a hassle, so let's
> make it safe to re-invoke replay_opts_release() instead.
> 
> I experimented with a change to be more paranoid instead, i.e. to
> exhaustively check our state via an enum. We could make sure that we:
> 
> - Only allow calling "replay_opts_release()" after
>    "sequencer_remove_state()", but not the other way around.
> 
> - Forbid invoking either function twice in a row.
> 
> But such paranoia isn't warranted here, let's instead take the easy
> way out and FREE_AND_NULL() this.
> 
> See [2] for the initial implementation of "sequencer_remove_state()",
> which assumed that it should be removing the full (including on-disk)
> rebase state as a one-off.
> 
> 1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
>     2019-04-17)
> 2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
>     2011-08-04)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> Re the "I experimented with...", the earlier version can be seen at
> https://github.com/avar/git/commit/7a150d1b7e2; It's the commit I
> linked to from
> https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/
> (which is mentioned in the CL).
> 
>   builtin/rebase.c                       |  2 ++
>   sequencer.c                            | 13 +++++++------
>   sequencer.h                            |  1 +
>   t/t3405-rebase-malformed.sh            |  1 +
>   t/t3412-rebase-root.sh                 |  1 +
>   t/t3423-rebase-reword.sh               |  1 +
>   t/t3437-rebase-fixup-options.sh        |  1 +
>   t/t3438-rebase-broken-files.sh         |  2 ++
>   t/t7402-submodule-rebase.sh            |  1 +
>   t/t9106-git-svn-commit-diff-clobber.sh |  1 -
>   t/t9164-git-svn-dcommit-concurrent.sh  |  1 -
>   11 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 7141fd5e0c1..91bf55be6e6 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -301,6 +301,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   	}
>   
>   cleanup:
> +	replay_opts_release(&replay);
>   	string_list_clear(&commands, 0);
>   	free(revisions);
>   	free(shortrevisions);
> @@ -343,6 +344,7 @@ static int run_sequencer_rebase(struct rebase_options *opts)
>   		struct replay_opts replay_opts = get_replay_opts(opts);
>   
>   		ret = sequencer_continue(the_repository, &replay_opts);
> +		replay_opts_release(&replay_opts);
>   		break;
>   	}
>   	case ACTION_EDIT_TODO:
> diff --git a/sequencer.c b/sequencer.c
> index 655ae9f1a72..e29a97b6caa 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -351,15 +351,16 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>   	return buf.buf;
>   }
>   
> -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);
>   	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);
>   	strbuf_release(&opts->current_fixups);
>   }
>   
> diff --git a/sequencer.h b/sequencer.h
> index 888c18aad71..3bcdfa1b586 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -158,6 +158,7 @@ int sequencer_pick_revisions(struct repository *repo,
>   int sequencer_continue(struct repository *repo, struct replay_opts *opts);
>   int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
>   int sequencer_skip(struct repository *repo, struct replay_opts *opts);
> +void replay_opts_release(struct replay_opts *opts);
>   int sequencer_remove_state(struct replay_opts *opts);
>   
>   #define TODO_LIST_KEEP_EMPTY (1U << 0)
> diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
> index 25243318618..8979bc34073 100755
> --- a/t/t3405-rebase-malformed.sh
> +++ b/t/t3405-rebase-malformed.sh
> @@ -5,6 +5,7 @@ test_description='rebase should handle arbitrary git message'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY"/lib-rebase.sh
>   
> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
> index 58371d8a547..e75b3d0e07c 100755
> --- a/t/t3412-rebase-root.sh
> +++ b/t/t3412-rebase-root.sh
> @@ -7,6 +7,7 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   log_with_names () {
> diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
> index 4859bb8f722..2fab703d615 100755
> --- a/t/t3423-rebase-reword.sh
> +++ b/t/t3423-rebase-reword.sh
> @@ -2,6 +2,7 @@
>   
>   test_description='git rebase interactive with rewording'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   . "$TEST_DIRECTORY"/lib-rebase.sh
> diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
> index c023fefd681..274699dadb8 100755
> --- a/t/t3437-rebase-fixup-options.sh
> +++ b/t/t3437-rebase-fixup-options.sh
> @@ -14,6 +14,7 @@ to the "fixup" command that works with "fixup!", "fixup -C" works with
>   "amend!" upon --autosquash.
>   '
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   . "$TEST_DIRECTORY"/lib-rebase.sh
> diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh
> index b92a3ce46b8..c614c4f2e4b 100755
> --- a/t/t3438-rebase-broken-files.sh
> +++ b/t/t3438-rebase-broken-files.sh
> @@ -1,6 +1,8 @@
>   #!/bin/sh
>   
>   test_description='rebase behavior when on-disk files are broken'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success 'set up conflicting branches' '
> diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
> index ebeca12a711..b19792b3269 100755
> --- a/t/t7402-submodule-rebase.sh
> +++ b/t/t7402-submodule-rebase.sh
> @@ -5,6 +5,7 @@
>   
>   test_description='Test rebasing, stashing, etc. with submodules'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success setup '
> diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
> index 3cab0b9720a..bca496c40e0 100755
> --- a/t/t9106-git-svn-commit-diff-clobber.sh
> +++ b/t/t9106-git-svn-commit-diff-clobber.sh
> @@ -3,7 +3,6 @@
>   # Copyright (c) 2006 Eric Wong
>   test_description='git svn commit-diff clobber'
>   
> -TEST_FAILS_SANITIZE_LEAK=true
>   . ./lib-git-svn.sh
>   
>   test_expect_success 'initialize repo' '
> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> index 1465156072e..c8e6c0733f4 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -5,7 +5,6 @@
>   
>   test_description='concurrent git svn dcommit'
>   
> -TEST_FAILS_SANITIZE_LEAK=true
>   . ./lib-git-svn.sh
>   
>   

[PATCH 04/10] builtin/revert.c: refactor run_sequencer() return pattern

[Ævar Arnfjörð Bjarmason]

Refactor the return pattern in run_sequencer() to make it easier to
insert a "replay_opts_release()" call between the "fn(...)" invocation
and the eventual return.

Usually we'd name the "cbfun" here "fn", but by using this name we'll
nicely align all the "cmd == ?" comparisons.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index f2d86d2a8f9..e956d125a2b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -93,6 +93,15 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 		die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
 }
 
+static int sequencer_remove_branch_state(struct repository *r,
+					 struct replay_opts *opts)
+{
+	int ret = sequencer_remove_state(opts);
+	if (!ret)
+		remove_branch_state(the_repository, 0);
+	return ret;
+}
+
 static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
@@ -120,6 +129,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END()
 	};
 	struct option *options = base_options;
+	int (*cbfun)(struct repository *repo, struct replay_opts *opts);
+	int ret;
 
 	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
@@ -223,19 +234,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 		opts->strategy = xstrdup(getenv("GIT_TEST_MERGE_ALGORITHM"));
 	free(options);
 
-	if (cmd == 'q') {
-		int ret = sequencer_remove_state(opts);
-		if (!ret)
-			remove_branch_state(the_repository, 0);
-		return ret;
-	}
-	if (cmd == 'c')
-		return sequencer_continue(the_repository, opts);
-	if (cmd == 'a')
-		return sequencer_rollback(the_repository, opts);
-	if (cmd == 's')
-		return sequencer_skip(the_repository, opts);
-	return sequencer_pick_revisions(the_repository, opts);
+	cbfun = cmd == 'q' ? sequencer_remove_branch_state :
+		cmd == 'c' ? sequencer_continue :
+		cmd == 'a' ? sequencer_rollback :
+		cmd == 's' ? sequencer_skip :
+		sequencer_pick_revisions;
+	ret = cbfun(the_repository, opts);
+	return ret;
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
-- 
2.39.0.1153.g589e4efe9dc

Re: [PATCH 04/10] builtin/revert.c: refactor run_sequencer() return pattern

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Refactor the return pattern in run_sequencer() to make it easier to
> insert a "replay_opts_release()" call between the "fn(...)" invocation
> and the eventual return.
>
> Usually we'd name the "cbfun" here "fn", but by using this name we'll
> nicely align all the "cmd == ?" comparisons.

If the new helper function sequencer_remove_branch_state() is used
elsewhere in later steps of the series, it is great that it is
extracted out of an existing code to handle the 'q(uit)' action.

However, I'd not appreciate this change from if/elseif/... cascade
to ? : ? : cascade, mixed into a creation of the new helper
function.  Such a change forces all conceivable future command
handlers to take only r and opts, and that is not a consistency we
do not know we need (yet---YAGNI).

Then you do not even have to talk about cbfun vs fn ;-).

And if sequencer_remove_branch_state() is not reused elsewhere in
the series, then simply dropping this step would be great.

Thanks.

[PATCH 05/10] builtin/revert.c: fix common leak by using replay_opts_release()

[Ævar Arnfjörð Bjarmason]

Use the replay_opts_release() function introduced in a preceding
commit to fix a memory leak in run_sequencer().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c                  | 1 +
 t/t3419-rebase-patch-id.sh        | 1 +
 t/t3425-rebase-topology-merges.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 +
 8 files changed, 9 insertions(+)

diff --git a/builtin/revert.c b/builtin/revert.c
index e956d125a2b..2f656b25619 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -240,6 +240,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 		cmd == 's' ? sequencer_skip :
 		sequencer_pick_revisions;
 	ret = cbfun(the_repository, opts);
+	replay_opts_release(opts);
 	return ret;
 }
 
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index 7181f176b81..6c61f240cf9 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -5,6 +5,7 @@ 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 () {
diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
index 63acc1ea4da..a16428bdf54 100755
--- a/t/t3425-rebase-topology-merges.sh
+++ b/t/t3425-rebase-topology-merges.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='rebase topology tests with merges'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 1f4cfc37449..2f3e3e24169 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -13,6 +13,7 @@ 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 '
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 5495eacfec1..1b2c0d6aca6 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -11,6 +11,7 @@ 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 '
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index 95fe4feaeee..76d393dc8a3 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -5,6 +5,7 @@ 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 '
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index 7e11bd4a4c5..b71bad17b85 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -5,6 +5,7 @@ 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 '
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 84a587daf3a..dd5d92ef302 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -2,6 +2,7 @@
 
 test_description='Test cherry-pick -x and -s'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 pristine_detach () {
-- 
2.39.0.1153.g589e4efe9dc

Re: [PATCH 05/10] builtin/revert.c: fix common leak by using replay_opts_release()

[René Scharfe]

Am 30.12.22 um 08:28 schrieb Ævar Arnfjörð Bjarmason:
> Use the replay_opts_release() function introduced in a preceding
> commit to fix a memory leak in run_sequencer().
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/revert.c                  | 1 +
>  t/t3419-rebase-patch-id.sh        | 1 +
>  t/t3425-rebase-topology-merges.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 +
>  8 files changed, 9 insertions(+)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index e956d125a2b..2f656b25619 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -240,6 +240,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>  		cmd == 's' ? sequencer_skip :
>  		sequencer_pick_revisions;
>  	ret = cbfun(the_repository, opts);
> +	replay_opts_release(opts);

Is this the right place for this call?  opts is passed in by this function's
two callers.  They should clean it up instead, no?  That would add one line,
but avoid the need for patch 4.  Would make patch 6 easier to read as well.

>  	return ret;
>  }
>
> diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
> index 7181f176b81..6c61f240cf9 100755
> --- a/t/t3419-rebase-patch-id.sh
> +++ b/t/t3419-rebase-patch-id.sh
> @@ -5,6 +5,7 @@ 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 () {
> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> index 63acc1ea4da..a16428bdf54 100755
> --- a/t/t3425-rebase-topology-merges.sh
> +++ b/t/t3425-rebase-topology-merges.sh
> @@ -1,6 +1,8 @@
>  #!/bin/sh
>
>  test_description='rebase topology tests with merges'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  . "$TEST_DIRECTORY"/lib-rebase.sh
>
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 1f4cfc37449..2f3e3e24169 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -13,6 +13,7 @@ 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 '
> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
> index 5495eacfec1..1b2c0d6aca6 100755
> --- a/t/t3502-cherry-pick-merge.sh
> +++ b/t/t3502-cherry-pick-merge.sh
> @@ -11,6 +11,7 @@ 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 '
> diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
> index 95fe4feaeee..76d393dc8a3 100755
> --- a/t/t3503-cherry-pick-root.sh
> +++ b/t/t3503-cherry-pick-root.sh
> @@ -5,6 +5,7 @@ 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 '
> diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
> index 7e11bd4a4c5..b71bad17b85 100755
> --- a/t/t3506-cherry-pick-ff.sh
> +++ b/t/t3506-cherry-pick-ff.sh
> @@ -5,6 +5,7 @@ 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 '
> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
> index 84a587daf3a..dd5d92ef302 100755
> --- a/t/t3511-cherry-pick-x.sh
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -2,6 +2,7 @@
>
>  test_description='Test cherry-pick -x and -s'
>
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>
>  pristine_detach () {

Re: [PATCH 05/10] builtin/revert.c: fix common leak by using replay_opts_release()

[Phillip Wood]

On 30/12/2022 23:37, René Scharfe wrote:
> Am 30.12.22 um 08:28 schrieb Ævar Arnfjörð Bjarmason:
>> Use the replay_opts_release() function introduced in a preceding
>> commit to fix a memory leak in run_sequencer().
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   builtin/revert.c                  | 1 +
>>   t/t3419-rebase-patch-id.sh        | 1 +
>>   t/t3425-rebase-topology-merges.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 +
>>   8 files changed, 9 insertions(+)
>>
>> diff --git a/builtin/revert.c b/builtin/revert.c
>> index e956d125a2b..2f656b25619 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -240,6 +240,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>>   		cmd == 's' ? sequencer_skip :
>>   		sequencer_pick_revisions;
>>   	ret = cbfun(the_repository, opts);
>> +	replay_opts_release(opts);
> 
> Is this the right place for this call?  opts is passed in by this function's
> two callers.  They should clean it up instead, no?  That would add one line,
> but avoid the need for patch 4.  Would make patch 6 easier to read as well.

Yes I agree that would be a better approach.

Best Wishes

Phillip

>>   	return ret;
>>   }
>>
>> diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
>> index 7181f176b81..6c61f240cf9 100755
>> --- a/t/t3419-rebase-patch-id.sh
>> +++ b/t/t3419-rebase-patch-id.sh
>> @@ -5,6 +5,7 @@ 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 () {
>> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
>> index 63acc1ea4da..a16428bdf54 100755
>> --- a/t/t3425-rebase-topology-merges.sh
>> +++ b/t/t3425-rebase-topology-merges.sh
>> @@ -1,6 +1,8 @@
>>   #!/bin/sh
>>
>>   test_description='rebase topology tests with merges'
>> +
>> +TEST_PASSES_SANITIZE_LEAK=true
>>   . ./test-lib.sh
>>   . "$TEST_DIRECTORY"/lib-rebase.sh
>>
>> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
>> index 1f4cfc37449..2f3e3e24169 100755
>> --- a/t/t3501-revert-cherry-pick.sh
>> +++ b/t/t3501-revert-cherry-pick.sh
>> @@ -13,6 +13,7 @@ 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 '
>> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
>> index 5495eacfec1..1b2c0d6aca6 100755
>> --- a/t/t3502-cherry-pick-merge.sh
>> +++ b/t/t3502-cherry-pick-merge.sh
>> @@ -11,6 +11,7 @@ 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 '
>> diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
>> index 95fe4feaeee..76d393dc8a3 100755
>> --- a/t/t3503-cherry-pick-root.sh
>> +++ b/t/t3503-cherry-pick-root.sh
>> @@ -5,6 +5,7 @@ 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 '
>> diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
>> index 7e11bd4a4c5..b71bad17b85 100755
>> --- a/t/t3506-cherry-pick-ff.sh
>> +++ b/t/t3506-cherry-pick-ff.sh
>> @@ -5,6 +5,7 @@ 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 '
>> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
>> index 84a587daf3a..dd5d92ef302 100755
>> --- a/t/t3511-cherry-pick-x.sh
>> +++ b/t/t3511-cherry-pick-x.sh
>> @@ -2,6 +2,7 @@
>>
>>   test_description='Test cherry-pick -x and -s'
>>
>> +TEST_PASSES_SANITIZE_LEAK=true
>>   . ./test-lib.sh
>>
>>   pristine_detach () {
> 

[PATCH 06/10] builtin/revert.c: move free-ing of "revs" to replay_opts_release()

[Ævar Arnfjörð Bjarmason]

In [1] and [2] I added the code being moved here to cmd_revert() and
cmd_cherry_pick(), now that we've got a "replay_opts_release()" for
the "struct replay_opts" it should know how to free these "revs",
rather than having these users reach into the struct to free its
individual members.

As explained in earlier change we should be using FREE_AND_NULL() in
replay_opts_release() rather than free().

1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
   2022-11-08)
2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c | 8 ++------
 sequencer.c      | 3 +++
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 2f656b25619..b9fb13c515a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -254,9 +254,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	res = run_sequencer(argc, argv, &opts);
 	if (res < 0)
 		die(_("revert failed"));
-	if (opts.revs)
-		release_revisions(opts.revs);
-	free(opts.revs);
+	replay_opts_release(&opts);
 	return res;
 }
 
@@ -268,9 +266,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_PICK;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, &opts);
-	if (opts.revs)
-		release_revisions(opts.revs);
-	free(opts.revs);
+	replay_opts_release(&opts);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
diff --git a/sequencer.c b/sequencer.c
index e29a97b6caa..47367e66842 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -362,6 +362,9 @@ void replay_opts_release(struct replay_opts *opts)
 	opts->xopts_nr = 0;
 	FREE_AND_NULL(opts->xopts);
 	strbuf_release(&opts->current_fixups);
+	if (opts->revs)
+		release_revisions(opts->revs);
+	FREE_AND_NULL(opts->revs);
 }
 
 int sequencer_remove_state(struct replay_opts *opts)
-- 
2.39.0.1153.g589e4efe9dc

[PATCH 07/10] builtin/rebase.c: fix "options.onto_name" leak

[Ævar Arnfjörð Bjarmason]

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.

1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c                 | 3 ++-
 t/t3416-rebase-onto-threedots.sh | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 91bf55be6e6..f7fd20d71c0 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1658,7 +1658,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		strbuf_addstr(&buf, options.upstream_name);
 		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);
 	} else if (!options.onto_name)
 		options.onto_name = options.upstream_name;
 	if (strstr(options.onto_name, "...")) {
diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index ea501f2b42b..f8c4ed78c9e 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-rebase.sh"
 
-- 
2.39.0.1153.g589e4efe9dc

Re: [PATCH 07/10] builtin/rebase.c: fix "options.onto_name" leak

[Phillip Wood]

Hi Ævar

On 30/12/2022 07:28, Ævar Arnfjörð Bjarmason wrote:
> 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.

I'm afraid I don't really follow the reasoning here. "squash_onto_name" 
is only used if --root is given and that option is incompatible with 
--keep-base so the --keep-base branch has no business touching it. 
Having said that I think you've stumbled upon a viable solution if you 
drop the unneeded free() added below and rename "squash_onto_name" to 
something like "onto_name_to_free".

Best Wishes

Phillip


> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
> 2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c                 | 3 ++-
>   t/t3416-rebase-onto-threedots.sh | 1 +
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 91bf55be6e6..f7fd20d71c0 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1658,7 +1658,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		strbuf_addstr(&buf, options.upstream_name);
>   		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);
>   	} else if (!options.onto_name)
>   		options.onto_name = options.upstream_name;
>   	if (strstr(options.onto_name, "...")) {
> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index ea501f2b42b..f8c4ed78c9e 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY/lib-rebase.sh"
>   

[PATCH 08/10] sequencer.c: always free() the "msgbuf" in do_pick_commit()

[Ævar Arnfjörð Bjarmason]

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

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;
 }
-- 
2.39.0.1153.g589e4efe9dc

Re: [PATCH 08/10] sequencer.c: always free() the "msgbuf" in do_pick_commit()

[Phillip Wood]

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;
>   }

[PATCH 09/10] builtin/rebase.c: free() "options.strategy_opts"

[Ævar Arnfjörð Bjarmason]

When the "strategy_opts" member was added in ba1905a5fef (builtin
rebase: add support for custom merge strategies, 2018-09-04) the
corresponding free() for it at the end of cmd_rebase() wasn't added,
let's do so.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index f7fd20d71c0..70b037dc43a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1833,6 +1833,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	free(options.gpg_sign_opt);
 	free(options.cmd);
 	free(options.strategy);
+	free(options.strategy_opts);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
 	string_list_clear(&exec, 0);
-- 
2.39.0.1153.g589e4efe9dc

[PATCH 10/10] commit.c: free() revs.commit in get_fork_point()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak that's been with us since d96855ff517 (merge-base:
teach "--fork-point" mode, 2013-10-23).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit.c                       | 1 +
 t/t3431-rebase-fork-point.sh   | 1 +
 t/t3432-rebase-fast-forward.sh | 1 +
 3 files changed, 3 insertions(+)

diff --git a/commit.c b/commit.c
index d00780bee5f..8174c927383 100644
--- a/commit.c
+++ b/commit.c
@@ -1022,6 +1022,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)
 	ret = bases->item;
 
 cleanup_return:
+	free(revs.commit);
 	free_commit_list(bases);
 	free(full_refname);
 	return ret;
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 70e81363569..4bfc779bb87 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -8,6 +8,7 @@ test_description='git rebase --fork-point test'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # A---B---D---E    (main)
diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh
index 5086e14c022..7f1a5dd3deb 100755
--- a/t/t3432-rebase-fast-forward.sh
+++ b/t/t3432-rebase-fast-forward.sh
@@ -8,6 +8,7 @@ test_description='ensure rebase fast-forwards commits when possible'
 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 '
-- 
2.39.0.1153.g589e4efe9dc

Re: [PATCH 00/10] sequencer API & users: fix widespread leaks

[Phillip Wood]

Hi Ævar

On 30/12/2022 07:28, Ævar Arnfjörð Bjarmason wrote:
> 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.
> 
> The main change is the 3/10 here, where we introduce a
> replay_opts_release() to free the "struct replay_opts". The rest is
> then either refactorings to be able to call that destructor
> (e.g. "return" to "goto cleanup"), or other miscellanious adjacent
> leaks.
> 
> This is a follow-up to the discussion ending at [1], as noted there
> the recent ff84d031a9d (Merge branch 'pw/rebase-no-reflog-action',
> 2022-11-23) ended up introducing a leak because of the disfunctional
> lack of a destructor (or rather, the current logic being tied up in
> sequencer_remove_state().

Thanks for working on this. Separating out replay_opts_release() from 
sequencer_remove_state() enables us to free struct replay_opts properly. 
I've left some comments, the patches I haven't commented on all looked fine.

Best Wishes

Phillip

> This can be queued and graduated independently of the other concurrent
> leak series I've submitted[2]. When the two are combined we'll end up
> passing more tests, i.e. both topics combined get us over the finish
> line for some of them, but neither one is enough in isolation.
> 
> But that's OK, we just won't opt them into the "linux-leaks"
> testing. I'll submit a follow-up similar to [3] at some later date to
> mark them as passing. I think that's a better trade-off than making
> these two depend on one another.
> 
> 1. https://lore.kernel.org/git/221108.864jv9sc9r.gmgdl@evledraar.gmail.com/
> 2. https://lore.kernel.org/git/cover-v2-00.20-00000000000-20221230T020341Z-avarab@gmail.com/
> 3. https://lore.kernel.org/git/patch-v2-01.20-3de29c6d75f-20221230T020341Z-avarab@gmail.com/
> 
> Ævar Arnfjörð Bjarmason (10):
>    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: refactor run_sequencer() return pattern
>    builtin/revert.c: fix common leak by using replay_opts_release()
>    builtin/revert.c: move free-ing of "revs" to replay_opts_release()
>    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                       | 19 +++++-----
>   builtin/revert.c                       | 40 +++++++++++----------
>   commit.c                               |  1 +
>   sequencer.c                            | 48 +++++++++++++++++---------
>   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, 82 insertions(+), 47 deletions(-)
> 

Re: [PATCH 00/10] sequencer API & users: fix widespread leaks

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Thanks for working on this. Separating out replay_opts_release() from
> sequencer_remove_state() enables us to free struct replay_opts
> properly. I've left some comments, the patches I haven't commented on
> all looked fine.

Ditto.  I agree with the review comments I so far saw on the list,
and this topic needs a bit more work, but it is great to aim for
allowing us freeing replay_opts properly.

Thanks, all.

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

[Ævar Arnfjörð Bjarmason]

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

[PATCH v2 1/9] rebase: use "cleanup" pattern in do_interactive_rebase()

[Ævar Arnfjörð Bjarmason]

Use a "goto cleanup" pattern in do_interactive_rebase(). This
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>
---
 builtin/rebase.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5b..7141fd5e0c1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -256,7 +256,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)
 
 static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 {
-	int ret;
+	int ret = -1;
 	char *revisions = NULL, *shortrevisions = NULL;
 	struct strvec make_script_args = STRVEC_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
@@ -265,16 +265,12 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 
 	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
 				&revisions, &shortrevisions))
-		return -1;
+		goto cleanup;
 
 	if (init_basic_state(&replay,
 			     opts->head_name ? opts->head_name : "detached HEAD",
-			     opts->onto, &opts->orig_head->object.oid)) {
-		free(revisions);
-		free(shortrevisions);
-
-		return -1;
-	}
+			     opts->onto, &opts->orig_head->object.oid))
+		goto cleanup;
 
 	if (!opts->upstream && opts->squash_onto)
 		write_file(path_squash_onto(), "%s\n",
@@ -304,6 +300,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 			opts->autosquash, opts->update_refs, &todo_list);
 	}
 
+cleanup:
 	string_list_clear(&commands, 0);
 	free(revisions);
 	free(shortrevisions);
-- 
2.39.0.1205.g2ca064edc27

[PATCH v2 2/9] sequencer.c: split up sequencer_remove_state()

[Ævar Arnfjörð Bjarmason]

Split off the free()-ing in sequencer_remove_state() into a utility
function, which will be adjusted and called independent of the other
code in sequencer_remove_state() in a subsequent commit.

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 | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bcb662e23be..d385bea2bed 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,10 +351,22 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
 	return buf.buf;
 }
 
+static void replay_opts_release(struct replay_opts *opts)
+{
+	free(opts->gpg_sign);
+	free(opts->reflog_action);
+	free(opts->default_strategy);
+	free(opts->strategy);
+	for (size_t i = 0; i < opts->xopts_nr; i++)
+		free(opts->xopts[i]);
+	free(opts->xopts);
+	strbuf_release(&opts->current_fixups);
+}
+
 int sequencer_remove_state(struct replay_opts *opts)
 {
 	struct strbuf buf = STRBUF_INIT;
-	int i, ret = 0;
+	int ret = 0;
 
 	if (is_rebase_i(opts) &&
 	    strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
@@ -373,14 +385,7 @@ int sequencer_remove_state(struct replay_opts *opts)
 		}
 	}
 
-	free(opts->gpg_sign);
-	free(opts->reflog_action);
-	free(opts->default_strategy);
-	free(opts->strategy);
-	for (i = 0; i < opts->xopts_nr; i++)
-		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));
-- 
2.39.0.1205.g2ca064edc27

[PATCH v2 3/9] rebase & sequencer API: fix get_replay_opts() leak in "rebase"

[Ævar Arnfjörð Bjarmason]

Make the recently added replay_opts_release() function non-static and
use it for freeing the "struct replay_opts" constructed by the
get_replay_opts() function in "builtin/rebase.c". See [1] for the
initial addition of get_replay_opts().

To safely call our new replay_opts_release() we'll need to change all
the free() to a FREE_AND_NULL(), and set "xopts_nr" to "0" after we
loop over it and free() it (the free() in the loop doesn't need to be
a FREE_AND_NULL()).

This is because in e.g. do_interactive_rebase() we construct a "struct
replay_opts" with "get_replay_opts()", and then call
"complete_action()". If we get far enough in that function without
encountering errors we'll call "pick_commits()" which (indirectly)
calls sequencer_remove_state() at the end.

But if we encounter errors anywhere along the way we'd punt out early,
and not free() the memory we allocated. Remembering whether we
previously called sequencer_remove_state() would be a hassle, so let's
make it safe to re-invoke replay_opts_release() instead.

I experimented with a change to be more paranoid instead, i.e. to
exhaustively check our state via an enum. We could make sure that we:

- Only allow calling "replay_opts_release()" after
  "sequencer_remove_state()", but not the other way around.

- Forbid invoking either function twice in a row.

But such paranoia isn't warranted here, let's instead take the easy
way out and FREE_AND_NULL() this.

See [2] for the initial implementation of "sequencer_remove_state()",
which assumed that it should be removing the full (including on-disk)
rebase state as a one-off.

1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
   2019-04-17)
2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
   2011-08-04)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c                       | 4 ++++
 builtin/revert.c                       | 2 ++
 sequencer.c                            | 5 ++---
 sequencer.h                            | 1 +
 t/t3405-rebase-malformed.sh            | 1 +
 t/t3412-rebase-root.sh                 | 1 +
 t/t3419-rebase-patch-id.sh             | 1 +
 t/t3423-rebase-reword.sh               | 1 +
 t/t3425-rebase-topology-merges.sh      | 2 ++
 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 -
 19 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 7141fd5e0c1..5859a5387d8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -301,6 +301,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	}
 
 cleanup:
+	replay_opts_release(&replay);
 	string_list_clear(&commands, 0);
 	free(revisions);
 	free(shortrevisions);
@@ -343,6 +344,7 @@ static int run_sequencer_rebase(struct rebase_options *opts)
 		struct replay_opts replay_opts = get_replay_opts(opts);
 
 		ret = sequencer_continue(the_repository, &replay_opts);
+		replay_opts_release(&replay_opts);
 		break;
 	}
 	case ACTION_EDIT_TODO:
@@ -558,6 +560,7 @@ 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))
@@ -1331,6 +1334,7 @@ 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);
diff --git a/builtin/revert.c b/builtin/revert.c
index f2d86d2a8f9..1cab16bf3ed 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -251,6 +251,7 @@ 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;
 }
 
@@ -267,5 +268,6 @@ 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;
 }
diff --git a/sequencer.c b/sequencer.c
index d385bea2bed..5d8c68912a1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,7 +351,7 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
 	return buf.buf;
 }
 
-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);
@@ -359,6 +359,7 @@ 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]);
+	opts->xopts_nr = 0;
 	free(opts->xopts);
 	strbuf_release(&opts->current_fixups);
 }
@@ -385,8 +386,6 @@ 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))
diff --git a/sequencer.h b/sequencer.h
index 888c18aad71..3bcdfa1b586 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -158,6 +158,7 @@ int sequencer_pick_revisions(struct repository *repo,
 int sequencer_continue(struct repository *repo, struct replay_opts *opts);
 int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_skip(struct repository *repo, struct replay_opts *opts);
+void replay_opts_release(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
 #define TODO_LIST_KEEP_EMPTY (1U << 0)
diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
index 25243318618..8979bc34073 100755
--- a/t/t3405-rebase-malformed.sh
+++ b/t/t3405-rebase-malformed.sh
@@ -5,6 +5,7 @@ test_description='rebase should handle arbitrary git message'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 58371d8a547..e75b3d0e07c 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -7,6 +7,7 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 log_with_names () {
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index 7181f176b81..6c61f240cf9 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -5,6 +5,7 @@ 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 () {
diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
index 4859bb8f722..2fab703d615 100755
--- a/t/t3423-rebase-reword.sh
+++ b/t/t3423-rebase-reword.sh
@@ -2,6 +2,7 @@
 
 test_description='git rebase interactive with rewording'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
index 63acc1ea4da..a16428bdf54 100755
--- a/t/t3425-rebase-topology-merges.sh
+++ b/t/t3425-rebase-topology-merges.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='rebase topology tests with merges'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index c023fefd681..274699dadb8 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -14,6 +14,7 @@ to the "fixup" command that works with "fixup!", "fixup -C" works with
 "amend!" upon --autosquash.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh
index b92a3ce46b8..c614c4f2e4b 100755
--- a/t/t3438-rebase-broken-files.sh
+++ b/t/t3438-rebase-broken-files.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='rebase behavior when on-disk files are broken'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'set up conflicting branches' '
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 1f4cfc37449..2f3e3e24169 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -13,6 +13,7 @@ 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 '
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 5495eacfec1..1b2c0d6aca6 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -11,6 +11,7 @@ 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 '
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index 95fe4feaeee..76d393dc8a3 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -5,6 +5,7 @@ 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 '
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index 7e11bd4a4c5..b71bad17b85 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -5,6 +5,7 @@ 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 '
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 84a587daf3a..dd5d92ef302 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -2,6 +2,7 @@
 
 test_description='Test cherry-pick -x and -s'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 pristine_detach () {
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index ebeca12a711..b19792b3269 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -5,6 +5,7 @@
 
 test_description='Test rebasing, stashing, etc. with submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
index 3cab0b9720a..bca496c40e0 100755
--- a/t/t9106-git-svn-commit-diff-clobber.sh
+++ b/t/t9106-git-svn-commit-diff-clobber.sh
@@ -3,7 +3,6 @@
 # Copyright (c) 2006 Eric Wong
 test_description='git svn commit-diff clobber'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'initialize repo' '
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index 1465156072e..c8e6c0733f4 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -5,7 +5,6 @@
 
 test_description='concurrent git svn dcommit'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 
-- 
2.39.0.1205.g2ca064edc27

Re: [PATCH v2 3/9] rebase & sequencer API: fix get_replay_opts() leak in "rebase"

[Phillip Wood]

Hi Ævar

On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> Make the recently added replay_opts_release() function non-static and
> use it for freeing the "struct replay_opts" constructed by the
> get_replay_opts() function in "builtin/rebase.c". See [1] for the
> initial addition of get_replay_opts().
> 
> To safely call our new replay_opts_release() we'll need to change all
> the free() to a FREE_AND_NULL(), and set "xopts_nr" to "0" after we
> loop over it and free() it (the free() in the loop doesn't need to be
> a FREE_AND_NULL()).
> 
> This is because in e.g. do_interactive_rebase() we construct a "struct
> replay_opts" with "get_replay_opts()", and then call
> "complete_action()". If we get far enough in that function without
> encountering errors we'll call "pick_commits()" which (indirectly)
> calls sequencer_remove_state() at the end.
> 
> But if we encounter errors anywhere along the way we'd punt out early,
> and not free() the memory we allocated. Remembering whether we
> previously called sequencer_remove_state() would be a hassle, so let's
> make it safe to re-invoke replay_opts_release() instead.
> 
> I experimented with a change to be more paranoid instead, i.e. to
> exhaustively check our state via an enum. We could make sure that we:
> 
> - Only allow calling "replay_opts_release()" after
>    "sequencer_remove_state()", but not the other way around.
> 
> - Forbid invoking either function twice in a row.
> 
> But such paranoia isn't warranted here, let's instead take the easy
> way out and FREE_AND_NULL() this.

The changes below look good, but this message needs updating to reflect 
the re-roll.

> @@ -359,6 +359,7 @@ 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]);
> +	opts->xopts_nr = 0;

I don't think we need this now we're only calling replay_opts_release() 
once.

Best Wishes

Phillip

[PATCH v2 4/9] builtin/revert.c: move free-ing of "revs" to replay_opts_release()

[Ævar Arnfjörð Bjarmason]

In [1] and [2] I added the code being moved here to cmd_revert() and
cmd_cherry_pick(), now that we've got a "replay_opts_release()" for
the "struct replay_opts" it should know how to free these "revs",
rather than having these users reach into the struct to free its
individual members.

As explained in earlier change we should be using FREE_AND_NULL() in
replay_opts_release() rather than free().

1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
   2022-11-08)
2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c | 6 ------
 sequencer.c      | 3 +++
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1cab16bf3ed..77d2035616e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -248,9 +248,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	res = run_sequencer(argc, argv, &opts);
 	if (res < 0)
 		die(_("revert failed"));
-	if (opts.revs)
-		release_revisions(opts.revs);
-	free(opts.revs);
 	replay_opts_release(&opts);
 	return res;
 }
@@ -263,9 +260,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_PICK;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, &opts);
-	if (opts.revs)
-		release_revisions(opts.revs);
-	free(opts.revs);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	replay_opts_release(&opts);
diff --git a/sequencer.c b/sequencer.c
index 5d8c68912a1..c729ce77260 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -362,6 +362,9 @@ void replay_opts_release(struct replay_opts *opts)
 	opts->xopts_nr = 0;
 	free(opts->xopts);
 	strbuf_release(&opts->current_fixups);
+	if (opts->revs)
+		release_revisions(opts->revs);
+	free(opts->revs);
 }
 
 int sequencer_remove_state(struct replay_opts *opts)
-- 
2.39.0.1205.g2ca064edc27

Re: [PATCH v2 4/9] builtin/revert.c: move free-ing of "revs" to replay_opts_release()

[Phillip Wood]

Hi Ævar

On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> In [1] and [2] I added the code being moved here to cmd_revert() and
> cmd_cherry_pick(),

I'm not sure how relevant this first clause is but the change itself 
looks good.

> now that we've got a "replay_opts_release()" for
> the "struct replay_opts" it should know how to free these "revs",
> rather than having these users reach into the struct to free its
> individual members.
> 
> As explained in earlier change we should be using FREE_AND_NULL() in
> replay_opts_release() rather than free().

This paragraph is out of date

Best Wishes

Phillip

> 1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
>     2022-11-08)
> 2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/revert.c | 6 ------
>   sequencer.c      | 3 +++
>   2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 1cab16bf3ed..77d2035616e 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -248,9 +248,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>   	res = run_sequencer(argc, argv, &opts);
>   	if (res < 0)
>   		die(_("revert failed"));
> -	if (opts.revs)
> -		release_revisions(opts.revs);
> -	free(opts.revs);
>   	replay_opts_release(&opts);
>   	return res;
>   }
> @@ -263,9 +260,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>   	opts.action = REPLAY_PICK;
>   	sequencer_init_config(&opts);
>   	res = run_sequencer(argc, argv, &opts);
> -	if (opts.revs)
> -		release_revisions(opts.revs);
> -	free(opts.revs);
>   	if (res < 0)
>   		die(_("cherry-pick failed"));
>   	replay_opts_release(&opts);
> diff --git a/sequencer.c b/sequencer.c
> index 5d8c68912a1..c729ce77260 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -362,6 +362,9 @@ void replay_opts_release(struct replay_opts *opts)
>   	opts->xopts_nr = 0;
>   	free(opts->xopts);
>   	strbuf_release(&opts->current_fixups);
> +	if (opts->revs)
> +		release_revisions(opts->revs);
> +	free(opts->revs);
>   }
>   
>   int sequencer_remove_state(struct replay_opts *opts)

[PATCH v2 5/9] builtin/rebase.c: rename "squash_onto_name" to "to_free"

[Ævar Arnfjörð Bjarmason]

The real use of the "squash_onto_name" added in [1] is to keep track
of a value for later free()-ing, we don't subsequently use it for
anything else.

Let's rename it in preparation for re-using it for free()-ing before
another assignment to "options.onto_name", which is an outstanding
leak that'll be fixed in a subsequent commit.

1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5859a5387d8..0d8c050f6b3 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1036,7 +1036,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	const char *rebase_merges = NULL;
 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
 	struct object_id squash_onto;
-	char *squash_onto_name = NULL;
+	char *to_free = NULL;
 	int reschedule_failed_exec = -1;
 	int allow_preemptive_ff = 1;
 	int preserve_merges_selected = 0;
@@ -1589,7 +1589,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 					&squash_onto, NULL, NULL) < 0)
 				die(_("Could not create new root commit"));
 			options.squash_onto = &squash_onto;
-			options.onto_name = squash_onto_name =
+			options.onto_name = to_free =
 				xstrdup(oid_to_hex(&squash_onto));
 		} else
 			options.root_with_onto = 1;
@@ -1835,7 +1835,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	free(options.cmd);
 	free(options.strategy);
 	strbuf_release(&options.git_format_patch_opt);
-	free(squash_onto_name);
+	free(to_free);
 	string_list_clear(&exec, 0);
 	string_list_clear(&strategy_options, 0);
 	return !!ret;
-- 
2.39.0.1205.g2ca064edc27

Re: [PATCH v2 5/9] builtin/rebase.c: rename "squash_onto_name" to "to_free"

[Phillip Wood]

On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> The real use of the "squash_onto_name" added in [1] is to keep track
> of a value for later free()-ing, we don't subsequently use it for
> anything else.
> 
> Let's rename it in preparation for re-using it for free()-ing before
> another assignment to "options.onto_name", which is an outstanding
> leak that'll be fixed in a subsequent commit.

This is good

Thanks

Phillip

> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5859a5387d8..0d8c050f6b3 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1036,7 +1036,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	const char *rebase_merges = NULL;
>   	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
>   	struct object_id squash_onto;
> -	char *squash_onto_name = NULL;
> +	char *to_free = NULL;
>   	int reschedule_failed_exec = -1;
>   	int allow_preemptive_ff = 1;
>   	int preserve_merges_selected = 0;
> @@ -1589,7 +1589,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   					&squash_onto, NULL, NULL) < 0)
>   				die(_("Could not create new root commit"));
>   			options.squash_onto = &squash_onto;
> -			options.onto_name = squash_onto_name =
> +			options.onto_name = to_free =
>   				xstrdup(oid_to_hex(&squash_onto));
>   		} else
>   			options.root_with_onto = 1;
> @@ -1835,7 +1835,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	free(options.cmd);
>   	free(options.strategy);
>   	strbuf_release(&options.git_format_patch_opt);
> -	free(squash_onto_name);
> +	free(to_free);
>   	string_list_clear(&exec, 0);
>   	string_list_clear(&strategy_options, 0);
>   	return !!ret;

[PATCH v2 6/9] builtin/rebase.c: fix "options.onto_name" leak

[Ævar Arnfjörð Bjarmason]

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

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c                 | 3 ++-
 t/t3416-rebase-onto-threedots.sh | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0d8c050f6b3..b4857b89f19 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1660,7 +1660,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		strbuf_addstr(&buf, options.upstream_name);
 		strbuf_addstr(&buf, "...");
 		strbuf_addstr(&buf, branch_name);
-		options.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, "...")) {
diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index ea501f2b42b..f8c4ed78c9e 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-rebase.sh"
 
-- 
2.39.0.1205.g2ca064edc27

Re: [PATCH v2 6/9] builtin/rebase.c: fix "options.onto_name" leak

[Phillip Wood]

Hi Ævar

On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> 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 value before
> re-assigning to "options.onto_name". Let's do that, and fix the memory
> leak.

As I said before I don't think this message makes any sense. It should 
just say that when --keep-base is given we need to free 
options.onto_name as it does not come from argv. As I also mentioned you 
do not need to add "free(to_free)" as it is unused at this point. 
Freeing it makes the reader wonder what was previously assigned to it.

Best Wishes

Phillip

> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
> 2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c                 | 3 ++-
>   t/t3416-rebase-onto-threedots.sh | 1 +
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 0d8c050f6b3..b4857b89f19 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1660,7 +1660,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		strbuf_addstr(&buf, options.upstream_name);
>   		strbuf_addstr(&buf, "...");
>   		strbuf_addstr(&buf, branch_name);
> -		options.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, "...")) {
> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index ea501f2b42b..f8c4ed78c9e 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY/lib-rebase.sh"
>   

[PATCH v2 7/9] sequencer.c: always free() the "msgbuf" in do_pick_commit()

[Ævar Arnfjörð Bjarmason]

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>
---
 sequencer.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c729ce77260..31ea9b036cd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2278,8 +2278,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 leave;
+		}
 		flags |= AMEND_MSG;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
@@ -2289,9 +2291,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 leave;
+			}
 			unlink(git_path_merge_msg(r));
 			msg_file = dest;
 			flags |= EDIT_MSG;
@@ -2329,7 +2333,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,6 +2406,7 @@ static int do_pick_commit(struct repository *r,
 leave:
 	free_message(commit, &msg);
 	free(author);
+	strbuf_release(&msgbuf);
 	update_abort_safety_file();
 
 	return res;
-- 
2.39.0.1205.g2ca064edc27

[PATCH v2 8/9] builtin/rebase.c: free() "options.strategy_opts"

[Ævar Arnfjörð Bjarmason]

When the "strategy_opts" member was added in ba1905a5fef (builtin
rebase: add support for custom merge strategies, 2018-09-04) the
corresponding free() for it at the end of cmd_rebase() wasn't added,
let's do so.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b4857b89f19..4a5a34de6e2 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1835,6 +1835,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	free(options.gpg_sign_opt);
 	free(options.cmd);
 	free(options.strategy);
+	free(options.strategy_opts);
 	strbuf_release(&options.git_format_patch_opt);
 	free(to_free);
 	string_list_clear(&exec, 0);
-- 
2.39.0.1205.g2ca064edc27

[PATCH v2 9/9] commit.c: free() revs.commit in get_fork_point()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak that's been with us since d96855ff517 (merge-base:
teach "--fork-point" mode, 2013-10-23).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit.c                       | 1 +
 t/t3431-rebase-fork-point.sh   | 1 +
 t/t3432-rebase-fast-forward.sh | 1 +
 3 files changed, 3 insertions(+)

diff --git a/commit.c b/commit.c
index 14538a811ae..e433c33bb01 100644
--- a/commit.c
+++ b/commit.c
@@ -1033,6 +1033,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)
 	ret = bases->item;
 
 cleanup_return:
+	free(revs.commit);
 	free_commit_list(bases);
 	free(full_refname);
 	return ret;
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 70e81363569..4bfc779bb87 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -8,6 +8,7 @@ test_description='git rebase --fork-point test'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # A---B---D---E    (main)
diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh
index 5086e14c022..7f1a5dd3deb 100755
--- a/t/t3432-rebase-fast-forward.sh
+++ b/t/t3432-rebase-fast-forward.sh
@@ -8,6 +8,7 @@ test_description='ensure rebase fast-forwards commits when possible'
 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 '
-- 
2.39.0.1205.g2ca064edc27

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

[Phillip Wood]

Hi Ævar

The code changes in this version look good modulo a couple of minor 
comments, however some of the commit messages need to be updated to 
reflect the (very welcome) changes you've made in v2.

Thanks again for working on this

Phillip

On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> 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()

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Ævar
>
> The code changes in this version look good modulo a couple of minor
> comments, however some of the commit messages need to be updated to
> reflect the (very welcome) changes you've made in v2.
>
> Thanks again for working on this

Thanks, both.  Perhaps third-time a charm, with a bit more care on
the presentation ;-)

[PATCH v3 0/8] sequencer API & users: fix widespread leaks

[Ævar Arnfjörð Bjarmason]

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 v2:

 * Reword/amend commit messages for some of the functional changes in
   v1..v2.
 * Remove leftover "opts->xopts_nr = 0" now that we don't
   FREE_AND_NULL() anymore.
 * Drop the "squash_onto_name" refactoring to a "to_free"
 * Instead add a new "keep_base_onto_name" in the next commit. I'd
   missed that if both options were provided we'd die(), so that
   free() wasn't needed before re-assignment, as Phillip pointed
   out...

CI & branch for this available at:
https://github.com/avar/git/tree/avar/leak-fixes-sequencer-rebase-3

Ævar Arnfjörð Bjarmason (8):
  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: 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                       | 22 ++++++++------
 builtin/revert.c                       |  8 ++---
 commit.c                               |  1 +
 sequencer.c                            | 42 ++++++++++++++++----------
 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, 61 insertions(+), 33 deletions(-)

Range-diff against v2:
 1:  d0a0524f3d4 =  1:  b223429df33 rebase: use "cleanup" pattern in do_interactive_rebase()
 2:  c4eaa8dfef4 =  2:  00c7f04363f sequencer.c: split up sequencer_remove_state()
 3:  f06f565ceaf !  3:  e4a96898a68 rebase & sequencer API: fix get_replay_opts() leak in "rebase"
    @@ Commit message
         get_replay_opts() function in "builtin/rebase.c". See [1] for the
         initial addition of get_replay_opts().
     
    -    To safely call our new replay_opts_release() we'll need to change all
    -    the free() to a FREE_AND_NULL(), and set "xopts_nr" to "0" after we
    -    loop over it and free() it (the free() in the loop doesn't need to be
    -    a FREE_AND_NULL()).
    +    To safely call our new replay_opts_release() we'll need to stop
    +    calling it in sequencer_remove_state(), and instead call it where we
    +    allocate the "struct replay_opts" itself.
     
         This is because in e.g. do_interactive_rebase() we construct a "struct
         replay_opts" with "get_replay_opts()", and then call
    @@ Commit message
     
         But if we encounter errors anywhere along the way we'd punt out early,
         and not free() the memory we allocated. Remembering whether we
    -    previously called sequencer_remove_state() would be a hassle, so let's
    -    make it safe to re-invoke replay_opts_release() instead.
    +    previously called sequencer_remove_state() would be a hassle.
     
    -    I experimented with a change to be more paranoid instead, i.e. to
    -    exhaustively check our state via an enum. We could make sure that we:
    -
    -    - Only allow calling "replay_opts_release()" after
    -      "sequencer_remove_state()", but not the other way around.
    -
    -    - Forbid invoking either function twice in a row.
    -
    -    But such paranoia isn't warranted here, let's instead take the easy
    -    way out and FREE_AND_NULL() this.
    +    Using a FREE_AND_NULL() pattern would also work, as it would be safe
    +    replay_opts_release() repeatedly, but let's fix this properly instead,
    +    by having the owner of the data free() it.
     
         See [2] for the initial implementation of "sequencer_remove_state()",
         which assumed that it should be removing the full (including on-disk)
    @@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
      {
      	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]);
    -+	opts->xopts_nr = 0;
    - 	free(opts->xopts);
    - 	strbuf_release(&opts->current_fixups);
    - }
     @@ sequencer.c: int sequencer_remove_state(struct replay_opts *opts)
      		}
      	}
 4:  e83bdfab046 !  4:  9f72cc6e46b builtin/revert.c: move free-ing of "revs" to replay_opts_release()
    @@ Commit message
         rather than having these users reach into the struct to free its
         individual members.
     
    -    As explained in earlier change we should be using FREE_AND_NULL() in
    -    replay_opts_release() rather than free().
    -
         1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
            2022-11-08)
         2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)
    @@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *p
     
      ## sequencer.c ##
     @@ sequencer.c: void replay_opts_release(struct replay_opts *opts)
    - 	opts->xopts_nr = 0;
    + 		free(opts->xopts[i]);
      	free(opts->xopts);
      	strbuf_release(&opts->current_fixups);
     +	if (opts->revs)
 5:  4fea2b77c6d <  -:  ----------- builtin/rebase.c: rename "squash_onto_name" to "to_free"
 6:  898bb7698fc !  5:  3d5c3152f69 builtin/rebase.c: fix "options.onto_name" leak
    @@ Metadata
      ## Commit message ##
         builtin/rebase.c: fix "options.onto_name" leak
     
    -    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 value before
    -    re-assigning to "options.onto_name". Let's do that, and fix the memory
    -    leak.
    +    Similar to the existing "squash_onto_name" added in [1] we need to
    +    free() the xstrdup()'d "options.onto.name" added for "--keep-base" in
    +    [2]..
     
         1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
         2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/rebase.c ##
    +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
    + 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
    + 	struct object_id squash_onto;
    + 	char *squash_onto_name = NULL;
    ++	char *keep_base_onto_name = NULL;
    + 	int reschedule_failed_exec = -1;
    + 	int allow_preemptive_ff = 1;
    + 	int preserve_merges_selected = 0;
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
      		strbuf_addstr(&buf, options.upstream_name);
      		strbuf_addstr(&buf, "...");
      		strbuf_addstr(&buf, branch_name);
     -		options.onto_name = xstrdup(buf.buf);
    -+		free(to_free);
    -+		options.onto_name = to_free = xstrdup(buf.buf);
    ++		options.onto_name = keep_base_onto_name = xstrdup(buf.buf);
      	} else if (!options.onto_name)
      		options.onto_name = options.upstream_name;
      	if (strstr(options.onto_name, "...")) {
    +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
    + 	free(options.strategy);
    + 	strbuf_release(&options.git_format_patch_opt);
    + 	free(squash_onto_name);
    ++	free(keep_base_onto_name);
    + 	string_list_clear(&exec, 0);
    + 	string_list_clear(&strategy_options, 0);
    + 	return !!ret;
     
      ## t/t3416-rebase-onto-threedots.sh ##
     @@ t/t3416-rebase-onto-threedots.sh: test_description='git rebase --onto A...B'
 7:  fb38dc873f9 =  6:  c07dc006c6d sequencer.c: always free() the "msgbuf" in do_pick_commit()
 8:  d4b0e2a5c83 !  7:  ee8262ab22a 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(to_free);
    - 	string_list_clear(&exec, 0);
    + 	free(squash_onto_name);
    + 	free(keep_base_onto_name);
 9:  fd9c7a5547c =  8:  84343ea6bf6 commit.c: free() revs.commit in get_fork_point()
-- 
2.39.0.1225.g30a3d88132d

[PATCH v3 1/8] rebase: use "cleanup" pattern in do_interactive_rebase()

[Ævar Arnfjörð Bjarmason]

Use a "goto cleanup" pattern in do_interactive_rebase(). This
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>
---
 builtin/rebase.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1481c5b6a5b..7141fd5e0c1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -256,7 +256,7 @@ static void split_exec_commands(const char *cmd, struct string_list *commands)
 
 static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 {
-	int ret;
+	int ret = -1;
 	char *revisions = NULL, *shortrevisions = NULL;
 	struct strvec make_script_args = STRVEC_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
@@ -265,16 +265,12 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 
 	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
 				&revisions, &shortrevisions))
-		return -1;
+		goto cleanup;
 
 	if (init_basic_state(&replay,
 			     opts->head_name ? opts->head_name : "detached HEAD",
-			     opts->onto, &opts->orig_head->object.oid)) {
-		free(revisions);
-		free(shortrevisions);
-
-		return -1;
-	}
+			     opts->onto, &opts->orig_head->object.oid))
+		goto cleanup;
 
 	if (!opts->upstream && opts->squash_onto)
 		write_file(path_squash_onto(), "%s\n",
@@ -304,6 +300,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 			opts->autosquash, opts->update_refs, &todo_list);
 	}
 
+cleanup:
 	string_list_clear(&commands, 0);
 	free(revisions);
 	free(shortrevisions);
-- 
2.39.0.1225.g30a3d88132d

[PATCH v3 2/8] sequencer.c: split up sequencer_remove_state()

[Ævar Arnfjörð Bjarmason]

Split off the free()-ing in sequencer_remove_state() into a utility
function, which will be adjusted and called independent of the other
code in sequencer_remove_state() in a subsequent commit.

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 | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index bcb662e23be..d385bea2bed 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,10 +351,22 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
 	return buf.buf;
 }
 
+static void replay_opts_release(struct replay_opts *opts)
+{
+	free(opts->gpg_sign);
+	free(opts->reflog_action);
+	free(opts->default_strategy);
+	free(opts->strategy);
+	for (size_t i = 0; i < opts->xopts_nr; i++)
+		free(opts->xopts[i]);
+	free(opts->xopts);
+	strbuf_release(&opts->current_fixups);
+}
+
 int sequencer_remove_state(struct replay_opts *opts)
 {
 	struct strbuf buf = STRBUF_INIT;
-	int i, ret = 0;
+	int ret = 0;
 
 	if (is_rebase_i(opts) &&
 	    strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
@@ -373,14 +385,7 @@ int sequencer_remove_state(struct replay_opts *opts)
 		}
 	}
 
-	free(opts->gpg_sign);
-	free(opts->reflog_action);
-	free(opts->default_strategy);
-	free(opts->strategy);
-	for (i = 0; i < opts->xopts_nr; i++)
-		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));
-- 
2.39.0.1225.g30a3d88132d

[PATCH v3 3/8] rebase & sequencer API: fix get_replay_opts() leak in "rebase"

[Ævar Arnfjörð Bjarmason]

Make the recently added replay_opts_release() function non-static and
use it for freeing the "struct replay_opts" constructed by the
get_replay_opts() function in "builtin/rebase.c". See [1] for the
initial addition of get_replay_opts().

To safely call our new replay_opts_release() we'll need to stop
calling it in sequencer_remove_state(), and instead call it where we
allocate the "struct replay_opts" itself.

This is because in e.g. do_interactive_rebase() we construct a "struct
replay_opts" with "get_replay_opts()", and then call
"complete_action()". If we get far enough in that function without
encountering errors we'll call "pick_commits()" which (indirectly)
calls sequencer_remove_state() at the end.

But if we encounter errors anywhere along the way we'd punt out early,
and not free() the memory we allocated. Remembering whether we
previously called sequencer_remove_state() would be a hassle.

Using a FREE_AND_NULL() pattern would also work, as it would be safe
replay_opts_release() repeatedly, but let's fix this properly instead,
by having the owner of the data free() it.

See [2] for the initial implementation of "sequencer_remove_state()",
which assumed that it should be removing the full (including on-disk)
rebase state as a one-off.

1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
   2019-04-17)
2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
   2011-08-04)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c                       | 4 ++++
 builtin/revert.c                       | 2 ++
 sequencer.c                            | 4 +---
 sequencer.h                            | 1 +
 t/t3405-rebase-malformed.sh            | 1 +
 t/t3412-rebase-root.sh                 | 1 +
 t/t3419-rebase-patch-id.sh             | 1 +
 t/t3423-rebase-reword.sh               | 1 +
 t/t3425-rebase-topology-merges.sh      | 2 ++
 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 -
 19 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 7141fd5e0c1..5859a5387d8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -301,6 +301,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	}
 
 cleanup:
+	replay_opts_release(&replay);
 	string_list_clear(&commands, 0);
 	free(revisions);
 	free(shortrevisions);
@@ -343,6 +344,7 @@ static int run_sequencer_rebase(struct rebase_options *opts)
 		struct replay_opts replay_opts = get_replay_opts(opts);
 
 		ret = sequencer_continue(the_repository, &replay_opts);
+		replay_opts_release(&replay_opts);
 		break;
 	}
 	case ACTION_EDIT_TODO:
@@ -558,6 +560,7 @@ 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))
@@ -1331,6 +1334,7 @@ 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);
diff --git a/builtin/revert.c b/builtin/revert.c
index f2d86d2a8f9..1cab16bf3ed 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -251,6 +251,7 @@ 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;
 }
 
@@ -267,5 +268,6 @@ 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;
 }
diff --git a/sequencer.c b/sequencer.c
index d385bea2bed..8ff29262c1e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,7 +351,7 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
 	return buf.buf;
 }
 
-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);
@@ -385,8 +385,6 @@ 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))
diff --git a/sequencer.h b/sequencer.h
index 888c18aad71..3bcdfa1b586 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -158,6 +158,7 @@ int sequencer_pick_revisions(struct repository *repo,
 int sequencer_continue(struct repository *repo, struct replay_opts *opts);
 int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_skip(struct repository *repo, struct replay_opts *opts);
+void replay_opts_release(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
 #define TODO_LIST_KEEP_EMPTY (1U << 0)
diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
index 25243318618..8979bc34073 100755
--- a/t/t3405-rebase-malformed.sh
+++ b/t/t3405-rebase-malformed.sh
@@ -5,6 +5,7 @@ test_description='rebase should handle arbitrary git message'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 58371d8a547..e75b3d0e07c 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -7,6 +7,7 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 log_with_names () {
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index 7181f176b81..6c61f240cf9 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -5,6 +5,7 @@ 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 () {
diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
index 4859bb8f722..2fab703d615 100755
--- a/t/t3423-rebase-reword.sh
+++ b/t/t3423-rebase-reword.sh
@@ -2,6 +2,7 @@
 
 test_description='git rebase interactive with rewording'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
index 63acc1ea4da..a16428bdf54 100755
--- a/t/t3425-rebase-topology-merges.sh
+++ b/t/t3425-rebase-topology-merges.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='rebase topology tests with merges'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index c023fefd681..274699dadb8 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -14,6 +14,7 @@ to the "fixup" command that works with "fixup!", "fixup -C" works with
 "amend!" upon --autosquash.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh
index b92a3ce46b8..c614c4f2e4b 100755
--- a/t/t3438-rebase-broken-files.sh
+++ b/t/t3438-rebase-broken-files.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='rebase behavior when on-disk files are broken'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'set up conflicting branches' '
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 1f4cfc37449..2f3e3e24169 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -13,6 +13,7 @@ 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 '
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 5495eacfec1..1b2c0d6aca6 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -11,6 +11,7 @@ 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 '
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index 95fe4feaeee..76d393dc8a3 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -5,6 +5,7 @@ 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 '
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index 7e11bd4a4c5..b71bad17b85 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -5,6 +5,7 @@ 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 '
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 84a587daf3a..dd5d92ef302 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -2,6 +2,7 @@
 
 test_description='Test cherry-pick -x and -s'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 pristine_detach () {
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index ebeca12a711..b19792b3269 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -5,6 +5,7 @@
 
 test_description='Test rebasing, stashing, etc. with submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
index 3cab0b9720a..bca496c40e0 100755
--- a/t/t9106-git-svn-commit-diff-clobber.sh
+++ b/t/t9106-git-svn-commit-diff-clobber.sh
@@ -3,7 +3,6 @@
 # Copyright (c) 2006 Eric Wong
 test_description='git svn commit-diff clobber'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'initialize repo' '
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index 1465156072e..c8e6c0733f4 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -5,7 +5,6 @@
 
 test_description='concurrent git svn dcommit'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 
-- 
2.39.0.1225.g30a3d88132d

Re: [PATCH v3 3/8] rebase & sequencer API: fix get_replay_opts() leak in "rebase"

[Phillip Wood]

Hi Ævar

On 18/01/2023 16:09, Ævar Arnfjörð Bjarmason wrote:
> Make the recently added replay_opts_release() function non-static and
> use it for freeing the "struct replay_opts" constructed by the
> get_replay_opts() function in "builtin/rebase.c". See [1] for the
> initial addition of get_replay_opts().

This paragraph only talks about rebase but we're also modifying 
cherry-pick and revert here. I'm not sure the commit reference really 
adds anything to the discussion.

> To safely call our new replay_opts_release() we'll need to stop
> calling it in sequencer_remove_state(), and instead call it where we
> allocate the "struct replay_opts" itself.

This paragraph is good, I'm not sure we really need most of the next 
three. It is much cleaner for the owner of a struct to free it and 
that's what we should be doing wherever possible.

> This is because in e.g. do_interactive_rebase() we construct a "struct
> replay_opts" with "get_replay_opts()", and then call
> "complete_action()". If we get far enough in that function without
> encountering errors we'll call "pick_commits()" which (indirectly)
> calls sequencer_remove_state() at the end.
> 
> But if we encounter errors anywhere along the way we'd punt out early,
> and not free() the memory we allocated. Remembering whether we
> previously called sequencer_remove_state() would be a hassle.
> 
> Using a FREE_AND_NULL() pattern would also work, as it would be safe
> replay_opts_release() repeatedly, but let's fix this properly instead,
> by having the owner of the data free() it.
> > See [2] for the initial implementation of "sequencer_remove_state()",
> which assumed that it should be removing the full (including on-disk)
> rebase state as a one-off.

That commit has nothing to do with rebase, it is cleaning up after 
cherry-pick/revert. rebase was still scripted back then.

The code changes look good

Best Wishes

Phillip

> 1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
>     2019-04-17)
> 2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
>     2011-08-04)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c                       | 4 ++++
>   builtin/revert.c                       | 2 ++
>   sequencer.c                            | 4 +---
>   sequencer.h                            | 1 +
>   t/t3405-rebase-malformed.sh            | 1 +
>   t/t3412-rebase-root.sh                 | 1 +
>   t/t3419-rebase-patch-id.sh             | 1 +
>   t/t3423-rebase-reword.sh               | 1 +
>   t/t3425-rebase-topology-merges.sh      | 2 ++
>   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 -
>   19 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 7141fd5e0c1..5859a5387d8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -301,6 +301,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>   	}
>   
>   cleanup:
> +	replay_opts_release(&replay);
>   	string_list_clear(&commands, 0);
>   	free(revisions);
>   	free(shortrevisions);
> @@ -343,6 +344,7 @@ static int run_sequencer_rebase(struct rebase_options *opts)
>   		struct replay_opts replay_opts = get_replay_opts(opts);
>   
>   		ret = sequencer_continue(the_repository, &replay_opts);
> +		replay_opts_release(&replay_opts);
>   		break;
>   	}
>   	case ACTION_EDIT_TODO:
> @@ -558,6 +560,7 @@ 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))
> @@ -1331,6 +1334,7 @@ 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);
> diff --git a/builtin/revert.c b/builtin/revert.c
> index f2d86d2a8f9..1cab16bf3ed 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -251,6 +251,7 @@ 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;
>   }
>   
> @@ -267,5 +268,6 @@ 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;
>   }
> diff --git a/sequencer.c b/sequencer.c
> index d385bea2bed..8ff29262c1e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -351,7 +351,7 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>   	return buf.buf;
>   }
>   
> -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);
> @@ -385,8 +385,6 @@ 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))
> diff --git a/sequencer.h b/sequencer.h
> index 888c18aad71..3bcdfa1b586 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -158,6 +158,7 @@ int sequencer_pick_revisions(struct repository *repo,
>   int sequencer_continue(struct repository *repo, struct replay_opts *opts);
>   int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
>   int sequencer_skip(struct repository *repo, struct replay_opts *opts);
> +void replay_opts_release(struct replay_opts *opts);
>   int sequencer_remove_state(struct replay_opts *opts);
>   
>   #define TODO_LIST_KEEP_EMPTY (1U << 0)
> diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
> index 25243318618..8979bc34073 100755
> --- a/t/t3405-rebase-malformed.sh
> +++ b/t/t3405-rebase-malformed.sh
> @@ -5,6 +5,7 @@ test_description='rebase should handle arbitrary git message'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY"/lib-rebase.sh
>   
> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
> index 58371d8a547..e75b3d0e07c 100755
> --- a/t/t3412-rebase-root.sh
> +++ b/t/t3412-rebase-root.sh
> @@ -7,6 +7,7 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   log_with_names () {
> diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
> index 7181f176b81..6c61f240cf9 100755
> --- a/t/t3419-rebase-patch-id.sh
> +++ b/t/t3419-rebase-patch-id.sh
> @@ -5,6 +5,7 @@ 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 () {
> diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
> index 4859bb8f722..2fab703d615 100755
> --- a/t/t3423-rebase-reword.sh
> +++ b/t/t3423-rebase-reword.sh
> @@ -2,6 +2,7 @@
>   
>   test_description='git rebase interactive with rewording'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   . "$TEST_DIRECTORY"/lib-rebase.sh
> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> index 63acc1ea4da..a16428bdf54 100755
> --- a/t/t3425-rebase-topology-merges.sh
> +++ b/t/t3425-rebase-topology-merges.sh
> @@ -1,6 +1,8 @@
>   #!/bin/sh
>   
>   test_description='rebase topology tests with merges'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY"/lib-rebase.sh
>   
> diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
> index c023fefd681..274699dadb8 100755
> --- a/t/t3437-rebase-fixup-options.sh
> +++ b/t/t3437-rebase-fixup-options.sh
> @@ -14,6 +14,7 @@ to the "fixup" command that works with "fixup!", "fixup -C" works with
>   "amend!" upon --autosquash.
>   '
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   . "$TEST_DIRECTORY"/lib-rebase.sh
> diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh
> index b92a3ce46b8..c614c4f2e4b 100755
> --- a/t/t3438-rebase-broken-files.sh
> +++ b/t/t3438-rebase-broken-files.sh
> @@ -1,6 +1,8 @@
>   #!/bin/sh
>   
>   test_description='rebase behavior when on-disk files are broken'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success 'set up conflicting branches' '
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 1f4cfc37449..2f3e3e24169 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -13,6 +13,7 @@ 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 '
> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
> index 5495eacfec1..1b2c0d6aca6 100755
> --- a/t/t3502-cherry-pick-merge.sh
> +++ b/t/t3502-cherry-pick-merge.sh
> @@ -11,6 +11,7 @@ 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 '
> diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
> index 95fe4feaeee..76d393dc8a3 100755
> --- a/t/t3503-cherry-pick-root.sh
> +++ b/t/t3503-cherry-pick-root.sh
> @@ -5,6 +5,7 @@ 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 '
> diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
> index 7e11bd4a4c5..b71bad17b85 100755
> --- a/t/t3506-cherry-pick-ff.sh
> +++ b/t/t3506-cherry-pick-ff.sh
> @@ -5,6 +5,7 @@ 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 '
> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
> index 84a587daf3a..dd5d92ef302 100755
> --- a/t/t3511-cherry-pick-x.sh
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -2,6 +2,7 @@
>   
>   test_description='Test cherry-pick -x and -s'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   pristine_detach () {
> diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
> index ebeca12a711..b19792b3269 100755
> --- a/t/t7402-submodule-rebase.sh
> +++ b/t/t7402-submodule-rebase.sh
> @@ -5,6 +5,7 @@
>   
>   test_description='Test rebasing, stashing, etc. with submodules'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success setup '
> diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
> index 3cab0b9720a..bca496c40e0 100755
> --- a/t/t9106-git-svn-commit-diff-clobber.sh
> +++ b/t/t9106-git-svn-commit-diff-clobber.sh
> @@ -3,7 +3,6 @@
>   # Copyright (c) 2006 Eric Wong
>   test_description='git svn commit-diff clobber'
>   
> -TEST_FAILS_SANITIZE_LEAK=true
>   . ./lib-git-svn.sh
>   
>   test_expect_success 'initialize repo' '
> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
> index 1465156072e..c8e6c0733f4 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -5,7 +5,6 @@
>   
>   test_description='concurrent git svn dcommit'
>   
> -TEST_FAILS_SANITIZE_LEAK=true
>   . ./lib-git-svn.sh
>   
>   

[PATCH v3 4/8] builtin/revert.c: move free-ing of "revs" to replay_opts_release()

[Ævar Arnfjörð Bjarmason]

In [1] and [2] I added the code being moved here to cmd_revert() and
cmd_cherry_pick(), now that we've got a "replay_opts_release()" for
the "struct replay_opts" it should know how to free these "revs",
rather than having these users reach into the struct to free its
individual members.

1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
   2022-11-08)
2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c | 6 ------
 sequencer.c      | 3 +++
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1cab16bf3ed..77d2035616e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -248,9 +248,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	res = run_sequencer(argc, argv, &opts);
 	if (res < 0)
 		die(_("revert failed"));
-	if (opts.revs)
-		release_revisions(opts.revs);
-	free(opts.revs);
 	replay_opts_release(&opts);
 	return res;
 }
@@ -263,9 +260,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_PICK;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, &opts);
-	if (opts.revs)
-		release_revisions(opts.revs);
-	free(opts.revs);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	replay_opts_release(&opts);
diff --git a/sequencer.c b/sequencer.c
index 8ff29262c1e..ea568d99498 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -361,6 +361,9 @@ void replay_opts_release(struct replay_opts *opts)
 		free(opts->xopts[i]);
 	free(opts->xopts);
 	strbuf_release(&opts->current_fixups);
+	if (opts->revs)
+		release_revisions(opts->revs);
+	free(opts->revs);
 }
 
 int sequencer_remove_state(struct replay_opts *opts)
-- 
2.39.0.1225.g30a3d88132d

[PATCH v3 5/8] builtin/rebase.c: fix "options.onto_name" leak

[Ævar Arnfjörð Bjarmason]

Similar to the existing "squash_onto_name" added in [1] we need to
free() the xstrdup()'d "options.onto.name" added for "--keep-base" in
[2]..

1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c                 | 4 +++-
 t/t3416-rebase-onto-threedots.sh | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5859a5387d8..5c474fb6edd 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1037,6 +1037,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
 	struct object_id squash_onto;
 	char *squash_onto_name = NULL;
+	char *keep_base_onto_name = NULL;
 	int reschedule_failed_exec = -1;
 	int allow_preemptive_ff = 1;
 	int preserve_merges_selected = 0;
@@ -1660,7 +1661,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		strbuf_addstr(&buf, options.upstream_name);
 		strbuf_addstr(&buf, "...");
 		strbuf_addstr(&buf, branch_name);
-		options.onto_name = xstrdup(buf.buf);
+		options.onto_name = keep_base_onto_name = xstrdup(buf.buf);
 	} else if (!options.onto_name)
 		options.onto_name = options.upstream_name;
 	if (strstr(options.onto_name, "...")) {
@@ -1836,6 +1837,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	free(options.strategy);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
+	free(keep_base_onto_name);
 	string_list_clear(&exec, 0);
 	string_list_clear(&strategy_options, 0);
 	return !!ret;
diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index ea501f2b42b..f8c4ed78c9e 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-rebase.sh"
 
-- 
2.39.0.1225.g30a3d88132d

Re: [PATCH v3 5/8] builtin/rebase.c: fix "options.onto_name" leak

[Phillip Wood]

Hi Ævar

On 18/01/2023 16:09, Ævar Arnfjörð Bjarmason wrote:
> Similar to the existing "squash_onto_name" added in [1] we need to
> free() the xstrdup()'d "options.onto.name" added for "--keep-base" in
> [2]..

Apart from the unnecessary free() I was quite happy with the previous 
implementation that renamed squash_onto_name and used that but this 
patch looks fine.

Best Wishes

Phillip

> 1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
> 2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c                 | 4 +++-
>   t/t3416-rebase-onto-threedots.sh | 1 +
>   2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5859a5387d8..5c474fb6edd 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1037,6 +1037,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
>   	struct object_id squash_onto;
>   	char *squash_onto_name = NULL;
> +	char *keep_base_onto_name = NULL;
>   	int reschedule_failed_exec = -1;
>   	int allow_preemptive_ff = 1;
>   	int preserve_merges_selected = 0;
> @@ -1660,7 +1661,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		strbuf_addstr(&buf, options.upstream_name);
>   		strbuf_addstr(&buf, "...");
>   		strbuf_addstr(&buf, branch_name);
> -		options.onto_name = xstrdup(buf.buf);
> +		options.onto_name = keep_base_onto_name = xstrdup(buf.buf);
>   	} else if (!options.onto_name)
>   		options.onto_name = options.upstream_name;
>   	if (strstr(options.onto_name, "...")) {
> @@ -1836,6 +1837,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	free(options.strategy);
>   	strbuf_release(&options.git_format_patch_opt);
>   	free(squash_onto_name);
> +	free(keep_base_onto_name);
>   	string_list_clear(&exec, 0);
>   	string_list_clear(&strategy_options, 0);
>   	return !!ret;
> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index ea501f2b42b..f8c4ed78c9e 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY/lib-rebase.sh"
>   

[PATCH v3 6/8] sequencer.c: always free() the "msgbuf" in do_pick_commit()

[Ævar Arnfjörð Bjarmason]

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>
---
 sequencer.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ea568d99498..2c5ed5bc5ea 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2277,8 +2277,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 leave;
+		}
 		flags |= AMEND_MSG;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
@@ -2288,9 +2290,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 leave;
+			}
 			unlink(git_path_merge_msg(r));
 			msg_file = dest;
 			flags |= EDIT_MSG;
@@ -2328,7 +2332,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
@@ -2402,6 +2405,7 @@ static int do_pick_commit(struct repository *r,
 leave:
 	free_message(commit, &msg);
 	free(author);
+	strbuf_release(&msgbuf);
 	update_abort_safety_file();
 
 	return res;
-- 
2.39.0.1225.g30a3d88132d

[PATCH v3 7/8] builtin/rebase.c: free() "options.strategy_opts"

[Ævar Arnfjörð Bjarmason]

When the "strategy_opts" member was added in ba1905a5fef (builtin
rebase: add support for custom merge strategies, 2018-09-04) the
corresponding free() for it at the end of cmd_rebase() wasn't added,
let's do so.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5c474fb6edd..611c1d20ddf 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1835,6 +1835,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	free(options.gpg_sign_opt);
 	free(options.cmd);
 	free(options.strategy);
+	free(options.strategy_opts);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
 	free(keep_base_onto_name);
-- 
2.39.0.1225.g30a3d88132d

[PATCH v4 0/8] sequencer API & users: fix widespread leaks

[Ævar Arnfjörð Bjarmason]

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 v3:

* Rebased for newer "master", there were some conflicts due to
  adjacent changes.
* Addressed Phillip's commit message comments (hopefully).

Branch & CI for this at:
https://github.com/avar/git/tree/avar/leak-fixes-sequencer-rebase-4

Ævar Arnfjörð Bjarmason (8):
  rebase: use "cleanup" pattern in do_interactive_rebase()
  sequencer.c: split up sequencer_remove_state()
  sequencer API users: fix get_replay_opts() leaks
  builtin/revert.c: move free-ing of "revs" to replay_opts_release()
  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                       | 22 ++++++++------
 builtin/revert.c                       |  8 ++---
 commit.c                               |  1 +
 sequencer.c                            | 42 ++++++++++++++++----------
 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, 61 insertions(+), 33 deletions(-)

Range-diff against v3:
1:  b223429df33 ! 1:  029fc5f4b8c rebase: use "cleanup" pattern in do_interactive_rebase()
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/rebase.c ##
    -@@ builtin/rebase.c: static void split_exec_commands(const char *cmd, struct string_list *commands)
    +@@ builtin/rebase.c: static int init_basic_state(struct replay_opts *opts, const char *head_name,
      
      static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
      {
    @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts,
      	}
      
     +cleanup:
    - 	string_list_clear(&commands, 0);
      	free(revisions);
      	free(shortrevisions);
    + 	todo_list_release(&todo_list);
2:  00c7f04363f = 2:  b0c9da95ca1 sequencer.c: split up sequencer_remove_state()
3:  e4a96898a68 ! 3:  dbac0501424 rebase & sequencer API: fix get_replay_opts() leak in "rebase"
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    rebase & sequencer API: fix get_replay_opts() leak in "rebase"
    +    sequencer API users: fix get_replay_opts() leaks
     
    -    Make the recently added replay_opts_release() function non-static and
    -    use it for freeing the "struct replay_opts" constructed by the
    -    get_replay_opts() function in "builtin/rebase.c". See [1] for the
    -    initial addition of get_replay_opts().
    +    Make the replay_opts_release() function added in the preceding commit
    +    non-static, and use it for freeing the "struct replay_opts"
    +    constructed for "rebase" and "revert".
     
         To safely call our new replay_opts_release() we'll need to stop
         calling it in sequencer_remove_state(), and instead call it where we
    @@ Commit message
         previously called sequencer_remove_state() would be a hassle.
     
         Using a FREE_AND_NULL() pattern would also work, as it would be safe
    -    replay_opts_release() repeatedly, but let's fix this properly instead,
    -    by having the owner of the data free() it.
    -
    -    See [2] for the initial implementation of "sequencer_remove_state()",
    -    which assumed that it should be removing the full (including on-disk)
    -    rebase state as a one-off.
    -
    -    1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
    -       2019-04-17)
    -    2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
    -       2011-08-04)
    +    to call replay_opts_release() repeatedly. But let's fix this properly
    +    instead, by having the owner of the data free() it.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts,
      
      cleanup:
     +	replay_opts_release(&replay);
    - 	string_list_clear(&commands, 0);
      	free(revisions);
      	free(shortrevisions);
    + 	todo_list_release(&todo_list);
     @@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts)
      		struct replay_opts replay_opts = get_replay_opts(opts);
      
4:  9f72cc6e46b = 4:  6b29d7d00c2 builtin/revert.c: move free-ing of "revs" to replay_opts_release()
5:  3d5c3152f69 ! 5:  f9c4d17fe70 builtin/rebase.c: fix "options.onto_name" leak
    @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
      	strbuf_release(&options.git_format_patch_opt);
      	free(squash_onto_name);
     +	free(keep_base_onto_name);
    - 	string_list_clear(&exec, 0);
      	string_list_clear(&strategy_options, 0);
      	return !!ret;
    + }
     
      ## t/t3416-rebase-onto-threedots.sh ##
     @@ t/t3416-rebase-onto-threedots.sh: test_description='git rebase --onto A...B'
6:  c07dc006c6d = 6:  5c2870ed2e6 sequencer.c: always free() the "msgbuf" in do_pick_commit()
7:  ee8262ab22a ! 7:  07ab875c3e2 builtin/rebase.c: free() "options.strategy_opts"
    @@ Commit message
      ## builtin/rebase.c ##
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
      	free(options.gpg_sign_opt);
    - 	free(options.cmd);
    + 	string_list_clear(&options.exec, 0);
      	free(options.strategy);
     +	free(options.strategy_opts);
      	strbuf_release(&options.git_format_patch_opt);
8:  84343ea6bf6 = 8:  6ab2edcc135 commit.c: free() revs.commit in get_fork_point()
-- 
2.39.1.1425.ge02fe682bd8

[PATCH v4 1/8] rebase: use "cleanup" pattern in do_interactive_rebase()

[Ævar Arnfjörð Bjarmason]

Use a "goto cleanup" pattern in do_interactive_rebase(). This
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>
---
 builtin/rebase.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 7171be40eeb..c97ce642cf3 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -254,7 +254,7 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
 
 static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 {
-	int ret;
+	int ret = -1;
 	char *revisions = NULL, *shortrevisions = NULL;
 	struct strvec make_script_args = STRVEC_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
@@ -262,16 +262,12 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 
 	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
 				&revisions, &shortrevisions))
-		return -1;
+		goto cleanup;
 
 	if (init_basic_state(&replay,
 			     opts->head_name ? opts->head_name : "detached HEAD",
-			     opts->onto, &opts->orig_head->object.oid)) {
-		free(revisions);
-		free(shortrevisions);
-
-		return -1;
-	}
+			     opts->onto, &opts->orig_head->object.oid))
+		goto cleanup;
 
 	if (!opts->upstream && opts->squash_onto)
 		write_file(path_squash_onto(), "%s\n",
@@ -300,6 +296,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 			opts->autosquash, opts->update_refs, &todo_list);
 	}
 
+cleanup:
 	free(revisions);
 	free(shortrevisions);
 	todo_list_release(&todo_list);
-- 
2.39.1.1425.ge02fe682bd8

[PATCH v4 2/8] sequencer.c: split up sequencer_remove_state()

[Ævar Arnfjörð Bjarmason]

Split off the free()-ing in sequencer_remove_state() into a utility
function, which will be adjusted and called independent of the other
code in sequencer_remove_state() in a subsequent commit.

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 | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3e4a1972897..b6392b43204 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,10 +351,22 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
 	return buf.buf;
 }
 
+static void replay_opts_release(struct replay_opts *opts)
+{
+	free(opts->gpg_sign);
+	free(opts->reflog_action);
+	free(opts->default_strategy);
+	free(opts->strategy);
+	for (size_t i = 0; i < opts->xopts_nr; i++)
+		free(opts->xopts[i]);
+	free(opts->xopts);
+	strbuf_release(&opts->current_fixups);
+}
+
 int sequencer_remove_state(struct replay_opts *opts)
 {
 	struct strbuf buf = STRBUF_INIT;
-	int i, ret = 0;
+	int ret = 0;
 
 	if (is_rebase_i(opts) &&
 	    strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {
@@ -373,14 +385,7 @@ int sequencer_remove_state(struct replay_opts *opts)
 		}
 	}
 
-	free(opts->gpg_sign);
-	free(opts->reflog_action);
-	free(opts->default_strategy);
-	free(opts->strategy);
-	for (i = 0; i < opts->xopts_nr; i++)
-		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));
-- 
2.39.1.1425.ge02fe682bd8

[PATCH v4 3/8] sequencer API users: fix get_replay_opts() leaks

[Ævar Arnfjörð Bjarmason]

Make the replay_opts_release() function added in the preceding commit
non-static, and use it for freeing the "struct replay_opts"
constructed for "rebase" and "revert".

To safely call our new replay_opts_release() we'll need to stop
calling it in sequencer_remove_state(), and instead call it where we
allocate the "struct replay_opts" itself.

This is because in e.g. do_interactive_rebase() we construct a "struct
replay_opts" with "get_replay_opts()", and then call
"complete_action()". If we get far enough in that function without
encountering errors we'll call "pick_commits()" which (indirectly)
calls sequencer_remove_state() at the end.

But if we encounter errors anywhere along the way we'd punt out early,
and not free() the memory we allocated. Remembering whether we
previously called sequencer_remove_state() would be a hassle.

Using a FREE_AND_NULL() pattern would also work, as it would be safe
to call replay_opts_release() repeatedly. But let's fix this properly
instead, by having the owner of the data free() it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c                       | 4 ++++
 builtin/revert.c                       | 2 ++
 sequencer.c                            | 4 +---
 sequencer.h                            | 1 +
 t/t3405-rebase-malformed.sh            | 1 +
 t/t3412-rebase-root.sh                 | 1 +
 t/t3419-rebase-patch-id.sh             | 1 +
 t/t3423-rebase-reword.sh               | 1 +
 t/t3425-rebase-topology-merges.sh      | 2 ++
 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 -
 19 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index c97ce642cf3..2ec3ae0b42e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -297,6 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	}
 
 cleanup:
+	replay_opts_release(&replay);
 	free(revisions);
 	free(shortrevisions);
 	todo_list_release(&todo_list);
@@ -338,6 +339,7 @@ static int run_sequencer_rebase(struct rebase_options *opts)
 		struct replay_opts replay_opts = get_replay_opts(opts);
 
 		ret = sequencer_continue(the_repository, &replay_opts);
+		replay_opts_release(&replay_opts);
 		break;
 	}
 	case ACTION_EDIT_TODO:
@@ -553,6 +555,7 @@ 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))
@@ -1324,6 +1327,7 @@ 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);
diff --git a/builtin/revert.c b/builtin/revert.c
index f2d86d2a8f9..1cab16bf3ed 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -251,6 +251,7 @@ 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;
 }
 
@@ -267,5 +268,6 @@ 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;
 }
diff --git a/sequencer.c b/sequencer.c
index b6392b43204..1547fb98597 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -351,7 +351,7 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
 	return buf.buf;
 }
 
-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);
@@ -385,8 +385,6 @@ 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))
diff --git a/sequencer.h b/sequencer.h
index 888c18aad71..3bcdfa1b586 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -158,6 +158,7 @@ int sequencer_pick_revisions(struct repository *repo,
 int sequencer_continue(struct repository *repo, struct replay_opts *opts);
 int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_skip(struct repository *repo, struct replay_opts *opts);
+void replay_opts_release(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
 #define TODO_LIST_KEEP_EMPTY (1U << 0)
diff --git a/t/t3405-rebase-malformed.sh b/t/t3405-rebase-malformed.sh
index 25243318618..8979bc34073 100755
--- a/t/t3405-rebase-malformed.sh
+++ b/t/t3405-rebase-malformed.sh
@@ -5,6 +5,7 @@ test_description='rebase should handle arbitrary git message'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
index 58371d8a547..e75b3d0e07c 100755
--- a/t/t3412-rebase-root.sh
+++ b/t/t3412-rebase-root.sh
@@ -7,6 +7,7 @@ Tests if git rebase --root --onto <newparent> can rebase the root commit.
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 log_with_names () {
diff --git a/t/t3419-rebase-patch-id.sh b/t/t3419-rebase-patch-id.sh
index 7181f176b81..6c61f240cf9 100755
--- a/t/t3419-rebase-patch-id.sh
+++ b/t/t3419-rebase-patch-id.sh
@@ -5,6 +5,7 @@ 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 () {
diff --git a/t/t3423-rebase-reword.sh b/t/t3423-rebase-reword.sh
index 4859bb8f722..2fab703d615 100755
--- a/t/t3423-rebase-reword.sh
+++ b/t/t3423-rebase-reword.sh
@@ -2,6 +2,7 @@
 
 test_description='git rebase interactive with rewording'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
index 63acc1ea4da..a16428bdf54 100755
--- a/t/t3425-rebase-topology-merges.sh
+++ b/t/t3425-rebase-topology-merges.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='rebase topology tests with merges'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index c023fefd681..274699dadb8 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -14,6 +14,7 @@ to the "fixup" command that works with "fixup!", "fixup -C" works with
 "amend!" upon --autosquash.
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 . "$TEST_DIRECTORY"/lib-rebase.sh
diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh
index b92a3ce46b8..c614c4f2e4b 100755
--- a/t/t3438-rebase-broken-files.sh
+++ b/t/t3438-rebase-broken-files.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='rebase behavior when on-disk files are broken'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'set up conflicting branches' '
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 1f4cfc37449..2f3e3e24169 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -13,6 +13,7 @@ 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 '
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 5495eacfec1..1b2c0d6aca6 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -11,6 +11,7 @@ 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 '
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index 95fe4feaeee..76d393dc8a3 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -5,6 +5,7 @@ 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 '
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index 7e11bd4a4c5..b71bad17b85 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -5,6 +5,7 @@ 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 '
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 84a587daf3a..dd5d92ef302 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -2,6 +2,7 @@
 
 test_description='Test cherry-pick -x and -s'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 pristine_detach () {
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index ebeca12a711..b19792b3269 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -5,6 +5,7 @@
 
 test_description='Test rebasing, stashing, etc. with submodules'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
index 3cab0b9720a..bca496c40e0 100755
--- a/t/t9106-git-svn-commit-diff-clobber.sh
+++ b/t/t9106-git-svn-commit-diff-clobber.sh
@@ -3,7 +3,6 @@
 # Copyright (c) 2006 Eric Wong
 test_description='git svn commit-diff clobber'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 test_expect_success 'initialize repo' '
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh b/t/t9164-git-svn-dcommit-concurrent.sh
index 1465156072e..c8e6c0733f4 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -5,7 +5,6 @@
 
 test_description='concurrent git svn dcommit'
 
-TEST_FAILS_SANITIZE_LEAK=true
 . ./lib-git-svn.sh
 
 
-- 
2.39.1.1425.ge02fe682bd8

[PATCH v4 4/8] builtin/revert.c: move free-ing of "revs" to replay_opts_release()

[Ævar Arnfjörð Bjarmason]

In [1] and [2] I added the code being moved here to cmd_revert() and
cmd_cherry_pick(), now that we've got a "replay_opts_release()" for
the "struct replay_opts" it should know how to free these "revs",
rather than having these users reach into the struct to free its
individual members.

1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
   2022-11-08)
2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/revert.c | 6 ------
 sequencer.c      | 3 +++
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1cab16bf3ed..77d2035616e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -248,9 +248,6 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	res = run_sequencer(argc, argv, &opts);
 	if (res < 0)
 		die(_("revert failed"));
-	if (opts.revs)
-		release_revisions(opts.revs);
-	free(opts.revs);
 	replay_opts_release(&opts);
 	return res;
 }
@@ -263,9 +260,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_PICK;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, &opts);
-	if (opts.revs)
-		release_revisions(opts.revs);
-	free(opts.revs);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	replay_opts_release(&opts);
diff --git a/sequencer.c b/sequencer.c
index 1547fb98597..cb4b1ce062c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -361,6 +361,9 @@ void replay_opts_release(struct replay_opts *opts)
 		free(opts->xopts[i]);
 	free(opts->xopts);
 	strbuf_release(&opts->current_fixups);
+	if (opts->revs)
+		release_revisions(opts->revs);
+	free(opts->revs);
 }
 
 int sequencer_remove_state(struct replay_opts *opts)
-- 
2.39.1.1425.ge02fe682bd8

[PATCH v4 5/8] builtin/rebase.c: fix "options.onto_name" leak

[Ævar Arnfjörð Bjarmason]

Similar to the existing "squash_onto_name" added in [1] we need to
free() the xstrdup()'d "options.onto.name" added for "--keep-base" in
[2]..

1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c                 | 4 +++-
 t/t3416-rebase-onto-threedots.sh | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 2ec3ae0b42e..41e96f0eb5a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1039,6 +1039,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
 	struct object_id squash_onto;
 	char *squash_onto_name = NULL;
+	char *keep_base_onto_name = NULL;
 	int reschedule_failed_exec = -1;
 	int allow_preemptive_ff = 1;
 	int preserve_merges_selected = 0;
@@ -1675,7 +1676,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		strbuf_addstr(&buf, options.upstream_name);
 		strbuf_addstr(&buf, "...");
 		strbuf_addstr(&buf, branch_name);
-		options.onto_name = xstrdup(buf.buf);
+		options.onto_name = keep_base_onto_name = xstrdup(buf.buf);
 	} else if (!options.onto_name)
 		options.onto_name = options.upstream_name;
 	if (strstr(options.onto_name, "...")) {
@@ -1851,6 +1852,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	free(options.strategy);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
+	free(keep_base_onto_name);
 	string_list_clear(&strategy_options, 0);
 	return !!ret;
 }
diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index ea501f2b42b..f8c4ed78c9e 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -5,6 +5,7 @@ test_description='git rebase --onto A...B'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY/lib-rebase.sh"
 
-- 
2.39.1.1425.ge02fe682bd8

[PATCH v4 6/8] sequencer.c: always free() the "msgbuf" in do_pick_commit()

[Ævar Arnfjörð Bjarmason]

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>
---
 sequencer.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index cb4b1ce062c..fb23f734ade 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2277,8 +2277,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 leave;
+		}
 		flags |= AMEND_MSG;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
@@ -2288,9 +2290,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 leave;
+			}
 			unlink(git_path_merge_msg(r));
 			msg_file = dest;
 			flags |= EDIT_MSG;
@@ -2328,7 +2332,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
@@ -2402,6 +2405,7 @@ static int do_pick_commit(struct repository *r,
 leave:
 	free_message(commit, &msg);
 	free(author);
+	strbuf_release(&msgbuf);
 	update_abort_safety_file();
 
 	return res;
-- 
2.39.1.1425.ge02fe682bd8

[PATCH v4 7/8] builtin/rebase.c: free() "options.strategy_opts"

[Ævar Arnfjörð Bjarmason]

When the "strategy_opts" member was added in ba1905a5fef (builtin
rebase: add support for custom merge strategies, 2018-09-04) the
corresponding free() for it at the end of cmd_rebase() wasn't added,
let's do so.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rebase.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 41e96f0eb5a..6635f10d529 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1850,6 +1850,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	free(options.gpg_sign_opt);
 	string_list_clear(&options.exec, 0);
 	free(options.strategy);
+	free(options.strategy_opts);
 	strbuf_release(&options.git_format_patch_opt);
 	free(squash_onto_name);
 	free(keep_base_onto_name);
-- 
2.39.1.1425.ge02fe682bd8

[PATCH v4 8/8] commit.c: free() revs.commit in get_fork_point()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak that's been with us since d96855ff517 (merge-base:
teach "--fork-point" mode, 2013-10-23).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit.c                       | 1 +
 t/t3431-rebase-fork-point.sh   | 1 +
 t/t3432-rebase-fast-forward.sh | 1 +
 3 files changed, 3 insertions(+)

diff --git a/commit.c b/commit.c
index 14538a811ae..e433c33bb01 100644
--- a/commit.c
+++ b/commit.c
@@ -1033,6 +1033,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)
 	ret = bases->item;
 
 cleanup_return:
+	free(revs.commit);
 	free_commit_list(bases);
 	free(full_refname);
 	return ret;
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 70e81363569..4bfc779bb87 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -8,6 +8,7 @@ test_description='git rebase --fork-point test'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # A---B---D---E    (main)
diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh
index 5086e14c022..7f1a5dd3deb 100755
--- a/t/t3432-rebase-fast-forward.sh
+++ b/t/t3432-rebase-fast-forward.sh
@@ -8,6 +8,7 @@ test_description='ensure rebase fast-forwards commits when possible'
 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 '
-- 
2.39.1.1425.ge02fe682bd8

Re: [PATCH v4 0/8] sequencer API & users: fix widespread leaks

[Phillip Wood]

Hi Ævar

On 06/02/2023 19:08, Ævar Arnfjörð Bjarmason wrote:
> 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 v3:
> 
> * Rebased for newer "master", there were some conflicts due to
>    adjacent changes.
> * Addressed Phillip's commit message comments (hopefully).

You have! Thanks for the re-roll this all looks good to me now

Best Wishes

Phillip

> Branch & CI for this at:
> https://github.com/avar/git/tree/avar/leak-fixes-sequencer-rebase-4
> 
> Ævar Arnfjörð Bjarmason (8):
>    rebase: use "cleanup" pattern in do_interactive_rebase()
>    sequencer.c: split up sequencer_remove_state()
>    sequencer API users: fix get_replay_opts() leaks
>    builtin/revert.c: move free-ing of "revs" to replay_opts_release()
>    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                       | 22 ++++++++------
>   builtin/revert.c                       |  8 ++---
>   commit.c                               |  1 +
>   sequencer.c                            | 42 ++++++++++++++++----------
>   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, 61 insertions(+), 33 deletions(-)
> 
> Range-diff against v3:
> 1:  b223429df33 ! 1:  029fc5f4b8c rebase: use "cleanup" pattern in do_interactive_rebase()
>      @@ Commit message
>           Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>        ## builtin/rebase.c ##
>      -@@ builtin/rebase.c: static void split_exec_commands(const char *cmd, struct string_list *commands)
>      +@@ builtin/rebase.c: static int init_basic_state(struct replay_opts *opts, const char *head_name,
>        
>        static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>        {
>      @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts,
>        	}
>        
>       +cleanup:
>      - 	string_list_clear(&commands, 0);
>        	free(revisions);
>        	free(shortrevisions);
>      + 	todo_list_release(&todo_list);
> 2:  00c7f04363f = 2:  b0c9da95ca1 sequencer.c: split up sequencer_remove_state()
> 3:  e4a96898a68 ! 3:  dbac0501424 rebase & sequencer API: fix get_replay_opts() leak in "rebase"
>      @@ Metadata
>       Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>        ## Commit message ##
>      -    rebase & sequencer API: fix get_replay_opts() leak in "rebase"
>      +    sequencer API users: fix get_replay_opts() leaks
>       
>      -    Make the recently added replay_opts_release() function non-static and
>      -    use it for freeing the "struct replay_opts" constructed by the
>      -    get_replay_opts() function in "builtin/rebase.c". See [1] for the
>      -    initial addition of get_replay_opts().
>      +    Make the replay_opts_release() function added in the preceding commit
>      +    non-static, and use it for freeing the "struct replay_opts"
>      +    constructed for "rebase" and "revert".
>       
>           To safely call our new replay_opts_release() we'll need to stop
>           calling it in sequencer_remove_state(), and instead call it where we
>      @@ Commit message
>           previously called sequencer_remove_state() would be a hassle.
>       
>           Using a FREE_AND_NULL() pattern would also work, as it would be safe
>      -    replay_opts_release() repeatedly, but let's fix this properly instead,
>      -    by having the owner of the data free() it.
>      -
>      -    See [2] for the initial implementation of "sequencer_remove_state()",
>      -    which assumed that it should be removing the full (including on-disk)
>      -    rebase state as a one-off.
>      -
>      -    1. 73fdc535d26 (rebase -i: use struct rebase_options to parse args,
>      -       2019-04-17)
>      -    2. 26ae337be11 (revert: Introduce --reset to remove sequencer state,
>      -       2011-08-04)
>      +    to call replay_opts_release() repeatedly. But let's fix this properly
>      +    instead, by having the owner of the data free() it.
>       
>           Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>      @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts,
>        
>        cleanup:
>       +	replay_opts_release(&replay);
>      - 	string_list_clear(&commands, 0);
>        	free(revisions);
>        	free(shortrevisions);
>      + 	todo_list_release(&todo_list);
>       @@ builtin/rebase.c: static int run_sequencer_rebase(struct rebase_options *opts)
>        		struct replay_opts replay_opts = get_replay_opts(opts);
>        
> 4:  9f72cc6e46b = 4:  6b29d7d00c2 builtin/revert.c: move free-ing of "revs" to replay_opts_release()
> 5:  3d5c3152f69 ! 5:  f9c4d17fe70 builtin/rebase.c: fix "options.onto_name" leak
>      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
>        	strbuf_release(&options.git_format_patch_opt);
>        	free(squash_onto_name);
>       +	free(keep_base_onto_name);
>      - 	string_list_clear(&exec, 0);
>        	string_list_clear(&strategy_options, 0);
>        	return !!ret;
>      + }
>       
>        ## t/t3416-rebase-onto-threedots.sh ##
>       @@ t/t3416-rebase-onto-threedots.sh: test_description='git rebase --onto A...B'
> 6:  c07dc006c6d = 6:  5c2870ed2e6 sequencer.c: always free() the "msgbuf" in do_pick_commit()
> 7:  ee8262ab22a ! 7:  07ab875c3e2 builtin/rebase.c: free() "options.strategy_opts"
>      @@ Commit message
>        ## builtin/rebase.c ##
>       @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>        	free(options.gpg_sign_opt);
>      - 	free(options.cmd);
>      + 	string_list_clear(&options.exec, 0);
>        	free(options.strategy);
>       +	free(options.strategy_opts);
>        	strbuf_release(&options.git_format_patch_opt);
> 8:  84343ea6bf6 = 8:  6ab2edcc135 commit.c: free() revs.commit in get_fork_point()

[PATCH v3 8/8] commit.c: free() revs.commit in get_fork_point()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak that's been with us since d96855ff517 (merge-base:
teach "--fork-point" mode, 2013-10-23).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 commit.c                       | 1 +
 t/t3431-rebase-fork-point.sh   | 1 +
 t/t3432-rebase-fast-forward.sh | 1 +
 3 files changed, 3 insertions(+)

diff --git a/commit.c b/commit.c
index 14538a811ae..e433c33bb01 100644
--- a/commit.c
+++ b/commit.c
@@ -1033,6 +1033,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)
 	ret = bases->item;
 
 cleanup_return:
+	free(revs.commit);
 	free_commit_list(bases);
 	free(full_refname);
 	return ret;
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 70e81363569..4bfc779bb87 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -8,6 +8,7 @@ test_description='git rebase --fork-point test'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # A---B---D---E    (main)
diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh
index 5086e14c022..7f1a5dd3deb 100755
--- a/t/t3432-rebase-fast-forward.sh
+++ b/t/t3432-rebase-fast-forward.sh
@@ -8,6 +8,7 @@ test_description='ensure rebase fast-forwards commits when possible'
 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 '
-- 
2.39.0.1225.g30a3d88132d

Re: [PATCH v3 0/8] sequencer API & users: fix widespread leaks

[Phillip Wood]

Hi Ævar

On 18/01/2023 16:09, Ævar Arnfjörð Bjarmason wrote:
> 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 v2:
> 
>   * Reword/amend commit messages for some of the functional changes in
>     v1..v2.
>   * Remove leftover "opts->xopts_nr = 0" now that we don't
>     FREE_AND_NULL() anymore.
>   * Drop the "squash_onto_name" refactoring to a "to_free"
>   * Instead add a new "keep_base_onto_name" in the next commit. I'd
>     missed that if both options were provided we'd die(), so that
>     free() wasn't needed before re-assignment, as Phillip pointed
>     out...

This version looks good. The commit message for patch 3 is a bit 
rambling and the commit citation is not entirely accurate but I'm not 
sure it is worth a re-roll just for that.

Thanks for working on this

Phillip


> CI & branch for this available at:
> https://github.com/avar/git/tree/avar/leak-fixes-sequencer-rebase-3
> 
> Ævar Arnfjörð Bjarmason (8):
>    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: 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                       | 22 ++++++++------
>   builtin/revert.c                       |  8 ++---
>   commit.c                               |  1 +
>   sequencer.c                            | 42 ++++++++++++++++----------
>   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, 61 insertions(+), 33 deletions(-)
> 
> Range-diff against v2:
>   1:  d0a0524f3d4 =  1:  b223429df33 rebase: use "cleanup" pattern in do_interactive_rebase()
>   2:  c4eaa8dfef4 =  2:  00c7f04363f sequencer.c: split up sequencer_remove_state()
>   3:  f06f565ceaf !  3:  e4a96898a68 rebase & sequencer API: fix get_replay_opts() leak in "rebase"
>      @@ Commit message
>           get_replay_opts() function in "builtin/rebase.c". See [1] for the
>           initial addition of get_replay_opts().
>       
>      -    To safely call our new replay_opts_release() we'll need to change all
>      -    the free() to a FREE_AND_NULL(), and set "xopts_nr" to "0" after we
>      -    loop over it and free() it (the free() in the loop doesn't need to be
>      -    a FREE_AND_NULL()).
>      +    To safely call our new replay_opts_release() we'll need to stop
>      +    calling it in sequencer_remove_state(), and instead call it where we
>      +    allocate the "struct replay_opts" itself.
>       
>           This is because in e.g. do_interactive_rebase() we construct a "struct
>           replay_opts" with "get_replay_opts()", and then call
>      @@ Commit message
>       
>           But if we encounter errors anywhere along the way we'd punt out early,
>           and not free() the memory we allocated. Remembering whether we
>      -    previously called sequencer_remove_state() would be a hassle, so let's
>      -    make it safe to re-invoke replay_opts_release() instead.
>      +    previously called sequencer_remove_state() would be a hassle.
>       
>      -    I experimented with a change to be more paranoid instead, i.e. to
>      -    exhaustively check our state via an enum. We could make sure that we:
>      -
>      -    - Only allow calling "replay_opts_release()" after
>      -      "sequencer_remove_state()", but not the other way around.
>      -
>      -    - Forbid invoking either function twice in a row.
>      -
>      -    But such paranoia isn't warranted here, let's instead take the easy
>      -    way out and FREE_AND_NULL() this.
>      +    Using a FREE_AND_NULL() pattern would also work, as it would be safe
>      +    replay_opts_release() repeatedly, but let's fix this properly instead,
>      +    by having the owner of the data free() it.
>       
>           See [2] for the initial implementation of "sequencer_remove_state()",
>           which assumed that it should be removing the full (including on-disk)
>      @@ sequencer.c: static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
>        {
>        	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]);
>      -+	opts->xopts_nr = 0;
>      - 	free(opts->xopts);
>      - 	strbuf_release(&opts->current_fixups);
>      - }
>       @@ sequencer.c: int sequencer_remove_state(struct replay_opts *opts)
>        		}
>        	}
>   4:  e83bdfab046 !  4:  9f72cc6e46b builtin/revert.c: move free-ing of "revs" to replay_opts_release()
>      @@ Commit message
>           rather than having these users reach into the struct to free its
>           individual members.
>       
>      -    As explained in earlier change we should be using FREE_AND_NULL() in
>      -    replay_opts_release() rather than free().
>      -
>           1. d1ec656d68f (cherry-pick: free "struct replay_opts" members,
>              2022-11-08)
>           2. fd74ac95ac3 (revert: free "struct replay_opts" members, 2022-07-01)
>      @@ builtin/revert.c: int cmd_cherry_pick(int argc, const char **argv, const char *p
>       
>        ## sequencer.c ##
>       @@ sequencer.c: void replay_opts_release(struct replay_opts *opts)
>      - 	opts->xopts_nr = 0;
>      + 		free(opts->xopts[i]);
>        	free(opts->xopts);
>        	strbuf_release(&opts->current_fixups);
>       +	if (opts->revs)
>   5:  4fea2b77c6d <  -:  ----------- builtin/rebase.c: rename "squash_onto_name" to "to_free"
>   6:  898bb7698fc !  5:  3d5c3152f69 builtin/rebase.c: fix "options.onto_name" leak
>      @@ Metadata
>        ## Commit message ##
>           builtin/rebase.c: fix "options.onto_name" leak
>       
>      -    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 value before
>      -    re-assigning to "options.onto_name". Let's do that, and fix the memory
>      -    leak.
>      +    Similar to the existing "squash_onto_name" added in [1] we need to
>      +    free() the xstrdup()'d "options.onto.name" added for "--keep-base" in
>      +    [2]..
>       
>           1. 9dba809a69a (builtin rebase: support --root, 2018-09-04)
>           2. 414d924beb4 (rebase: teach rebase --keep-base, 2019-08-27)
>      @@ Commit message
>           Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>       
>        ## builtin/rebase.c ##
>      +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>      + 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
>      + 	struct object_id squash_onto;
>      + 	char *squash_onto_name = NULL;
>      ++	char *keep_base_onto_name = NULL;
>      + 	int reschedule_failed_exec = -1;
>      + 	int allow_preemptive_ff = 1;
>      + 	int preserve_merges_selected = 0;
>       @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>        		strbuf_addstr(&buf, options.upstream_name);
>        		strbuf_addstr(&buf, "...");
>        		strbuf_addstr(&buf, branch_name);
>       -		options.onto_name = xstrdup(buf.buf);
>      -+		free(to_free);
>      -+		options.onto_name = to_free = xstrdup(buf.buf);
>      ++		options.onto_name = keep_base_onto_name = xstrdup(buf.buf);
>        	} else if (!options.onto_name)
>        		options.onto_name = options.upstream_name;
>        	if (strstr(options.onto_name, "...")) {
>      +@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
>      + 	free(options.strategy);
>      + 	strbuf_release(&options.git_format_patch_opt);
>      + 	free(squash_onto_name);
>      ++	free(keep_base_onto_name);
>      + 	string_list_clear(&exec, 0);
>      + 	string_list_clear(&strategy_options, 0);
>      + 	return !!ret;
>       
>        ## t/t3416-rebase-onto-threedots.sh ##
>       @@ t/t3416-rebase-onto-threedots.sh: test_description='git rebase --onto A...B'
>   7:  fb38dc873f9 =  6:  c07dc006c6d sequencer.c: always free() the "msgbuf" in do_pick_commit()
>   8:  d4b0e2a5c83 !  7:  ee8262ab22a 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(to_free);
>      - 	string_list_clear(&exec, 0);
>      + 	free(squash_onto_name);
>      + 	free(keep_base_onto_name);
>   9:  fd9c7a5547c =  8:  84343ea6bf6 commit.c: free() revs.commit in get_fork_point()