Re: [PATCH 10/17] sequencer.c: fix "opts->strategy" leak in read_strategy_opts()

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 8, 2022, 15:00 UTC
Message-ID: <66835bfb-2815-4321-9d06-52f85a0c72f9@dunelm.org.uk>
In-Reply-To: <b9f08244-36bc-b74f-ac1a-b665423bc2e7@dunelm.org.uk>

On 04/11/2022 14:50, Phillip Wood wrote:

Show 7 quoted lines

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