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

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

[Stefan Beller]

This fixes a memory leak, when building the cache entries as
refresh_cache_entry may decide to return NULL in case of

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

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

[Junio C Hamano]

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?

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.

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

(unknown),

[Stefan Beller]

On Tue, Feb 17, 2015 at 10:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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