Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: phillip.wood@dunelm.org.uk
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Andrzej Hunt" <ajrhunt@google.com>, "Jeff King" <peff@peff.net>
Subject: Re: [PATCH 04/10] unpack-trees API: don't have clear_unpack_trees_porcelain() reset
Date: Mon, 04 Oct 2021 13:12:16 +0200	[thread overview]
Message-ID: <87bl45niqs.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <f24733e9-8742-33b6-5fae-93d89698cc00@gmail.com>

On Mon, Oct 04 2021, Phillip Wood wrote:

> Hi Ævar
>
> On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:
>> Change the clear_unpack_trees_porcelain() to be like a *_release()
>> function, not a *_reset() (in strbuf.c terms). Let's move the only API
>> user that relied on the latter to doing its own
>> unpack_trees_options_init(). See the commit that introduced
>> unpack_trees_options_init() for details on the control flow involved
>> here.
>
> Before this change if there was a call to unpack_trees() after
> clear_unpack_trees_porcelain() then that caller would get the default 
> error messages. After this change we end up with a use-after-free
> error in that situation. I found the subject line of this patch hard
> to understand, the commit message explains what it is doing but I'm
> still not sure what the motivation for this change is.

I'll work on the commit message part.

With this series such a caller is purely hypothetical, isn't it?
I.e. the journey in 02/10 & 04/10, and later in the 07/10 you commented
on is to make the API behave similarly to e.g. strbuf, where there's a
release() that leaves it in such a state, different from a "reset".

Perhaps this step in isolation is confusing at it leaves the function
named as clear_unpack_trees_porcelain(). I had this all in one change
initially, but figured having such a large rename diff & one behavior
change was worse.

We could just leave the "reset" semantics in place for everyone, but
just like "strbuf_release()" et al I think it's good for
self-documentation purposes to explicitly make clear if you're re-using
the struct, or just freeing it at the end.

For this API only one user of the API cares about the re-use case,
merge-recursive.c.

>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   merge-recursive.c | 1 +
>>   unpack-trees.c    | 1 -
>>   2 files changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index d24a4903f1d..a77f66b006c 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -442,6 +442,7 @@ static void unpack_trees_finish(struct merge_options *opt)
>>   {
>>   	discard_index(&opt->priv->orig_index);
>>   	clear_unpack_trees_porcelain(&opt->priv->unpack_opts);
>> +	unpack_trees_options_init(&opt->priv->unpack_opts);
>>   }
>>     static int save_files_dirs(const struct object_id *oid,
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index 94767d3f96f..e7365322e82 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -197,7 +197,6 @@ void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
>>   {
>>   	strvec_clear(&opts->msgs_to_free);
>>   	dir_clear(&opts->dir);
>> -	memset(opts->msgs, 0, sizeof(opts->msgs));
>>   }
>>     static int do_add_entry(struct unpack_trees_options *o, struct
>> cache_entry *ce,
>> 