* [PATCH v2 0/3] cherry-pick: fix memory leaks
@ 2013-05-30 13:34 Felipe Contreras
2013-05-30 13:34 ` [PATCH v2 1/3] read-cache: plug a few leaks Felipe Contreras
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Felipe Contreras @ 2013-05-30 13:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 1/3] read-cache: plug a few leaks
2013-05-30 13:34 [PATCH v2 0/3] cherry-pick: fix memory leaks Felipe Contreras
@ 2013-05-30 13:34 ` Felipe Contreras
2013-05-30 15:13 ` René Scharfe
2013-05-30 13:34 ` [PATCH v2 2/3] unpack-trees: plug a memory leak Felipe Contreras
2013-05-30 13:34 ` [PATCH v2 3/3] unpack-trees: free created cache entries Felipe Contreras
2 siblings, 1 reply; 9+ messages in thread
From: Felipe Contreras @ 2013-05-30 13:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/3] unpack-trees: plug a memory leak
2013-05-30 13:34 [PATCH v2 0/3] cherry-pick: fix memory leaks Felipe Contreras
2013-05-30 13:34 ` [PATCH v2 1/3] read-cache: plug a few leaks Felipe Contreras
@ 2013-05-30 13:34 ` Felipe Contreras
2013-05-30 13:40 ` Stefano Lattarini
2013-05-30 13:34 ` [PATCH v2 3/3] unpack-trees: free created cache entries Felipe Contreras
2 siblings, 1 reply; 9+ messages in thread
From: Felipe Contreras @ 2013-05-30 13:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 3/3] unpack-trees: free created cache entries
2013-05-30 13:34 [PATCH v2 0/3] cherry-pick: fix memory leaks Felipe Contreras
2013-05-30 13:34 ` [PATCH v2 1/3] read-cache: plug a few leaks Felipe Contreras
2013-05-30 13:34 ` [PATCH v2 2/3] unpack-trees: plug a memory leak Felipe Contreras
@ 2013-05-30 13:34 ` Felipe Contreras
2013-05-30 14:49 ` René Scharfe
2 siblings, 1 reply; 9+ messages in thread
From: Felipe Contreras @ 2013-05-30 13:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/3] unpack-trees: plug a memory leak
2013-05-30 13:34 ` [PATCH v2 2/3] unpack-trees: plug a memory leak Felipe Contreras
@ 2013-05-30 13:40 ` Stefano Lattarini
0 siblings, 0 replies; 9+ messages in thread
From: Stefano Lattarini @ 2013-05-30 13:40 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, René Scharfe,
Nguyễn Thái Ngọc Duy, Adam Spiers,
Ramkumar Ramachandra, Stephen Boyd
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] unpack-trees: free created cache entries
2013-05-30 13:34 ` [PATCH v2 3/3] unpack-trees: free created cache entries Felipe Contreras
@ 2013-05-30 14:49 ` René Scharfe
0 siblings, 0 replies; 9+ messages in thread
From: René Scharfe @ 2013-05-30 14:49 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy,
Adam Spiers, Ramkumar Ramachandra, Stephen Boyd
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é
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] read-cache: plug a few leaks
2013-05-30 13:34 ` [PATCH v2 1/3] read-cache: plug a few leaks Felipe Contreras
@ 2013-05-30 15:13 ` René Scharfe
2013-05-31 3:40 ` Felipe Contreras
2013-05-31 8:22 ` Felipe Contreras
0 siblings, 2 replies; 9+ messages in thread
From: René Scharfe @ 2013-05-30 15:13 UTC (permalink / raw)
To: Felipe Contreras
Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy,
Adam Spiers, Ramkumar Ramachandra, Stephen Boyd
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;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] read-cache: plug a few leaks
2013-05-30 15:13 ` René Scharfe
@ 2013-05-31 3:40 ` Felipe Contreras
2013-05-31 8:22 ` Felipe Contreras
1 sibling, 0 replies; 9+ messages in thread
From: Felipe Contreras @ 2013-05-31 3:40 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy,
Adam Spiers, Ramkumar Ramachandra, Stephen Boyd
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/3] read-cache: plug a few leaks
2013-05-30 15:13 ` René Scharfe
2013-05-31 3:40 ` Felipe Contreras
@ 2013-05-31 8:22 ` Felipe Contreras
1 sibling, 0 replies; 9+ messages in thread
From: Felipe Contreras @ 2013-05-31 8:22 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy,
Adam Spiers, Ramkumar Ramachandra, Stephen Boyd
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-05-31 8:22 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-30 13:34 [PATCH v2 0/3] cherry-pick: fix memory leaks Felipe Contreras
2013-05-30 13:34 ` [PATCH v2 1/3] read-cache: plug a few leaks Felipe Contreras
2013-05-30 15:13 ` René Scharfe
2013-05-31 3:40 ` Felipe Contreras
2013-05-31 8:22 ` Felipe Contreras
2013-05-30 13:34 ` [PATCH v2 2/3] unpack-trees: plug a memory leak Felipe Contreras
2013-05-30 13:40 ` Stefano Lattarini
2013-05-30 13:34 ` [PATCH v2 3/3] unpack-trees: free created cache entries Felipe Contreras
2013-05-30 14:49 ` René Scharfe
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).