{"thread":{"id":"38054","subject":"[PATCH 1/1] cat-file: fix a memory leak in cat_one_file","startedAt":"2014-11-26T14:17:17Z","lastAt":"2014-11-27T06:18:30Z","messageCount":3,"participants":["Alexander Kuleshov","Jonathan Nieder","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"252581","messageId":"CANCZXo7CKz2bOM=6ehed-VLUGtyThuBbwtd_QaBMm2KKhEo=Bw@mail.gmail.com","threadId":"38054","inReplyTo":null,"subject":"[PATCH 1/1] cat-file: fix a memory leak in cat_one_file","fromName":"Alexander Kuleshov","fromEmail":"kuleshovmail@gmail.com","sentAt":"2014-11-26T14:17:17Z","receivedAt":"2014-11-26T14:17:17Z","isPatch":true,"sender":{"key":"kuleshovmail@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2699235?v=4"},"body":"Sometimes we should free output buffer in cat_one_file, but not always.\nIt must to be freed only if it was allocated during git cat-file -t,\ncommit,tag and tree types handling.\n\nSigned-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>\n---\n builtin/cat-file.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex f8d8129..f5dad60 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -18,6 +18,7 @@ static int cat_one_file(int opt, const char\n*exp_type, const char *obj_name)\n     unsigned char sha1[20];\n     enum object_type type;\n     char *buf;\n+    int buf_must_free = 0;\n     unsigned long size;\n     struct object_context obj_context;\n\n@@ -68,9 +69,14 @@ static int cat_one_file(int opt, const char\n*exp_type, const char *obj_name)\n\n         if (type == OBJ_BLOB)\n             return stream_blob_to_fd(1, sha1, NULL, 0);\n+\n         buf = read_sha1_file(sha1, &type, &size);\n-        if (!buf)\n+        buf_must_free = 1;\n+\n+        if (!buf) {\n+            free(buf);\n             die(\"Cannot read object %s\", obj_name);\n+        }\n\n         /* otherwise just spit out the data */\n         break;\n@@ -100,6 +106,7 @@ static int cat_one_file(int opt, const char\n*exp_type, const char *obj_name)\n              */\n         }\n         buf = read_object_with_reference(sha1, exp_type, &size, NULL);\n+        buf_must_free = 1;\n         break;\n\n     default:\n@@ -110,6 +117,10 @@ static int cat_one_file(int opt, const char\n*exp_type, const char *obj_name)\n         die(\"git cat-file %s: bad file\", obj_name);\n\n     write_or_die(1, buf, size);\n+\n+    if (buf_must_free)\n+        free(buf);\n+\n     return 0;\n }\n\n-- \n2.2.0.rc3.224.gb2e243b\n"},{"id":"252623","messageId":"20141127002417.GT6527@google.com","threadId":"38054","inReplyTo":"CANCZXo7CKz2bOM=6ehed-VLUGtyThuBbwtd_QaBMm2KKhEo=Bw@mail.gmail.com","subject":"Re: [PATCH 1/1] cat-file: fix a memory leak in cat_one_file","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-27T00:24:17Z","receivedAt":"2014-11-27T00:24:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nAlexander Kuleshov wrote:\n\n> [Subject: cat-file: fix a memory leak in cat_one_file]\n\nCan you explain further?  How did you run into this?  Is it an\nunbounded leak or a small one-time leak?  What is the benefit of\napplying this patch?\n\n\"By code inspection\" is a fine answer to the question of how the\nproblem was discovered.  I'm just trying to understand the impact of\nthe change.\n\nLikewise, if the problem was discovered using valgrind, \"in order to\nhave cleaner valgrind output so other problems are easier to find\"\nmight be an okay answer to the question of what the benefit of the\npatch is.  (Or it might not --- it depends on the maintainability\nimpact and how much noise is being suppressed.)\n\n[...]\n> +++ b/builtin/cat-file.c\n> @@ -18,6 +18,7 @@ static int cat_one_file(int opt, const char\n> *exp_type, const char *obj_name)\n>      unsigned char sha1[20];\n>      enum object_type type;\n>      char *buf;\n> +    int buf_must_free = 0;\n\nThe usual idiom is to initialize a pointer the current function is\nresponsible for to NULL so it can call free(ptr) unconditionally\nbefore returning.\n\n[...]\n> @@ -68,9 +69,14 @@ static int cat_one_file(int opt, const char\n> *exp_type, const char *obj_name)\n> \n>          if (type == OBJ_BLOB)\n>              return stream_blob_to_fd(1, sha1, NULL, 0);\n> +\n>          buf = read_sha1_file(sha1, &type, &size);\n\nUnrelated change?\n\n> +        buf_must_free = 1;\n> +\n> -        if (!buf)\n> +        if (!buf) {\n> +            free(buf);\n>              die(\"Cannot read object %s\", obj_name);\n\nIs this free() needed?  The program die()s right afterward, which\nwould free all memory automatically as part of exiting.  Tools like\nvalgrind would understand that nothing is wrong since buf is still\nreachable through the stack.\n\n[...]\n> @@ -110,6 +117,10 @@ static int cat_one_file(int opt, const char\n> *exp_type, const char *obj_name)\n>          die(\"git cat-file %s: bad file\", obj_name);\n> \n>      write_or_die(1, buf, size);\n> +\n> +    if (buf_must_free)\n> +        free(buf);\n> +\n>      return 0;\n\nIs this free() needed?  The only caller is cmd_cat_file, which exits\nright away.\n\nHowever, cmd_cat_file (and more importantly, cat_one_file) uses\n'return' to exit instead of calling exit(), so it does create some\nvalgrind noise.  Is that what this is about fixing?\n\nOn the whole, I'm not too excited --- this patch seems to add\ncomplication for no obvious benefit.\n\nHope that helps,\nJonathan\n"},{"id":"252636","messageId":"CAPig+cSnOc42msPYsBSv+PTgFsDyBqeL7yhSutuRQUkezpTdmw@mail.gmail.com","threadId":"38054","inReplyTo":"20141127002417.GT6527@google.com","subject":"Re: [PATCH 1/1] cat-file: fix a memory leak in cat_one_file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-11-27T06:18:30Z","receivedAt":"2014-11-27T06:18:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 26, 2014 at 7:24 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Alexander Kuleshov wrote:\n> [...]\n>> +++ b/builtin/cat-file.c\n>> @@ -68,9 +69,14 @@ static int cat_one_file(int opt, const char\n>> +        buf_must_free = 1;\n>> +\n>> -        if (!buf)\n>> +        if (!buf) {\n>> +            free(buf);\n>>              die(\"Cannot read object %s\", obj_name);\n>\n> Is this free() needed?  The program die()s right afterward, which\n> would free all memory automatically as part of exiting.  Tools like\n> valgrind would understand that nothing is wrong since buf is still\n> reachable through the stack.\n\nMoreover, 'buf' can only be NULL inside the conditional, so the free()\nis pointless.\n"}]}