Re: [PATCH v2 13/13] alloc: allow arbitrary repositories for alloc functions

[Jeff King <peff@peff.net>]

On Mon, May 07, 2018 at 03:59:16PM -0700, Stefan Beller wrote:

> @@ -501,9 +509,31 @@ void raw_object_store_clear(struct raw_object_store *o)
>  void parsed_object_pool_clear(struct parsed_object_pool *o)
> [...]
> +	for (i = 0; i < o->obj_hash_size; i++) {
> +		struct object *obj = o->obj_hash[i];
> +
> +		if (!obj)
> +			continue;
> +
> +		if (obj->type == OBJ_TREE) {
> +			free(((struct tree*)obj)->buffer);
> +		} else if (obj->type == OBJ_COMMIT) {
> +			free_commit_list(((struct commit*)obj)->parents);
> +			free(&((struct commit*)obj)->util);
> +		}
> +	}

Coverity complains about this final free(). I think the "&" is doing an
incorrect extra level of indirection?

That said, I'm not sure if it is safe to blindly free the util field. We
don't necessarily know what downstream code has pointed it to. It may
not be allocated memory[1], or it may even be a more complicated data
structure that has sub-components that need freeing[2].

In the long run, it may be worth trying to get rid of this util field
completely, in favor of having callers use a commit_slab. That has
better memory-ownership semantics, and it would save 8 bytes in struct
commit.

[1] Grepping for "commit->util =", sequencer.c seems to assign pointers
    into other arrays, as well as the "(void *)1".

[2] Most assignments seem to be flex-structs, but blame.c assigns a
    linked list.

-Peff