# [PATCH v2 1/3] read-cache: plug a few leaks 9 messages from 2013-05-30 to 2013-05-31. Participants: Felipe Contreras, Stefano Lattarini, René Scharfe. Thread: https://gitlist.dev/t/33987 ## Felipe Contreras, 2013-05-30 13:34 Subject: [PATCH v2 0/3] cherry-pick: fix memory leaks Message-ID: <1369920861-30030-1-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1369920861-30030-1-git-send-email-felipe.contreras%40gmail.com ``` 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 Contreras, 2013-05-30 13:34 Subject: [PATCH v2 1/3] read-cache: plug a few leaks Message-ID: <1369920861-30030-2-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1369920861-30030-2-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1369920861-30030-1-git-send-email-felipe.contreras@gmail.com> ``` 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 --- 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 ``` ## Felipe Contreras, 2013-05-30 13:34 Subject: [PATCH v2 2/3] unpack-trees: plug a memory leak Message-ID: <1369920861-30030-3-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1369920861-30030-3-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1369920861-30030-1-git-send-email-felipe.contreras@gmail.com> ``` Before overwriting the destination index, first let's discard it's contents. Signed-off-by: Felipe Contreras --- 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 ``` ## Felipe Contreras, 2013-05-30 13:34 Subject: [PATCH v2 3/3] unpack-trees: free created cache entries Message-ID: <1369920861-30030-4-git-send-email-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/1369920861-30030-4-git-send-email-felipe.contreras%40gmail.com In-Reply-To: <1369920861-30030-1-git-send-email-felipe.contreras@gmail.com> ``` We created them, and nobody else is going to destroy them. Signed-off-by: Felipe Contreras --- 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 ``` ## Stefano Lattarini, 2013-05-30 13:40 Subject: Re: [PATCH v2 2/3] unpack-trees: plug a memory leak Message-ID: <51A756E0.4050409@gmail.com> URL: https://gitlist.dev/e/51A756E0.4050409%40gmail.com In-Reply-To: <1369920861-30030-3-git-send-email-felipe.contreras@gmail.com> ``` 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 ``` ## René Scharfe, 2013-05-30 14:49 Subject: Re: [PATCH v2 3/3] unpack-trees: free created cache entries Message-ID: <51A766F0.3030408@lsrfire.ath.cx> URL: https://gitlist.dev/e/51A766F0.3030408%40lsrfire.ath.cx In-Reply-To: <1369920861-30030-4-git-send-email-felipe.contreras@gmail.com> ``` 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 > --- > 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é ``` ## René Scharfe, 2013-05-30 15:13 Subject: Re: [PATCH v2 1/3] read-cache: plug a few leaks Message-ID: <51A76C8E.1080009@lsrfire.ath.cx> URL: https://gitlist.dev/e/51A76C8E.1080009%40lsrfire.ath.cx In-Reply-To: <1369920861-30030-2-git-send-email-felipe.contreras@gmail.com> ``` 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 > --- > 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 Contreras, 2013-05-31 03:40 Subject: Re: [PATCH v2 1/3] read-cache: plug a few leaks Message-ID: URL: https://gitlist.dev/e/CAMP44s06DCaR2GF9yV5h9-d4tF43kjgbk2WyCqB8ZYCDYkETfQ%40mail.gmail.com In-Reply-To: <51A76C8E.1080009@lsrfire.ath.cx> ``` On Thu, May 30, 2013 at 10:13 AM, René Scharfe 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 >> --- >> 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 Contreras, 2013-05-31 08:22 Subject: Re: [PATCH v2 1/3] read-cache: plug a few leaks Message-ID: URL: https://gitlist.dev/e/CAMP44s1PYK1efXjk8WhCpXM9g_tBf-vXbXRY4eC2tiVym%2B_07g%40mail.gmail.com In-Reply-To: <51A76C8E.1080009@lsrfire.ath.cx> ``` On Thu, May 30, 2013 at 10:13 AM, René Scharfe 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 >> --- >> 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 ```