From: Phillip Wood Date: Tue, 08 Nov 2022 15:00:55 GMT Subject: Re: [PATCH 10/17] sequencer.c: fix "opts->strategy" leak in read_strategy_opts() Message-ID: <66835bfb-2815-4321-9d06-52f85a0c72f9@dunelm.org.uk> In-Reply-To: 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 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)) {