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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 22, 2021, 09:02 UTC
Message-ID: <d1ef45c1-067e-abde-62a2-1df2c12ba3a3@gmail.com>
In-Reply-To: <CABPp-BEQkUQLt-ZbwdO+ecd2rumttBUKUmh3=7LaKRxwXCkB+g@mail.gmail.com>

Hi Elijah

On 21/06/2021 22:39, Elijah Newren wrote:

Show 64 quoted lines

> On Sun, Jun 20, 2021 at 11:29 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> 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.
> 
> 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...

It's not immediately obvious but I think those are actually safe. opts->default_strategy is allocated by sequencer_init_config() so it is correct to free it and when we assign it in rebase.c we do

	else if (!replay.strategy && replay.default_strategy) {
		replay.strategy = replay.default_strategy;
		replay.default_strategy = NULL;
	}

so there is no double free. There is similar code in builtin/revert.c which I think is where your other example came from. I think there is a leak in builtin/revert.c though

	if (!opts->strategy && opts->default_strategy) {
		opts->strategy = opts->default_strategy;
		opts->default_strategy = NULL;
	}

	/* do some other stuff */

	/* These option values will be free()d */
	opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
	opts->strategy = xstrdup_or_null(opts->strategy);

So we copy the default strategy, leaking the original copy from sequencer_init_options() if --strategy isn't given on the command line. I think it would be simple to fix this by making the copy earlier.

	if (!opts->strategy && opts->default_strategy) {
		opts->strategy = opts->default_strategy;
		opts->default_strategy = NULL;
	} else if (opts->strategy) {
	/* This option will be free()d in sequencer_remove_state() */
		opts->strategy = xstrdup(opts->strategy);
	}

I'm going offline for a week or so in a couple of days but I'll have look at making a proper patch when I get back.

Best Wishes

Phillip