From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 13 Jan 2023 10:34:34 GMT
Subject: Re: [PATCH v2 3/9] rebase & sequencer API: fix get_replay_opts() leak in "rebase"
Message-ID: <44d98c85-d1d7-ed01-20e7-744324e36b13@dunelm.org.uk>
In-Reply-To: <patch-v2-3.9-f06f565ceaf-20230112T124201Z-avarab@gmail.com>

Hi Ævar

On 12/01/2023 12:45, Ævar Arnfjörð Bjarmason wrote:
> Make the recently added replay_opts_release() function non-static and
> use it for freeing the "struct replay_opts" constructed by the
> get_replay_opts() function in "builtin/rebase.c". See [1] for the
> initial addition of get_replay_opts().
> 
> To safely call our new replay_opts_release() we'll need to change all
> the free() to a FREE_AND_NULL(), and set "xopts_nr" to "0" after we
> loop over it and free() it (the free() in the loop doesn't need to be
> a FREE_AND_NULL()).
> 
> This is because in e.g. do_interactive_rebase() we construct a "struct
> replay_opts" with "get_replay_opts()", and then call
> "complete_action()". If we get far enough in that function without
> encountering errors we'll call "pick_commits()" which (indirectly)
> calls sequencer_remove_state() at the end.
> 
> But if we encounter errors anywhere along the way we'd punt out early,
> and not free() the memory we allocated. Remembering whether we
> previously called sequencer_remove_state() would be a hassle, so let's
> make it safe to re-invoke replay_opts_release() instead.
> 
> I experimented with a change to be more paranoid instead, i.e. to
> exhaustively check our state via an enum. We could make sure that we:
> 
> - Only allow calling "replay_opts_release()" after
>    "sequencer_remove_state()", but not the other way around.
> 
> - Forbid invoking either function twice in a row.
> 
> But such paranoia isn't warranted here, let's instead take the easy
> way out and FREE_AND_NULL() this.

The changes below look good, but this message needs updating to reflect 
the re-roll.

> @@ -359,6 +359,7 @@ static void replay_opts_release(struct replay_opts *opts)
>   	free(opts->strategy);
>   	for (size_t i = 0; i < opts->xopts_nr; i++)
>   		free(opts->xopts[i]);
> +	opts->xopts_nr = 0;

I don't think we need this now we're only calling replay_opts_release() 
once.

Best Wishes

Phillip