* [PATCH v2 1/9] rebase: use "cleanup" pattern in do_interactive_rebase()
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
@ 2023-01-12 12:45 ` Ævar Arnfjörð Bjarmason
2023-01-12 12:45 ` [PATCH v2 2/9] sequencer.c: split up sequencer_remove_state() Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
10 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 12:45 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 2/9] sequencer.c: split up sequencer_remove_state()
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
2023-01-12 12:45 ` [PATCH v2 1/9] rebase: use "cleanup" pattern in do_interactive_rebase() Ævar Arnfjörð Bjarmason
@ 2023-01-12 12:45 ` Ævar Arnfjörð Bjarmason
2023-01-12 12:45 ` [PATCH v2 3/9] rebase & sequencer API: fix get_replay_opts() leak in "rebase" Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
10 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 12:45 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 3/9] rebase & sequencer API: fix get_replay_opts() leak in "rebase"
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
2023-01-12 12:45 ` [PATCH v2 1/9] rebase: use "cleanup" pattern in do_interactive_rebase() Ævar Arnfjörð Bjarmason
2023-01-12 12:45 ` [PATCH v2 2/9] sequencer.c: split up sequencer_remove_state() Ævar Arnfjörð Bjarmason
@ 2023-01-12 12:45 ` Ævar Arnfjörð Bjarmason
2023-01-13 10:34 ` Phillip Wood
2023-01-12 12:45 ` [PATCH v2 4/9] builtin/revert.c: move free-ing of "revs" to replay_opts_release() Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
10 siblings, 1 reply; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 12:45 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v2 3/9] rebase & sequencer API: fix get_replay_opts() leak in "rebase"
2023-01-12 12:45 ` [PATCH v2 3/9] rebase & sequencer API: fix get_replay_opts() leak in "rebase" Ævar Arnfjörð Bjarmason
@ 2023-01-13 10:34 ` Phillip Wood
0 siblings, 0 replies; 60+ messages in thread
From: Phillip Wood @ 2023-01-13 10:34 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
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
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v2 4/9] builtin/revert.c: move free-ing of "revs" to replay_opts_release()
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2023-01-12 12:45 ` [PATCH v2 3/9] rebase & sequencer API: fix get_replay_opts() leak in "rebase" Ævar Arnfjörð Bjarmason
@ 2023-01-12 12:45 ` Ævar Arnfjörð Bjarmason
2023-01-13 10:36 ` Phillip Wood
2023-01-12 12:45 ` [PATCH v2 5/9] builtin/rebase.c: rename "squash_onto_name" to "to_free" Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
10 siblings, 1 reply; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 12:45 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v2 4/9] builtin/revert.c: move free-ing of "revs" to replay_opts_release()
2023-01-12 12:45 ` [PATCH v2 4/9] builtin/revert.c: move free-ing of "revs" to replay_opts_release() Ævar Arnfjörð Bjarmason
@ 2023-01-13 10:36 ` Phillip Wood
0 siblings, 0 replies; 60+ messages in thread
From: Phillip Wood @ 2023-01-13 10:36 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
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)
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v2 5/9] builtin/rebase.c: rename "squash_onto_name" to "to_free"
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2023-01-12 12:45 ` [PATCH v2 4/9] builtin/revert.c: move free-ing of "revs" to replay_opts_release() Ævar Arnfjörð Bjarmason
@ 2023-01-12 12:45 ` Ævar Arnfjörð Bjarmason
2023-01-13 10:37 ` Phillip Wood
2023-01-12 12:45 ` [PATCH v2 6/9] builtin/rebase.c: fix "options.onto_name" leak Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
10 siblings, 1 reply; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 12:45 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v2 5/9] builtin/rebase.c: rename "squash_onto_name" to "to_free"
2023-01-12 12:45 ` [PATCH v2 5/9] builtin/rebase.c: rename "squash_onto_name" to "to_free" Ævar Arnfjörð Bjarmason
@ 2023-01-13 10:37 ` Phillip Wood
0 siblings, 0 replies; 60+ messages in thread
From: Phillip Wood @ 2023-01-13 10:37 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
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;
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v2 6/9] builtin/rebase.c: fix "options.onto_name" leak
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2023-01-12 12:45 ` [PATCH v2 5/9] builtin/rebase.c: rename "squash_onto_name" to "to_free" Ævar Arnfjörð Bjarmason
@ 2023-01-12 12:45 ` Ævar Arnfjörð Bjarmason
2023-01-13 10:41 ` Phillip Wood
2023-01-12 12:45 ` [PATCH v2 7/9] sequencer.c: always free() the "msgbuf" in do_pick_commit() Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
10 siblings, 1 reply; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 12:45 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v2 6/9] builtin/rebase.c: fix "options.onto_name" leak
2023-01-12 12:45 ` [PATCH v2 6/9] builtin/rebase.c: fix "options.onto_name" leak Ævar Arnfjörð Bjarmason
@ 2023-01-13 10:41 ` Phillip Wood
0 siblings, 0 replies; 60+ messages in thread
From: Phillip Wood @ 2023-01-13 10:41 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
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"
>
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v2 7/9] sequencer.c: always free() the "msgbuf" in do_pick_commit()
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2023-01-12 12:45 ` [PATCH v2 6/9] builtin/rebase.c: fix "options.onto_name" leak Ævar Arnfjörð Bjarmason
@ 2023-01-12 12:45 ` Ævar Arnfjörð Bjarmason
2023-01-12 12:46 ` [PATCH v2 8/9] builtin/rebase.c: free() "options.strategy_opts" Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
10 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 12:45 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 8/9] builtin/rebase.c: free() "options.strategy_opts"
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2023-01-12 12:45 ` [PATCH v2 7/9] sequencer.c: always free() the "msgbuf" in do_pick_commit() Ævar Arnfjörð Bjarmason
@ 2023-01-12 12:46 ` Ævar Arnfjörð Bjarmason
2023-01-12 12:46 ` [PATCH v2 9/9] commit.c: free() revs.commit in get_fork_point() Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
10 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 12:46 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 9/9] commit.c: free() revs.commit in get_fork_point()
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2023-01-12 12:46 ` [PATCH v2 8/9] builtin/rebase.c: free() "options.strategy_opts" Ævar Arnfjörð Bjarmason
@ 2023-01-12 12:46 ` Ævar Arnfjörð Bjarmason
2023-01-13 10:45 ` [PATCH v2 0/9] sequencer API & users: fix widespread leaks Phillip Wood
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
10 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-12 12:46 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v2 0/9] sequencer API & users: fix widespread leaks
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2023-01-12 12:46 ` [PATCH v2 9/9] commit.c: free() revs.commit in get_fork_point() Ævar Arnfjörð Bjarmason
@ 2023-01-13 10:45 ` Phillip Wood
2023-01-13 20:47 ` Junio C Hamano
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
10 siblings, 1 reply; 60+ messages in thread
From: Phillip Wood @ 2023-01-13 10:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
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()
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 0/8] sequencer API & users: fix widespread leaks
2023-01-12 12:45 ` [PATCH v2 0/9] " Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2023-01-13 10:45 ` [PATCH v2 0/9] sequencer API & users: fix widespread leaks Phillip Wood
@ 2023-01-18 16:09 ` Ævar Arnfjörð Bjarmason
2023-01-18 16:09 ` [PATCH v3 1/8] rebase: use "cleanup" pattern in do_interactive_rebase() Ævar Arnfjörð Bjarmason
` (8 more replies)
10 siblings, 9 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 1/8] rebase: use "cleanup" pattern in do_interactive_rebase()
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
@ 2023-01-18 16:09 ` Ævar Arnfjörð Bjarmason
2023-01-18 16:09 ` [PATCH v3 2/8] sequencer.c: split up sequencer_remove_state() Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v3 2/8] sequencer.c: split up sequencer_remove_state()
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
2023-01-18 16:09 ` [PATCH v3 1/8] rebase: use "cleanup" pattern in do_interactive_rebase() Ævar Arnfjörð Bjarmason
@ 2023-01-18 16:09 ` Ævar Arnfjörð Bjarmason
2023-01-18 16:09 ` [PATCH v3 3/8] rebase & sequencer API: fix get_replay_opts() leak in "rebase" Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v3 3/8] rebase & sequencer API: fix get_replay_opts() leak in "rebase"
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
2023-01-18 16:09 ` [PATCH v3 1/8] rebase: use "cleanup" pattern in do_interactive_rebase() Ævar Arnfjörð Bjarmason
2023-01-18 16:09 ` [PATCH v3 2/8] sequencer.c: split up sequencer_remove_state() Ævar Arnfjörð Bjarmason
@ 2023-01-18 16:09 ` Ævar Arnfjörð Bjarmason
2023-01-24 14:36 ` Phillip Wood
2023-01-18 16:09 ` [PATCH v3 4/8] builtin/revert.c: move free-ing of "revs" to replay_opts_release() Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
8 siblings, 1 reply; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v3 3/8] rebase & sequencer API: fix get_replay_opts() leak in "rebase"
2023-01-18 16:09 ` [PATCH v3 3/8] rebase & sequencer API: fix get_replay_opts() leak in "rebase" Ævar Arnfjörð Bjarmason
@ 2023-01-24 14:36 ` Phillip Wood
0 siblings, 0 replies; 60+ messages in thread
From: Phillip Wood @ 2023-01-24 14:36 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
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
>
>
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 4/8] builtin/revert.c: move free-ing of "revs" to replay_opts_release()
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2023-01-18 16:09 ` [PATCH v3 3/8] rebase & sequencer API: fix get_replay_opts() leak in "rebase" Ævar Arnfjörð Bjarmason
@ 2023-01-18 16:09 ` Ævar Arnfjörð Bjarmason
2023-01-18 16:09 ` [PATCH v3 5/8] builtin/rebase.c: fix "options.onto_name" leak Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v3 5/8] builtin/rebase.c: fix "options.onto_name" leak
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2023-01-18 16:09 ` [PATCH v3 4/8] builtin/revert.c: move free-ing of "revs" to replay_opts_release() Ævar Arnfjörð Bjarmason
@ 2023-01-18 16:09 ` Ævar Arnfjörð Bjarmason
2023-01-24 14:40 ` Phillip Wood
2023-01-18 16:09 ` [PATCH v3 6/8] sequencer.c: always free() the "msgbuf" in do_pick_commit() Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
8 siblings, 1 reply; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v3 5/8] builtin/rebase.c: fix "options.onto_name" leak
2023-01-18 16:09 ` [PATCH v3 5/8] builtin/rebase.c: fix "options.onto_name" leak Ævar Arnfjörð Bjarmason
@ 2023-01-24 14:40 ` Phillip Wood
0 siblings, 0 replies; 60+ messages in thread
From: Phillip Wood @ 2023-01-24 14:40 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
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"
>
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 6/8] sequencer.c: always free() the "msgbuf" in do_pick_commit()
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2023-01-18 16:09 ` [PATCH v3 5/8] builtin/rebase.c: fix "options.onto_name" leak Ævar Arnfjörð Bjarmason
@ 2023-01-18 16:09 ` Ævar Arnfjörð Bjarmason
2023-01-18 16:09 ` [PATCH v3 7/8] builtin/rebase.c: free() "options.strategy_opts" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v3 7/8] builtin/rebase.c: free() "options.strategy_opts"
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2023-01-18 16:09 ` [PATCH v3 6/8] sequencer.c: always free() the "msgbuf" in do_pick_commit() Ævar Arnfjörð Bjarmason
@ 2023-01-18 16:09 ` Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Ævar Arnfjörð Bjarmason
2023-01-18 16:09 ` [PATCH v3 8/8] commit.c: free() revs.commit in get_fork_point() Ævar Arnfjörð Bjarmason
2023-01-24 14:41 ` [PATCH v3 0/8] sequencer API & users: fix widespread leaks Phillip Wood
8 siblings, 1 reply; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 0/8] sequencer API & users: fix widespread leaks
2023-01-18 16:09 ` [PATCH v3 7/8] builtin/rebase.c: free() "options.strategy_opts" Ævar Arnfjörð Bjarmason
@ 2023-02-06 19:08 ` Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 1/8] rebase: use "cleanup" pattern in do_interactive_rebase() Ævar Arnfjörð Bjarmason
` (8 more replies)
0 siblings, 9 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-06 19:08 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 1/8] rebase: use "cleanup" pattern in do_interactive_rebase()
2023-02-06 19:08 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Ævar Arnfjörð Bjarmason
@ 2023-02-06 19:08 ` Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 2/8] sequencer.c: split up sequencer_remove_state() Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-06 19:08 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 2/8] sequencer.c: split up sequencer_remove_state()
2023-02-06 19:08 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 1/8] rebase: use "cleanup" pattern in do_interactive_rebase() Ævar Arnfjörð Bjarmason
@ 2023-02-06 19:08 ` Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 3/8] sequencer API users: fix get_replay_opts() leaks Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-06 19:08 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 3/8] sequencer API users: fix get_replay_opts() leaks
2023-02-06 19:08 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 1/8] rebase: use "cleanup" pattern in do_interactive_rebase() Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 2/8] sequencer.c: split up sequencer_remove_state() Ævar Arnfjörð Bjarmason
@ 2023-02-06 19:08 ` Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 4/8] builtin/revert.c: move free-ing of "revs" to replay_opts_release() Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-06 19:08 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 4/8] builtin/revert.c: move free-ing of "revs" to replay_opts_release()
2023-02-06 19:08 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2023-02-06 19:08 ` [PATCH v4 3/8] sequencer API users: fix get_replay_opts() leaks Ævar Arnfjörð Bjarmason
@ 2023-02-06 19:08 ` Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 5/8] builtin/rebase.c: fix "options.onto_name" leak Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-06 19:08 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 5/8] builtin/rebase.c: fix "options.onto_name" leak
2023-02-06 19:08 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2023-02-06 19:08 ` [PATCH v4 4/8] builtin/revert.c: move free-ing of "revs" to replay_opts_release() Ævar Arnfjörð Bjarmason
@ 2023-02-06 19:08 ` Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 6/8] sequencer.c: always free() the "msgbuf" in do_pick_commit() Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-06 19:08 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 6/8] sequencer.c: always free() the "msgbuf" in do_pick_commit()
2023-02-06 19:08 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2023-02-06 19:08 ` [PATCH v4 5/8] builtin/rebase.c: fix "options.onto_name" leak Ævar Arnfjörð Bjarmason
@ 2023-02-06 19:08 ` Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 7/8] builtin/rebase.c: free() "options.strategy_opts" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-06 19:08 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 7/8] builtin/rebase.c: free() "options.strategy_opts"
2023-02-06 19:08 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2023-02-06 19:08 ` [PATCH v4 6/8] sequencer.c: always free() the "msgbuf" in do_pick_commit() Ævar Arnfjörð Bjarmason
@ 2023-02-06 19:08 ` Ævar Arnfjörð Bjarmason
2023-02-06 19:08 ` [PATCH v4 8/8] commit.c: free() revs.commit in get_fork_point() Ævar Arnfjörð Bjarmason
2023-02-07 10:19 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Phillip Wood
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-06 19:08 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 8/8] commit.c: free() revs.commit in get_fork_point()
2023-02-06 19:08 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2023-02-06 19:08 ` [PATCH v4 7/8] builtin/rebase.c: free() "options.strategy_opts" Ævar Arnfjörð Bjarmason
@ 2023-02-06 19:08 ` Ævar Arnfjörð Bjarmason
2023-02-07 10:19 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Phillip Wood
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-06 19:08 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v4 0/8] sequencer API & users: fix widespread leaks
2023-02-06 19:08 ` [PATCH v4 0/8] sequencer API & users: fix widespread leaks Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2023-02-06 19:08 ` [PATCH v4 8/8] commit.c: free() revs.commit in get_fork_point() Ævar Arnfjörð Bjarmason
@ 2023-02-07 10:19 ` Phillip Wood
8 siblings, 0 replies; 60+ messages in thread
From: Phillip Wood @ 2023-02-07 10:19 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
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()
^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 8/8] commit.c: free() revs.commit in get_fork_point()
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2023-01-18 16:09 ` [PATCH v3 7/8] builtin/rebase.c: free() "options.strategy_opts" Ævar Arnfjörð Bjarmason
@ 2023-01-18 16:09 ` Ævar Arnfjörð Bjarmason
2023-01-24 14:41 ` [PATCH v3 0/8] sequencer API & users: fix widespread leaks Phillip Wood
8 siblings, 0 replies; 60+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 16:09 UTC (permalink / raw)
To: git
Cc: Phillip Wood, Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe, Æ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
^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v3 0/8] sequencer API & users: fix widespread leaks
2023-01-18 16:09 ` [PATCH v3 0/8] " Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2023-01-18 16:09 ` [PATCH v3 8/8] commit.c: free() revs.commit in get_fork_point() Ævar Arnfjörð Bjarmason
@ 2023-01-24 14:41 ` Phillip Wood
8 siblings, 0 replies; 60+ messages in thread
From: Phillip Wood @ 2023-01-24 14:41 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Johannes Schindelin, Junio C Hamano, Taylor Blau,
René Scharfe
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()
^ permalink raw reply [flat|nested] 60+ messages in thread