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

# add entries

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);
+       istate->cache = NULL;
+       istate->cache_alloc = 0;
        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.


To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to
More majordomo info at

Reply via email to