Am 08.06.2013 14:15, schrieb Felipe Contreras:
On Sat, Jun 8, 2013 at 6:32 AM, René Scharfe
<rene.scharfe@xxxxxxxxxxxxxx> wrote:
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.
A comment before read_index_from says "remember to discard_cache()
before reading a different cache!". That is probably a reminder that
read_index_from does nothing if ->initialized is set. Entries added
before calling read_index_from make up a different cache, however, so I
think this comment applies for the call sequence above as well.
Only read_index_from and add_index_entry allocate ->cache, and only
discard_index frees it, so the two are a triple like malloc, realloc and
free.
Granted, these hints are not part of the API -- that looks like a
documentation bug, however.
Side note: I wonder why we need to guard against multiple
read_index_from calls in a row with ->initialized. Wouldn't it be
easier to avoid the duplicate calls in the first place? Finding them
now might be not so easy, though.
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?
Leaking is better than silently cleaning up after a buggy caller because
it still allows the underlying bug to be found. Even better would be an
assert, but it's important to make sure it doesn't trigger for existing
use cases.
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