Re: [PATCH v2 3/3] unpack_trees_options: free messages when done

[Martin Ågren]

On 18 May 2018 at 00:10, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> The `opts` string array contains multiple copies of the same pointers.
>> Be careful to only free each pointer once, then zeroize the whole array
>> so that we do not leave any dangling pointers.

> I wonder if an approach that is longer-term a bit more maintainable
> is to add a new string-list instance to opts, save these xstrfmt()'ed
> messages to it when setup_unpack_trees_porcelain() create them, and
> then make clear_unpack_trees_porcelain() pay *no* attention to msg[]
> array and the positions of these allocated messages and duplicates
> but just reclaim the resources held in that string-list, or
> something like that.

Thank you for thoughts and this suggestion. I will try this out,
hopefully later today.

Martin

Re: [PATCH v2 3/3] unpack_trees_options: free messages when done

[Junio C Hamano]

Martin Ågren  writes:

> The strings allocated in `setup_unpack_trees_porcelain()` are never
> freed. Provide a function `clear_unpack_trees_porcelain()` to do so and
> call it where we use `setup_unpack_trees_porcelain()`. The only
> non-trivial user is `unpack_trees_start()`, where we should place the
> new call in `unpack_trees_finish()`.
>
> The `opts` string array contains multiple copies of the same pointers.
> Be careful to only free each pointer once, then zeroize the whole array
> so that we do not leave any dangling pointers.

The verb to make it zero or fill it with zero is "to zero", I would
think.

To be honest I am not sure if I like the way this change is done.
The clear_unpack_trees_porcelain() function has too intimate
knowledge of what happens inside the setup_unpack_trees_porcelain()
function; it not just knows which fields are always allocated but
which are duplicates, which must be double checked for updates
whenever the latter gets modified, yet there is no large warning
sign painted in red in the latter, so it is easy to change the
latter and invalidate the assumption the former makes by mistake,
leading to new leaks and/or double freeing.

I wonder if an approach that is longer-term a bit more maintainable
is to add a new string-list instance to opts, save these xstrfmt()'ed
messages to it when setup_unpack_trees_porcelain() create them, and
then make clear_unpack_trees_porcelain() pay *no* attention to msg[]
array and the positions of these allocated messages and duplicates
but just reclaim the resources held in that string-list, or
something like that.

[PATCH v2 3/3] unpack_trees_options: free messages when done

[Martin Ågren]

The strings allocated in `setup_unpack_trees_porcelain()` are never
freed. Provide a function `clear_unpack_trees_porcelain()` to do so and
call it where we use `setup_unpack_trees_porcelain()`. The only
non-trivial user is `unpack_trees_start()`, where we should place the
new call in `unpack_trees_finish()`.

The `opts` string array contains multiple copies of the same pointers.
Be careful to only free each pointer once, then zeroize the whole array
so that we do not leave any dangling pointers.

Note that we only take responsibility for the memory allocated in
`setup_unpack_trees_porcelain()` and not any other members of the
`struct unpack_trees_options`.

Signed-off-by: Martin Ågren 
---
 unpack-trees.h |  5 +
 builtin/checkout.c |  1 +
 merge-recursive.c  |  1 +
 merge.c|  3 +++
 unpack-trees.c | 11 +++
 5 files changed, 21 insertions(+)

diff --git a/unpack-trees.h b/unpack-trees.h
index 41178ada94..70053cb3ff 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -33,6 +33,11 @@ enum unpack_trees_error_types {
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
  const char *cmd);
 
+/*
+ * Frees resources allocated by setup_unpack_trees_porcelain().
+ */
+extern void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
+
 struct unpack_trees_options {
unsigned int reset,
 merge,
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..5cebe170fc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -526,6 +526,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
init_tree_desc([1], tree->buffer, tree->size);
 
ret = unpack_trees(2, trees, );
+   clear_unpack_trees_porcelain();
if (ret == -1) {
/*
 * Unpack couldn't do a trivial merge; either
diff --git a/merge-recursive.c b/merge-recursive.c
index ddb0fa7369..338f63a952 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -382,6 +382,7 @@ static int unpack_trees_start(struct merge_options *o,
 static void unpack_trees_finish(struct merge_options *o)
 {
discard_index(>orig_index);
+   clear_unpack_trees_porcelain(>unpack_opts);
 }
 
 struct tree *write_tree_from_memory(struct merge_options *o)
diff --git a/merge.c b/merge.c
index f123658e58..b433291d0c 100644
--- a/merge.c
+++ b/merge.c
@@ -130,8 +130,11 @@ int checkout_fast_forward(const struct object_id *head,
 
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
+   clear_unpack_trees_porcelain();
return -1;
}
+   clear_unpack_trees_porcelain();
+
if (write_locked_index(_index, _file, COMMIT_LOCK))
return error(_("unable to write new index file"));
return 0;
diff --git a/unpack-trees.c b/unpack-trees.c
index 79fd97074e..25e766d30e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -179,6 +179,17 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
opts->unpack_rejects[i].strdup_strings = 1;
 }
 
+void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
+{
+   char **msgs = (char **)opts->msgs;
+
+   free(msgs[ERROR_WOULD_OVERWRITE]);
+   free(msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED]);
+   free(msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN]);
+
+   memset(opts->msgs, 0, sizeof(opts->msgs));
+}
+
 static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 unsigned int set, unsigned int clear)
 {
-- 
2.17.0.583.g9a75a153ac