On 04/11/2022 14:50, Phillip Wood wrote:
On 03/11/2022 17:06, Ævar Arnfjörð Bjarmason wrote:
When "read_strategy_opts()" is called we may have populated the
"opts->strategy" before, so we'll need to free() it to avoid leaking
memory.
Where is the previous value coming from? I guess it may be the config
but otherwise I'm confused.
Having looked a bit more at this I think this is the wrong fix. The
reason we're overwriting the existing value is that we're reading some
of the state files twice. I think the only way to get to this point is
to go through a code path that calls rebase.c:read_basic_state() which
already populates these options via a later call to
rebase.c:get_replay_opts(). I think the correct fixes looks something
like the diff below.
I have also looked at the cherry-pick/revert case and I think that we're
leaking opts->strategy (and probably some others) when running
git cherry-pick --continue
after
git -c pull.twohead=recursive cherry-pick -s ort <some commits>
I'm not sure what your strategy has been with the fixes in this series
but we're never going to have 100% coverage of all the option
combinations for rebase & cherry-pick so I think it is helpful to treat
these LSAN reports as a starting point for looking into why the leak is
occurring and also look for similar leaks.
Best Wishes
Phillip
--- >8 ---
sequencer.c | 46 ----------------------------------------------
1 file changed, 46 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index e658df7e8f..ece34dce9f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2889,55 +2889,12 @@ void parse_strategy_opts(struct replay_opts
*opts, char *raw_opts)
}
}
-static void read_strategy_opts(struct replay_opts *opts, struct strbuf
*buf)
-{
- strbuf_reset(buf);
- if (!read_oneliner(buf, rebase_path_strategy(), 0))
- return;
- opts->strategy = strbuf_detach(buf, NULL);
- if (!read_oneliner(buf, rebase_path_strategy_opts(), 0))
- return;
-
- parse_strategy_opts(opts, buf->buf);
-}
-
static int read_populate_opts(struct replay_opts *opts)
{
if (is_rebase_i(opts)) {
struct strbuf buf = STRBUF_INIT;
int ret = 0;
- if (read_oneliner(&buf, rebase_path_gpg_sign_opt(),
- READ_ONELINER_SKIP_IF_EMPTY)) {
- if (!starts_with(buf.buf, "-S"))
- strbuf_reset(&buf);
- else {
- free(opts->gpg_sign);
- opts->gpg_sign = xstrdup(buf.buf + 2);
- }
- strbuf_reset(&buf);
- }
-
- if (read_oneliner(&buf,
rebase_path_allow_rerere_autoupdate(),
- READ_ONELINER_SKIP_IF_EMPTY)) {
- if (!strcmp(buf.buf, "--rerere-autoupdate"))
- opts->allow_rerere_auto = RERERE_AUTOUPDATE;
- else if (!strcmp(buf.buf, "--no-rerere-autoupdate"))
- opts->allow_rerere_auto =
RERERE_NOAUTOUPDATE;
- strbuf_reset(&buf);
- }
-
- if (file_exists(rebase_path_verbose()))
- opts->verbose = 1;
-
- if (file_exists(rebase_path_quiet()))
- opts->quiet = 1;
-
- if (file_exists(rebase_path_signoff())) {
- opts->allow_ff = 0;
- opts->signoff = 1;
- }
-
if (file_exists(rebase_path_cdate_is_adate())) {
opts->allow_ff = 0;
opts->committer_date_is_author_date = 1;
@@ -2959,9 +2916,6 @@ static int read_populate_opts(struct replay_opts
*opts)
if (file_exists(rebase_path_keep_redundant_commits()))
opts->keep_redundant_commits = 1;
- read_strategy_opts(opts, &buf);
- strbuf_reset(&buf);
-
if (read_oneliner(&opts->current_fixups,
rebase_path_current_fixups(),
READ_ONELINER_SKIP_IF_EMPTY)) {
