On Sat, Jun 8, 2013 at 6:32 AM, René Scharfe <rene.scha...@lsrfire.ath.cx> wrote: > 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?
I don't know. > Or to get rid of it and check for ->cache_alloc instead? That might make sense. I was thinking on renaming 'initialized' to 'loaded', but I really don't care. >> Signed-off-by: Felipe Contreras <felipe.contre...@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? I don't know. > Wouldn't that be a > bug, namely forgetting to call discard_cache before read_cache? Why would it be a bug? There's nothing in the API that hints there's a problem with that. > 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. Why leave it out? If somebody makes the mistake of doing the above sequence, would you prefer that we leak? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
