{"thread":{"id":"38582","subject":"[PATCH] read-cache.c: free cache entry when refreshing fails","startedAt":"2015-02-17T18:06:14Z","lastAt":"2015-02-17T18:27:08Z","messageCount":3,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"256223","messageId":"1424196374-29509-1-git-send-email-sbeller@google.com","threadId":"38582","inReplyTo":null,"subject":"[PATCH] read-cache.c: free cache entry when refreshing fails","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-17T18:06:14Z","receivedAt":"2015-02-17T18:06:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This fixes a memory leak, when building the cache entries as\nrefresh_cache_entry may decide to return NULL in case of\n\nI am not sure how severe this memory leak is as it was found by\nscan.coverity.com, CID 290041.\n\nSigned-off-by: Stefan Beller \n---\n read-cache.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 9cff715..8d71860 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -725,7 +725,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \t\tunsigned int refresh_options)\n {\n \tint size, len;\n-\tstruct cache_entry *ce;\n+\tstruct cache_entry *ce, *ret;\n \n \tif (!verify_path(path)) {\n \t\terror(\"Invalid path '%s'\", path);\n@@ -742,7 +742,13 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \tce->ce_namelen = len;\n \tce->ce_mode = create_ce_mode(mode);\n \n-\treturn refresh_cache_entry(ce, refresh_options);\n+\tret = refresh_cache_entry(ce, refresh_options);\n+\tif (!ret) {\n+\t\tfree(ce);\n+\t\treturn NULL;\n+\t} else {\n+\t\treturn ret;\n+\t}\n }\n \n int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n-- \n2.3.0.81.gc37f363\n"},{"id":"256224","messageId":"xmqqoaos2yn5.fsf@gitster.dls.corp.google.com","threadId":"38582","inReplyTo":"1424196374-29509-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] read-cache.c: free cache entry when refreshing fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-17T18:14:54Z","receivedAt":"2015-02-17T18:14:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This fixes a memory leak, when building the cache entries as\n> refresh_cache_entry may decide to return NULL in case of\n>\n\nin case of what?\n\nI briefly wondered if refresh_cache_ent() should do the freeing but\nthat does not make sense at all if done unconditionally because the\nother caller of the function does want to retain ce on error, and it\nmakes little sense to invent FREE_CE_ON_ERROR bit in refresh_option,\neither, so this fix looks sensible.\n\n> I am not sure how severe this memory leak is as it was found by\n> scan.coverity.com, CID 290041.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> read-cache.c | 10 ++++++++--\n> 1 file changed, 8 insertions(+), 2 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 9cff715..8d71860 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -725,7 +725,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n> \t\tunsigned int refresh_options)\n> {\n> \tint size, len;\n> -\tstruct cache_entry *ce;\n> +\tstruct cache_entry *ce, *ret;\n> \n> \tif (!verify_path(path)) {\n> \t\terror(\"Invalid path '%s'\", path);\n> @@ -742,7 +742,13 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n> \tce->ce_namelen = len;\n> \tce->ce_mode = create_ce_mode(mode);\n> \n> -\treturn refresh_cache_entry(ce, refresh_options);\n> +\tret = refresh_cache_entry(ce, refresh_options);\n> +\tif (!ret) {\n> +\t\tfree(ce);\n> +\t\treturn NULL;\n> +\t} else {\n> +\t\treturn ret;\n> +\t}\n> }\n> \n> int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n"},{"id":"256226","messageId":"1424197628-568-1-git-send-email-sbeller@google.com","threadId":"38582","inReplyTo":"xmqqoaos2yn5.fsf@gitster.dls.corp.google.com","subject":"(unknown)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-17T18:27:08Z","receivedAt":"2015-02-17T18:27:08Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"\nOn Tue, Feb 17, 2015 at 10:14 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> This fixes a memory leak, when building the cache entries as\n>> refresh_cache_entry may decide to return NULL in case of\n>>\n>\n> in case of what?\n\nYeah, I got distracted when rewriting the commit message as I was \nlooking at refresh_cache_ent() wondering if there is a better place to \nfree the memory. Just as you said below.\n\nMaybe we can drop that part of the sentence as it doesn't matter\nwhy refresh_cache_ent() returns NULL. All that matters is the \npossibility of it returning NULL.\n\n>\n> I briefly wondered if refresh_cache_ent() should do the freeing but\n> that does not make sense at all if done unconditionally because the\n> other caller of the function does want to retain ce on error, and it\n> makes little sense to invent FREE_CE_ON_ERROR bit in refresh_option,\n> either, so this fix looks sensible.\n>\n\nSo here is a reworded commit message:\n\n---8<---\nFrom 3a1f646c1dbe828b68e1b269290d2b5593f86455 Mon Sep 17 00:00:00 2001\nFrom: Stefan Beller \nDate: Tue, 17 Feb 2015 10:05:36 -0800\nSubject: [PATCH] read-cache.c: free cache entry when refreshing fails\n\nThis fixes a memory leak when building the cache entries as\nrefresh_cache_entry may decide to return NULL, but it doesn't\nfree the cache entry structure which was passed in as an argument.\n\nI am not sure how severe this memory leak is as it was found by\nscan.coverity.com, CID 290041.\n\nSigned-off-by: Stefan Beller \n---\n read-cache.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 9cff715..8d71860 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -725,7 +725,7 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \t\tunsigned int refresh_options)\n {\n \tint size, len;\n-\tstruct cache_entry *ce;\n+\tstruct cache_entry *ce, *ret;\n \n \tif (!verify_path(path)) {\n \t\terror(\"Invalid path '%s'\", path);\n@@ -742,7 +742,13 @@ struct cache_entry *make_cache_entry(unsigned int mode,\n \tce->ce_namelen = len;\n \tce->ce_mode = create_ce_mode(mode);\n \n-\treturn refresh_cache_entry(ce, refresh_options);\n+\tret = refresh_cache_entry(ce, refresh_options);\n+\tif (!ret) {\n+\t\tfree(ce);\n+\t\treturn NULL;\n+\t} else {\n+\t\treturn ret;\n+\t}\n }\n \n int ce_same_name(const struct cache_entry *a, const struct cache_entry *b)\n-- \n2.3.0.81.gc37f363\n"}]}