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

From: Jeff King <peff@peff.net>
Date: May 8, 2018, 10:10 UTC
Message-ID: <20180508101022.GA17501@sigill.intra.peff.net>
In-Reply-To: <20180507225916.155236-14-sbeller@google.com>

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

Show 16 quoted lines

> @@ -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