threads / patch / 33987
Subject: [PATCH v2 1/3] read-cache: plug a few leaks
Felipe Contreras[PATCH v2 0/3] cherry-pick: fix memory leaks
Hi,
A small change since v1; one patch is dropped and another is updated to make up for it.
Felipe Contreras (3): read-cache: plug a few leaks unpack-trees: plug a memory leak unpack-trees: free created cache entries
read-cache.c | 4 ++++ unpack-trees.c | 16 +++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-)
-- 1.8.3.rc3.312.g47657de
Felipe ContrerasWe 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); 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;
-- 1.8.3.rc3.312.g47657de
René ScharfeRe: [PATCH v2 1/3] read-cache: plug a few leaks
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@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?
> 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; }
Felipe ContrerasRe: [PATCH v2 1/3] read-cache: plug a few leaks
On Thu, May 30, 2013 at 10:13 AM, René Scharfe <rene.scharfe@lsrfire.ath.cx> 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@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?
We can remove that line, but then if some code does this:
discard_cache(); # add entries read_cache();
There will be a memory leak.
-- Felipe Contreras
Felipe ContrerasRe: [PATCH v2 1/3] read-cache: plug a few leaks
On Thu, May 30, 2013 at 10:13 AM, René Scharfe <rene.scharfe@lsrfire.ath.cx> 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@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?
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
Felipe Contreras[PATCH v2 2/3] unpack-trees: plug a memory leak
Before overwriting the destination index, first let's discard it's contents.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- unpack-trees.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/unpack-trees.c b/unpack-trees.c index ede4299..eff2944 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options o->src_index = NULL; ret = check_updates(o) ? (-2) : 0; - if (o->dst_index) + if (o->dst_index) { + discard_index(o->dst_index); *o->dst_index = o->result; + } done: clear_exclude_list(&el);
-- 1.8.3.rc3.312.g47657de
Stefano LattariniRe: [PATCH v2 2/3] unpack-trees: plug a memory leak
On 05/30/2013 03:34 PM, Felipe Contreras wrote:
> Before overwriting the destination index, first let's discard it's >
s/it's/its/
> contents. > > [SNIP]
Regards, Stefano
Felipe Contreras[PATCH v2 3/3] unpack-trees: free created cache entries
We created them, and nobody else is going to destroy them.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- unpack-trees.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c index eff2944..9f19d01 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -590,8 +590,16 @@ static int unpack_nondirectories(int n, unsigned long mask, src[i + o->merge] = create_ce_entry(info, names + i, stage); } - if (o->merge) - return call_unpack_fn(src, o); + if (o->merge) { + int ret = call_unpack_fn(src, o); + for (i = 0; i < n; i++) { + struct cache_entry *ce = src[i + o->merge]; + if (!ce || ce == o->df_conflict_entry) + continue; + free(ce); + } + return ret; + } for (i = 0; i < n; i++) if (src[i] && src[i] != o->df_conflict_entry)
-- 1.8.3.rc3.312.g47657de
René ScharfeRe: [PATCH v2 3/3] unpack-trees: free created cache entries
Am 30.05.2013 15:34, schrieb Felipe Contreras:
> We created them, and nobody else is going to destroy them.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> unpack-trees.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index eff2944..9f19d01 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -590,8 +590,16 @@ static int unpack_nondirectories(int n, unsigned long mask,
> src[i + o->merge] = create_ce_entry(info, names + i, stage);
> }
>
> - if (o->merge)
> - return call_unpack_fn(src, o);
> + if (o->merge) {
> + int ret = call_unpack_fn(src, o);
> + for (i = 0; i < n; i++) {
> + struct cache_entry *ce = src[i + o->merge];
> + if (!ce || ce == o->df_conflict_entry)
> + continue;
> + free(ce);
> + }
> + return ret;
> + }Ah, now I understand what you meant in that other email. That works as well, of course. It's slightly nicer on the eye, admittedly.
René