On Sun, Jun 20, 2021 at 11:29 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>
> Hi Andrzej
>
> Thanks for working on removing memory leaks from git.
>
> On 20/06/2021 16:12, andrzej@xxxxxxxxx wrote:
> > From: Andrzej Hunt <ajrhunt@xxxxxxxxxx>
> >
> > This change:
> > - xstrdup()'s all string being used for replace_opts.strategy, to
>
> I think you mean replay_opts rather than replace_opts.
>
> > guarantee that replace_opts owns these strings. This is needed because
> > sequencer_remove_state() will free replace_opts.strategy, and it's
> > usually called as part of the usage of replace_opts.
> > - Removes xstrdup()'s being used to populate options.strategy in
> > cmd_rebase(), which avoids leaking options.strategy, even in the
> > case where strategy is never moved/copied into replace_opts.
>
>
> > These changes are needed because:
> > - We would always create a new string for options.strategy if we either
> > get a strategy via options (OPT_STRING(...strategy...), or via
> > GIT_TEST_MERGE_ALGORITHM.
> > - But only sometimes is this string copied into replace_opts - in which
> > case it did get free()'d in sequencer_remove_state().
> > - The rest of the time, the newly allocated string would remain unused,
> > causing a leak. But we can't just add a free because that can result
> > in a double-free in those cases where replace_opts was populated.
> >
> > An alternative approach would be to set options.strategy to NULL when
> > moving the pointer to replace_opts.strategy, combined with always
> > free()'ing options.strategy, but that seems like a more
> > complicated and wasteful approach.
>
> read_basic_state() contains
> if (file_exists(state_dir_path("strategy", opts))) {
> strbuf_reset(&buf);
> if (!read_oneliner(&buf, state_dir_path("strategy", opts),
> READ_ONELINER_WARN_MISSING))
> return -1;
> free(opts->strategy);
> opts->strategy = xstrdup(buf.buf);
> }
>
> So we do try to free opts->strategy when reading the state from disc and
> we allocate a new string. I suspect that opts->strategy is actually NULL
> in when this function is called but I haven't checked. Given that we are
> allocating a copy above I think maybe your alternative approach of
> always freeing opts->strategy would be better.
Good catches. sequencer_remove_state() in sequencer.c also has a
free(opts->strategy) call.
To make things even more muddy, we have code like
replay.strategy = replay.default_strategy;
or
opts->strategy = opts->default_strategy;
which both will probably work really poorly with the calls to
free(opts->default_strategy);
free(opts->strategy);
from sequencer_remove_state(). I suspect we've got a few bugs here...
