On Thu, May 30, 2013 at 10:13 AM, René Scharfe <rene.scharfe@xxxxxxxxxxxxxx> wrote: > Am 30.05.2013 15:34, schrieb Felipe Contreras: >> We don't free 'istate->cache' properly. >> >> Apparently 'initialized' doesn't really mean initialized, but loaded, or >> rather 'not-empty', and the cache can be used even if it's not >> 'initialized', so we can't rely on this variable to keep track of the >> 'istate->cache'. >> >> So assume it always has data, and free it before overwriting it. >> >> 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 04ed561..e5dc96f 100644 >> --- a/read-cache.c >> +++ b/read-cache.c >> @@ -1449,6 +1449,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); > > With that change, callers of read_index_from need to set ->cache to > NULL for uninitialized (on-stack) index_state variables. They only had > to set ->initialized to 0 before in that situation. It this chunk safe > for all existing callers? Shouldn't the same free in discard_index > (added below) be enough? It would be enough if every discard_cache() is not followed by a read_cache() after adding entries. I was adding a init_index() helper, but it turns out only very few places initialize the index, and all of them zero the structure (or declare it so it's zeroed on load), so I think this change is safe like that. Also, I don't see any place manually doing initialize=0. -- Felipe Contreras -- 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