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

On Mon, Oct 04 2021, Phillip Wood wrote:

Hi Ævar

On 04/10/2021 01:46, Ævar Arnfjörð Bjarmason wrote:

quoted

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.

quoted

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
  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,