[PATCH v2 0/3] cherry-pick: fix memory leaks

[PATCH v2 0/3] cherry-pick: fix memory leaks

[Felipe Contreras]

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

[PATCH v2 1/3] read-cache: plug a few leaks

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

Re: [PATCH v2 1/3] read-cache: plug a few leaks

[René Scharfe]

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

Re: [PATCH v2 1/3] read-cache: plug a few leaks

[Felipe Contreras]

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

Re: [PATCH v2 1/3] read-cache: plug a few leaks

[Felipe Contreras]

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

[PATCH v2 2/3] unpack-trees: plug a memory leak

[Felipe Contreras]

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

Re: [PATCH v2 2/3] unpack-trees: plug a memory leak

[Stefano Lattarini]

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

[PATCH v2 3/3] unpack-trees: free created cache entries

[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;
+	}
 
 	for (i = 0; i < n; i++)
 		if (src[i] && src[i] != o->df_conflict_entry)
-- 
1.8.3.rc3.312.g47657de

Re: [PATCH v2 3/3] unpack-trees: free created cache entries

[René Scharfe]

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é