Re: [PATCH v3 2/2] read-cache: plug a few leaks

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Am 08.06.2013 00:29, schrieb Felipe Contreras:
> We are not freeing 'istate->cache' properly.
>
> We can't rely on 'initialized' to keep track of the 'istate->cache',
> because it doesn't really mean it's initialized. So assume it always has
> data, and free it before overwriting it.

That sounds a bit backwards to me.  If ->initialized doesn't mean that 
the index state is initialized then something is fishy.  Would it make 
sense and be sufficient to set ->initialized in add_index_entry?  Or to 
get rid of it and check for ->cache_alloc instead?

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>   read-cache.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index 5e30746..a1dd04d 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate, const char *path)
>   	istate->version = ntohl(hdr->hdr_version);
>   	istate->cache_nr = ntohl(hdr->hdr_entries);
>   	istate->cache_alloc = alloc_nr(istate->cache_nr);
> +	free(istate->cache);
>   	istate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache));
>   	istate->initialized = 1;

You wrote earlier that this change is safe with current callers and that 
it prevents leaks with the following sequence:

discard_cache();
# add entries
read_cache();

Do we currently have such a call sequence somewhere?  Wouldn't that be a 
bug, namely forgetting to call discard_cache before read_cache?

I've added a "assert(istate->cache_nr == 0);" a few lines above and the 
test suite still passed.  With the hunk below, ->cache is also always 
NULL and cache_alloc is always 0 at that point.  So we don't need that 
free call there in the cases covered by the test suite at least -- 
better leave it out.

> @@ -1512,6 +1513,9 @@ int discard_index(struct index_state *istate)
>
>   	for (i = 0; i < istate->cache_nr; i++)
>   		free(istate->cache[i]);
> +	free(istate->cache);
> +	istate->cache = NULL;
> +	istate->cache_alloc = 0;
>   	resolve_undo_clear_index(istate);
>   	istate->cache_nr = 0;
>   	istate->cache_changed = 0;

I still like this part, but also would still recommend to remove the now 
doubly-outdated comment a few lines below that says "no need to throw 
away allocated active_cache".  It was valid back when there was only a 
single in-memory instance of the index and no istate parameter.

René