git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Jan 24, 2023, 14:36 UTC
Message-ID
<e09ca80a-f949-5bdc-f709-73025d1dd3f1@dunelm.org.uk>
In-Reply-To
<patch-v3-3.8-e4a96898a68-20230118T160600Z-avarab@gmail.com>
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.

Show 16 quoted lines
> 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
Show 299 quoted lines
> 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
>   
>
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 43 of 60 in “sequencer API & users: fix widespread leaks”
  1. 00/10 sequencer API & users: fix widespread leaksÆvar Arnfjörð Bjarmason, Dec 30, 2022
  2. 01/10 rebase: use "cleanup" pattern in do_interactive_rebase()Ævar Arnfjörð Bjarmason, Dec 30, 2022
  3. Phillip WoodDec 31, 2022
  4. 02/10 sequencer.c: split up sequencer_remove_state()Ævar Arnfjörð Bjarmason, Dec 30, 2022
  5. René ScharfeDec 30, 2022
  6. Phillip WoodDec 31, 2022
  7. 03/10 rebase & sequencer API: fix get_replay_opts() leak in "rebase"Ævar Arnfjörð Bjarmason, Dec 30, 2022
  8. Phillip WoodDec 31, 2022
  9. 04/10 builtin/revert.c: refactor run_sequencer() return patternÆvar Arnfjörð Bjarmason, Dec 30, 2022
  10. Junio C HamanoJan 1, 2023
  11. 05/10 builtin/revert.c: fix common leak by using replay_opts_release()Ævar Arnfjörð Bjarmason, Dec 30, 2022
  12. René ScharfeDec 30, 2022
  13. Phillip WoodDec 31, 2022
  14. 06/10 builtin/revert.c: move free-ing of "revs" to replay_opts_release()Ævar Arnfjörð Bjarmason, Dec 30, 2022
  15. 07/10 builtin/rebase.c: fix "options.onto_name" leakÆvar Arnfjörð Bjarmason, Dec 30, 2022
  16. Phillip WoodDec 31, 2022
  17. 08/10 sequencer.c: always free() the "msgbuf" in do_pick_commit()Ævar Arnfjörð Bjarmason, Dec 30, 2022
  18. Phillip WoodDec 31, 2022
  19. 09/10 builtin/rebase.c: free() "options.strategy_opts"Ævar Arnfjörð Bjarmason, Dec 30, 2022
  20. 10/10 commit.c: free() revs.commit in get_fork_point()Ævar Arnfjörð Bjarmason, Dec 30, 2022
  21. Phillip WoodDec 31, 2022
  22. Junio C HamanoJan 1, 2023
  23. 0/9 sequencer API & users: fix widespread leaksÆvar Arnfjörð Bjarmason, Jan 12, 2023
  24. 1/9 rebase: use "cleanup" pattern in do_interactive_rebase()Ævar Arnfjörð Bjarmason, Jan 12, 2023
  25. 2/9 sequencer.c: split up sequencer_remove_state()Ævar Arnfjörð Bjarmason, Jan 12, 2023
  26. 4/9 builtin/revert.c: move free-ing of "revs" to replay_opts_release()Ævar Arnfjörð Bjarmason, Jan 12, 2023
  27. Phillip WoodJan 13, 2023
  28. 3/9 rebase & sequencer API: fix get_replay_opts() leak in "rebase"Ævar Arnfjörð Bjarmason, Jan 12, 2023
  29. Phillip WoodJan 13, 2023
  30. 6/9 builtin/rebase.c: fix "options.onto_name" leakÆvar Arnfjörð Bjarmason, Jan 12, 2023
  31. Phillip WoodJan 13, 2023
  32. 5/9 builtin/rebase.c: rename "squash_onto_name" to "to_free"Ævar Arnfjörð Bjarmason, Jan 12, 2023
  33. Phillip WoodJan 13, 2023
  34. 7/9 sequencer.c: always free() the "msgbuf" in do_pick_commit()Ævar Arnfjörð Bjarmason, Jan 12, 2023
  35. 8/9 builtin/rebase.c: free() "options.strategy_opts"Ævar Arnfjörð Bjarmason, Jan 12, 2023
  36. 9/9 commit.c: free() revs.commit in get_fork_point()Ævar Arnfjörð Bjarmason, Jan 12, 2023
  37. Phillip WoodJan 13, 2023
  38. Junio C HamanoJan 13, 2023
  39. 0/8 sequencer API & users: fix widespread leaksÆvar Arnfjörð Bjarmason, Jan 18, 2023
  40. 1/8 rebase: use "cleanup" pattern in do_interactive_rebase()Ævar Arnfjörð Bjarmason, Jan 18, 2023
  41. 2/8 sequencer.c: split up sequencer_remove_state()Ævar Arnfjörð Bjarmason, Jan 18, 2023
  42. 3/8 rebase & sequencer API: fix get_replay_opts() leak in "rebase"Ævar Arnfjörð Bjarmason, Jan 18, 2023
  43. Phillip WoodJan 24, 2023
  44. 4/8 builtin/revert.c: move free-ing of "revs" to replay_opts_release()Ævar Arnfjörð Bjarmason, Jan 18, 2023
  45. 6/8 sequencer.c: always free() the "msgbuf" in do_pick_commit()Ævar Arnfjörð Bjarmason, Jan 18, 2023
  46. 8/8 commit.c: free() revs.commit in get_fork_point()Ævar Arnfjörð Bjarmason, Jan 18, 2023
  47. 7/8 builtin/rebase.c: free() "options.strategy_opts"Ævar Arnfjörð Bjarmason, Jan 18, 2023
  48. 0/8 sequencer API & users: fix widespread leaksÆvar Arnfjörð Bjarmason, Feb 6, 2023
  49. 1/8 rebase: use "cleanup" pattern in do_interactive_rebase()Ævar Arnfjörð Bjarmason, Feb 6, 2023
  50. 2/8 sequencer.c: split up sequencer_remove_state()Ævar Arnfjörð Bjarmason, Feb 6, 2023
  51. 3/8 sequencer API users: fix get_replay_opts() leaksÆvar Arnfjörð Bjarmason, Feb 6, 2023
  52. 4/8 builtin/revert.c: move free-ing of "revs" to replay_opts_release()Ævar Arnfjörð Bjarmason, Feb 6, 2023
  53. 5/8 builtin/rebase.c: fix "options.onto_name" leakÆvar Arnfjörð Bjarmason, Feb 6, 2023
  54. 7/8 builtin/rebase.c: free() "options.strategy_opts"Ævar Arnfjörð Bjarmason, Feb 6, 2023
  55. 6/8 sequencer.c: always free() the "msgbuf" in do_pick_commit()Ævar Arnfjörð Bjarmason, Feb 6, 2023
  56. 8/8 commit.c: free() revs.commit in get_fork_point()Ævar Arnfjörð Bjarmason, Feb 6, 2023
  57. Phillip WoodFeb 7, 2023
  58. 5/8 builtin/rebase.c: fix "options.onto_name" leakÆvar Arnfjörð Bjarmason, Jan 18, 2023
  59. Phillip WoodJan 24, 2023
  60. Phillip WoodJan 24, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.