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@xxxxxxxxx>
---
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é
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html