(unknown)
- From
Stefan Beller <sbeller@google.com>
- Date
- Feb 17, 2015, 18:27 UTC
- Message-ID
- <1424197628-568-1-git-send-email-sbeller@google.com>
- In-Reply-To
- <xmqqoaos2yn5.fsf@gitster.dls.corp.google.com>
On Tue, Feb 17, 2015 at 10:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> Stefan Beller <sbeller@google.com> writes: > >> This fixes a memory leak, when building the cache entries as >> refresh_cache_entry may decide to return NULL in case of >> > > in case of what?
Yeah, I got distracted when rewriting the commit message as I was looking at refresh_cache_ent() wondering if there is a better place to free the memory. Just as you said below.
Maybe we can drop that part of the sentence as it doesn't matter why refresh_cache_ent() returns NULL. All that matters is the possibility of it returning NULL.
Show 7 quoted lines
> > I briefly wondered if refresh_cache_ent() should do the freeing but > that does not make sense at all if done unconditionally because the > other caller of the function does want to retain ce on error, and it > makes little sense to invent FREE_CE_ON_ERROR bit in refresh_option, > either, so this fix looks sensible. >
So here is a reworded commit message:
---8<--- From 3a1f646c1dbe828b68e1b269290d2b5593f86455 Mon Sep 17 00:00:00 2001 From: Stefan Beller <sbeller@google.com> Date: Tue, 17 Feb 2015 10:05:36 -0800 Subject: [PATCH] read-cache.c: free cache entry when refreshing fails
This fixes a memory leak when building the cache entries as refresh_cache_entry may decide to return NULL, but it doesn't free the cache entry structure which was passed in as an argument.
I am not sure how severe this memory leak is as it was found by scan.coverity.com, CID 290041.
Signed-off-by: Stefan Beller <sbeller@google.com> --- read-cache.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/read-cache.c b/read-cache.c index 9cff715..8d71860 100644 --- a/read-cache.c +++ b/read-cache.c @@ -725,7 +725,7 @@ struct cache_entry *make_cache_entry(unsigned int mode, unsigned int refresh_options) { int size, len; - struct cache_entry *ce; + struct cache_entry *ce, *ret; if (!verify_path(path)) { error("Invalid path '%s'", path); @@ -742,7 +742,13 @@ struct cache_entry *make_cache_entry(unsigned int mode, ce->ce_namelen = len; ce->ce_mode = create_ce_mode(mode); - return refresh_cache_entry(ce, refresh_options); + ret = refresh_cache_entry(ce, refresh_options); + if (!ret) { + free(ce); + return NULL; + } else { + return ret; + } } int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)
-- 2.3.0.81.gc37f363