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

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Jun 8, 2013, 11:32 UTC
Message-ID: <51B31651.6020307@lsrfire.ath.cx>
In-Reply-To: <1370644168-4745-3-git-send-email-felipe.contreras@gmail.com>

Am 08.06.2013 00:29, schrieb Felipe Contreras:

Show 5 quoted lines

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

Show 16 quoted lines

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

Show 10 quoted lines

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