Re: [PATCH] read-cache.c: free cache entry when refreshing fails

Stefan Beller [off-list ref] 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?

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.

On Tue, Feb 17, 2015 at 10:14 AM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

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.

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 <redacted>
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 <redacted>
---
 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