{"thread":{"id":"33987","subject":"[PATCH v2 1/3] read-cache: plug a few leaks","startedAt":"2013-05-30T13:34:18Z","lastAt":"2013-05-31T08:22:33Z","messageCount":9,"participants":["Felipe Contreras","Stefano Lattarini","René Scharfe"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"218998","messageId":"1369920861-30030-1-git-send-email-felipe.contreras@gmail.com","threadId":"33987","inReplyTo":null,"subject":"[PATCH v2 0/3] cherry-pick: fix memory leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T13:34:18Z","receivedAt":"2013-05-30T13:34:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nA small change since v1; one patch is dropped and another is updated to make up\nfor it.\n\nFelipe Contreras (3):\n read-cache: plug a few leaks\n unpack-trees: plug a memory leak\n unpack-trees: free created cache entries\n\n read-cache.c | 4 ++++\n unpack-trees.c | 16 +++++++++++++---\n 2 files changed, 17 insertions(+), 3 deletions(-)\n\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218995","messageId":"1369920861-30030-2-git-send-email-felipe.contreras@gmail.com","threadId":"33987","inReplyTo":"1369920861-30030-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 1/3] read-cache: plug a few leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T13:34:19Z","receivedAt":"2013-05-30T13:34:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We don't free 'istate->cache' properly.\n\nApparently 'initialized' doesn't really mean initialized, but loaded, or\nrather 'not-empty', and the cache can be used even if it's not\n'initialized', so we can't rely on this variable to keep track of the\n'istate->cache'.\n\nSo assume it always has data, and free it before overwriting it.\n\nSigned-off-by: Felipe Contreras \n---\n read-cache.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 04ed561..e5dc96f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1449,6 +1449,7 @@ int read_index_from(struct index_state *istate, const char *path)\n \tistate->version = ntohl(hdr->hdr_version);\n \tistate->cache_nr = ntohl(hdr->hdr_entries);\n \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n+\tfree(istate->cache);\n \tistate->cache = xcalloc(istate->cache_alloc, sizeof(struct cache_entry *));\n \tistate->initialized = 1;\n \n@@ -1510,6 +1511,9 @@ int discard_index(struct index_state *istate)\n \n \tfor (i = 0; i < istate->cache_nr; i++)\n \t\tfree(istate->cache[i]);\n+\tfree(istate->cache);\n+\tistate->cache = NULL;\n+\tistate->cache_alloc = 0;\n \tresolve_undo_clear_index(istate);\n \tistate->cache_nr = 0;\n \tistate->cache_changed = 0;\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218997","messageId":"1369920861-30030-3-git-send-email-felipe.contreras@gmail.com","threadId":"33987","inReplyTo":"1369920861-30030-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 2/3] unpack-trees: plug a memory leak","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T13:34:20Z","receivedAt":"2013-05-30T13:34:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Before overwriting the destination index, first let's discard it's\ncontents.\n\nSigned-off-by: Felipe Contreras \n---\n unpack-trees.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex ede4299..eff2944 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1146,8 +1146,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \n \to->src_index = NULL;\n \tret = check_updates(o) ? (-2) : 0;\n-\tif (o->dst_index)\n+\tif (o->dst_index) {\n+\t\tdiscard_index(o->dst_index);\n \t\t*o->dst_index = o->result;\n+\t}\n \n done:\n \tclear_exclude_list(&el);\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218996","messageId":"1369920861-30030-4-git-send-email-felipe.contreras@gmail.com","threadId":"33987","inReplyTo":"1369920861-30030-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 3/3] unpack-trees: free created cache entries","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-30T13:34:21Z","receivedAt":"2013-05-30T13:34:21Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We created them, and nobody else is going to destroy them.\n\nSigned-off-by: Felipe Contreras \n---\n unpack-trees.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex eff2944..9f19d01 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -590,8 +590,16 @@ static int unpack_nondirectories(int n, unsigned long mask,\n \t\tsrc[i + o->merge] = create_ce_entry(info, names + i, stage);\n \t}\n \n-\tif (o->merge)\n-\t\treturn call_unpack_fn(src, o);\n+\tif (o->merge) {\n+\t\tint ret = call_unpack_fn(src, o);\n+\t\tfor (i = 0; i < n; i++) {\n+\t\t\tstruct cache_entry *ce = src[i + o->merge];\n+\t\t\tif (!ce || ce == o->df_conflict_entry)\n+\t\t\t\tcontinue;\n+\t\t\tfree(ce);\n+\t\t}\n+\t\treturn ret;\n+\t}\n \n \tfor (i = 0; i < n; i++)\n \t\tif (src[i] && src[i] != o->df_conflict_entry)\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218999","messageId":"51A756E0.4050409@gmail.com","threadId":"33987","inReplyTo":"1369920861-30030-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 2/3] unpack-trees: plug a memory leak","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2013-05-30T13:40:48Z","receivedAt":"2013-05-30T13:40:48Z","isPatch":true,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"On 05/30/2013 03:34 PM, Felipe Contreras wrote:\n> Before overwriting the destination index, first let's discard it's\n>\ns/it's/its/\n\n> contents.\n>\n> [SNIP]\n\nRegards,\n Stefano\n"},{"id":"219014","messageId":"51A766F0.3030408@lsrfire.ath.cx","threadId":"33987","inReplyTo":"1369920861-30030-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 3/3] unpack-trees: free created cache entries","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-05-30T14:49:20Z","receivedAt":"2013-05-30T14:49:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.05.2013 15:34, schrieb Felipe Contreras:\n> We created them, and nobody else is going to destroy them.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> unpack-trees.c | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n>\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index eff2944..9f19d01 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -590,8 +590,16 @@ static int unpack_nondirectories(int n, unsigned long mask,\n> \t\tsrc[i + o->merge] = create_ce_entry(info, names + i, stage);\n> \t}\n>\n> -\tif (o->merge)\n> -\t\treturn call_unpack_fn(src, o);\n> +\tif (o->merge) {\n> +\t\tint ret = call_unpack_fn(src, o);\n> +\t\tfor (i = 0; i < n; i++) {\n> +\t\t\tstruct cache_entry *ce = src[i + o->merge];\n> +\t\t\tif (!ce || ce == o->df_conflict_entry)\n> +\t\t\t\tcontinue;\n> +\t\t\tfree(ce);\n> +\t\t}\n> +\t\treturn ret;\n> +\t}\n\nAh, now I understand what you meant in that other email. That works as \nwell, of course. It's slightly nicer on the eye, admittedly.\n\nRené\n"},{"id":"219018","messageId":"51A76C8E.1080009@lsrfire.ath.cx","threadId":"33987","inReplyTo":"1369920861-30030-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 1/3] read-cache: plug a few leaks","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-05-30T15:13:18Z","receivedAt":"2013-05-30T15:13:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.05.2013 15:34, schrieb Felipe Contreras:\n> We don't free 'istate->cache' properly.\n> \n> Apparently 'initialized' doesn't really mean initialized, but loaded, or\n> rather 'not-empty', and the cache can be used even if it's not\n> 'initialized', so we can't rely on this variable to keep track of the\n> 'istate->cache'.\n> \n> So assume it always has data, and free it before overwriting it.\n> \n> Signed-off-by: Felipe Contreras \n> ---\n> read-cache.c | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/read-cache.c b/read-cache.c\n> index 04ed561..e5dc96f 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1449,6 +1449,7 @@ int read_index_from(struct index_state *istate, const char *path)\n> \tistate->version = ntohl(hdr->hdr_version);\n> \tistate->cache_nr = ntohl(hdr->hdr_entries);\n> \tistate->cache_alloc = alloc_nr(istate->cache_nr);\n> +\tfree(istate->cache);\n\nWith that change, callers of read_index_from need to set ->cache to\nNULL for uninitialized (on-stack) index_state variables. They only had\nto set ->initialized to 0 before in that situation. It this chunk safe\nfor all existing callers? Shouldn't the same free in discard_index\n(added below) be enough?\n\n> \tistate->cache = xcalloc(istate->cache_alloc, sizeof(struct cache_entry *));\n> \tistate->initialized = 1;\n> \n> @@ -1510,6 +1511,9 @@ int discard_index(struct index_state *istate)\n> \n> \tfor (i = 0; i < istate->cache_nr; i++)\n> \t\tfree(istate->cache[i]);\n> +\tfree(istate->cache);\n> +\tistate->cache = NULL;\n> +\tistate->cache_alloc = 0;\n> \tresolve_undo_clear_index(istate);\n> \tistate->cache_nr = 0;\n> \tistate->cache_changed = 0;\n\nI was preparing a similar change, looks good. There is a comment at\nthe end of discard_index() that becomes wrong due to that patch,\nthough -- better remove it as well. It was already outdated as it\nmentioned active_cache, while the function can be used with any\nindex_state.\n\ndiff --git a/read-cache.c b/read-cache.c\nindex e5dc96f..0f868af 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1522,8 +1522,6 @@ int discard_index(struct index_state *istate)\n \tfree_name_hash(istate);\n \tcache_tree_free(&(istate->cache_tree));\n \tistate->initialized = 0;\n-\n-\t/* no need to throw away allocated active_cache */\n \treturn 0;\n }\n \n"},{"id":"219054","messageId":"CAMP44s06DCaR2GF9yV5h9-d4tF43kjgbk2WyCqB8ZYCDYkETfQ@mail.gmail.com","threadId":"33987","inReplyTo":"51A76C8E.1080009@lsrfire.ath.cx","subject":"Re: [PATCH v2 1/3] read-cache: plug a few leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-31T03:40:43Z","receivedAt":"2013-05-31T03:40:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 30, 2013 at 10:13 AM, René Scharfe\n wrote:\n> Am 30.05.2013 15:34, schrieb Felipe Contreras:\n>> We don't free 'istate->cache' properly.\n>>\n>> Apparently 'initialized' doesn't really mean initialized, but loaded, or\n>> rather 'not-empty', and the cache can be used even if it's not\n>> 'initialized', so we can't rely on this variable to keep track of the\n>> 'istate->cache'.\n>>\n>> So assume it always has data, and free it before overwriting it.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>> read-cache.c | 4 ++++\n>> 1 file changed, 4 insertions(+)\n>>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 04ed561..e5dc96f 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -1449,6 +1449,7 @@ int read_index_from(struct index_state *istate, const char *path)\n>> istate->version = ntohl(hdr->hdr_version);\n>> istate->cache_nr = ntohl(hdr->hdr_entries);\n>> istate->cache_alloc = alloc_nr(istate->cache_nr);\n>> + free(istate->cache);\n>\n> With that change, callers of read_index_from need to set ->cache to\n> NULL for uninitialized (on-stack) index_state variables. They only had\n> to set ->initialized to 0 before in that situation. It this chunk safe\n> for all existing callers? Shouldn't the same free in discard_index\n> (added below) be enough?\n\nWe can remove that line, but then if some code does this:\n\ndiscard_cache();\n# add entries\nread_cache();\n\nThere will be a memory leak.\n\n-- \nFelipe Contreras\n"},{"id":"219086","messageId":"CAMP44s1PYK1efXjk8WhCpXM9g_tBf-vXbXRY4eC2tiVym+_07g@mail.gmail.com","threadId":"33987","inReplyTo":"51A76C8E.1080009@lsrfire.ath.cx","subject":"Re: [PATCH v2 1/3] read-cache: plug a few leaks","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-31T08:22:33Z","receivedAt":"2013-05-31T08:22:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, May 30, 2013 at 10:13 AM, René Scharfe\n wrote:\n> Am 30.05.2013 15:34, schrieb Felipe Contreras:\n>> We don't free 'istate->cache' properly.\n>>\n>> Apparently 'initialized' doesn't really mean initialized, but loaded, or\n>> rather 'not-empty', and the cache can be used even if it's not\n>> 'initialized', so we can't rely on this variable to keep track of the\n>> 'istate->cache'.\n>>\n>> So assume it always has data, and free it before overwriting it.\n>>\n>> Signed-off-by: Felipe Contreras \n>> ---\n>> read-cache.c | 4 ++++\n>> 1 file changed, 4 insertions(+)\n>>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 04ed561..e5dc96f 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -1449,6 +1449,7 @@ int read_index_from(struct index_state *istate, const char *path)\n>> istate->version = ntohl(hdr->hdr_version);\n>> istate->cache_nr = ntohl(hdr->hdr_entries);\n>> istate->cache_alloc = alloc_nr(istate->cache_nr);\n>> + free(istate->cache);\n>\n> With that change, callers of read_index_from need to set ->cache to\n> NULL for uninitialized (on-stack) index_state variables. They only had\n> to set ->initialized to 0 before in that situation. It this chunk safe\n> for all existing callers? Shouldn't the same free in discard_index\n> (added below) be enough?\n\nIt would be enough if every discard_cache() is not followed by a\nread_cache() after adding entries.\n\nI was adding a init_index() helper, but it turns out only very few\nplaces initialize the index, and all of them zero the structure (or\ndeclare it so it's zeroed on load), so I think this change is safe\nlike that. Also, I don't see any place manually doing initialize=0.\n\n-- \nFelipe Contreras\n"}]}