Re: [PATCH v2 1/3] read-cache: plug a few leaks
- From
René Scharfe <rene.scharfe@lsrfire.ath.cx>
- Date
- May 30, 2013, 15:13 UTC
- Message-ID
- <51A76C8E.1080009@lsrfire.ath.cx>
- In-Reply-To
- <1369920861-30030-2-git-send-email-felipe.contreras@gmail.com>
Am 30.05.2013 15:34, schrieb Felipe Contreras:
Show 23 quoted lines
> 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@gmail.com> > --- > 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?
Show 13 quoted lines
> istate->cache = xcalloc(istate->cache_alloc, sizeof(struct cache_entry *)); > istate->initialized = 1; > > @@ -1510,6 +1511,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 was preparing a similar change, looks good. There is a comment at the end of discard_index() that becomes wrong due to that patch, though -- better remove it as well. It was already outdated as it mentioned active_cache, while the function can be used with any index_state.
diff --git a/read-cache.c b/read-cache.c index e5dc96f..0f868af 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1522,8 +1522,6 @@ int discard_index(struct index_state *istate) free_name_hash(istate); cache_tree_free(&(istate->cache_tree)); istate->initialized = 0; - - /* no need to throw away allocated active_cache */ return 0; }