Re: [PATCH 11/12] builtin/rebase: fix options.strategy memory lifecycle

>
> Hi Andrzej
>
> Thanks for working on removing memory leaks from git.
>
> On 20/06/2021 16:12, andrzej@ahunt.org wrote:
> > From: Andrzej Hunt <ajrhunt@google.com>
> >
> > 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.