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

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.

Reply via email to