{"thread":{"id":"36118","subject":"[RFC memory leak?] Minor memory leak fix","startedAt":"2014-03-11T10:45:06Z","lastAt":"2014-03-11T19:42:38Z","messageCount":6,"participants":["Fredrik Gustafsson","Duy Nguyen","René Scharfe","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"236492","messageId":"1394534706-28987-1-git-send-email-iveqy@iveqy.com","threadId":"36118","inReplyTo":null,"subject":"[RFC memory leak?] Minor memory leak fix","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2014-03-11T10:45:06Z","receivedAt":"2014-03-11T10:45:06Z","isPatch":false,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"Strbuf needs to be released even if it's locally declared.\n\nSigned-off-by: Fredrik Gustafsson <iveqy@iveqy.com>\n---\n archive.c | 21 +++++++++++++++------\n 1 file changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 346f3b2..d6d56e6 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -113,6 +113,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \tstruct git_attr_check check[2];\n \tconst char *path_without_prefix;\n \tint err;\n+\tint to_ret = 0;\n \n \targs->convert = 0;\n \tstrbuf_reset(&path);\n@@ -126,8 +127,10 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \n \tsetup_archive_check(check);\n \tif (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {\n-\t\tif (ATTR_TRUE(check[0].value))\n-\t\t\treturn 0;\n+\t\tif (ATTR_TRUE(check[0].value)) {\n+\t\t\tto_ret = 0;\n+\t\t\tgoto cleanup;\n+\t\t}\n \t\targs->convert = ATTR_TRUE(check[1].value);\n \t}\n \n@@ -135,14 +138,20 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \t\tif (args->verbose)\n \t\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n \t\terr = write_entry(args, sha1, path.buf, path.len, mode);\n-\t\tif (err)\n-\t\t\treturn err;\n-\t\treturn (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n+\t\tif (err) {\n+\t\t\tto_ret = err;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t\tto_ret = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n+\t\tgoto cleanup;\n \t}\n \n \tif (args->verbose)\n \t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n-\treturn write_entry(args, sha1, path.buf, path.len, mode);\n+\tto_ret = write_entry(args, sha1, path.buf, path.len, mode);\n+cleanup:\n+\tstrbuf_release(&path);\n+\treturn to_ret;\n }\n \n int write_archive_entries(struct archiver_args *args,\n-- \n1.8.3.1.490.g39d9b24.dirty\n"},{"id":"236498","messageId":"CACsJy8Bp6rpe4hLg0eMBCjPjL5uDwXj-7ZVmZtUZvR26unAuoQ@mail.gmail.com","threadId":"36118","inReplyTo":"1394534706-28987-1-git-send-email-iveqy@iveqy.com","subject":"Re: [RFC memory leak?] Minor memory leak fix","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-11T11:58:11Z","receivedAt":"2014-03-11T11:58:11Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:\n> Strbuf needs to be released even if it's locally declared.\n\n\"path\" is declared static. So yes it's a leak but the leak is minimum.\nYour patch would make more sense if \"static\" is gone and it's leaked\nafter every write_archive_entry call.\n\n>\n> Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>\n> ---\n>  archive.c | 21 +++++++++++++++------\n>  1 file changed, 15 insertions(+), 6 deletions(-)\n>\n> diff --git a/archive.c b/archive.c\n> index 346f3b2..d6d56e6 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -113,6 +113,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>         struct git_attr_check check[2];\n>         const char *path_without_prefix;\n>         int err;\n> +       int to_ret = 0;\n>\n>         args->convert = 0;\n>         strbuf_reset(&path);\n> @@ -126,8 +127,10 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>\n>         setup_archive_check(check);\n>         if (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {\n> -               if (ATTR_TRUE(check[0].value))\n> -                       return 0;\n> +               if (ATTR_TRUE(check[0].value)) {\n> +                       to_ret = 0;\n> +                       goto cleanup;\n> +               }\n\nto_ret is already 0 so I think \"goto cleanup;\" is enough.\n\n>                 args->convert = ATTR_TRUE(check[1].value);\n>         }\n>\n> @@ -135,14 +138,20 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>                 if (args->verbose)\n>                         fprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n>                 err = write_entry(args, sha1, path.buf, path.len, mode);\n> -               if (err)\n> -                       return err;\n> -               return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n> +               if (err) {\n> +                       to_ret = err;\n> +                       goto cleanup;\n> +               }\n> +               to_ret = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n> +               goto cleanup;\n\nMaybe if (err) to_ret = ...; else to_ret = ...; so we only need one\n\"goto cleanup\" statement. Going even further:\n\nto_ret = write_entry(...);\nif (!to_ret) to_ret = (S_ISDIR(...));\ngoto cleanup;\n\n>         }\n>\n>         if (args->verbose)\n>                 fprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n> -       return write_entry(args, sha1, path.buf, path.len, mode);\n> +       to_ret = write_entry(args, sha1, path.buf, path.len, mode);\n> +cleanup:\n> +       strbuf_release(&path);\n> +       return to_ret;\n>  }\n>\n>  int write_archive_entries(struct archiver_args *args,\n> --\n> 1.8.3.1.490.g39d9b24.dirty\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n\n\n-- \nDuy\n"},{"id":"236499","messageId":"1394541371-2960-1-git-send-email-iveqy@iveqy.com","threadId":"36118","inReplyTo":"CACsJy8Bp6rpe4hLg0eMBCjPjL5uDwXj-7ZVmZtUZvR26unAuoQ@mail.gmail.com","subject":"[[RFC memory leak, v.2]] Minor memory leak fix","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2014-03-11T12:36:11Z","receivedAt":"2014-03-11T12:36:11Z","isPatch":false,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"Strbuf needs to be released even if it's locally declared.\n\nSigned-off-by: Fredrik Gustafsson <iveqy@iveqy.com>\n---\n archive.c | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 346f3b2..dfc557d 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -113,6 +113,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \tstruct git_attr_check check[2];\n \tconst char *path_without_prefix;\n \tint err;\n+\tint to_ret = 0;\n \n \targs->convert = 0;\n \tstrbuf_reset(&path);\n@@ -127,22 +128,25 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n \tsetup_archive_check(check);\n \tif (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {\n \t\tif (ATTR_TRUE(check[0].value))\n-\t\t\treturn 0;\n+\t\t\tgoto cleanup;\n \t\targs->convert = ATTR_TRUE(check[1].value);\n \t}\n \n \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\n \t\tif (args->verbose)\n \t\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n-\t\terr = write_entry(args, sha1, path.buf, path.len, mode);\n-\t\tif (err)\n-\t\t\treturn err;\n-\t\treturn (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n+\t\tto_ret = write_entry(args, sha1, path.buf, path.len, mode);\n+\t\tif (!to_ret)\n+\t\t\tto_ret = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n+\t\tgoto cleanup;\n \t}\n \n \tif (args->verbose)\n \t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n-\treturn write_entry(args, sha1, path.buf, path.len, mode);\n+\tto_ret = write_entry(args, sha1, path.buf, path.len, mode);\n+cleanup:\n+\tstrbuf_release(&path);\n+\treturn to_ret;\n }\n \n int write_archive_entries(struct archiver_args *args,\n-- \n1.8.3.1.490.g39d9b24.dirty\n"},{"id":"236500","messageId":"20140311124009.GI14327@paksenarrion.iveqy.com","threadId":"36118","inReplyTo":"CACsJy8Bp6rpe4hLg0eMBCjPjL5uDwXj-7ZVmZtUZvR26unAuoQ@mail.gmail.com","subject":"Re: [RFC memory leak?] Minor memory leak fix","fromName":"Fredrik Gustafsson","fromEmail":"iveqy@iveqy.com","sentAt":"2014-03-11T12:40:09Z","receivedAt":"2014-03-11T12:40:09Z","isPatch":false,"sender":{"key":"iveqy@iveqy.com","avatar":"https://avatars.githubusercontent.com/u/761743?v=4"},"body":"On Tue, Mar 11, 2014 at 06:58:11PM +0700, Duy Nguyen wrote:\n> On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:\n> > Strbuf needs to be released even if it's locally declared.\n> \n> \"path\" is declared static. So yes it's a leak but the leak is minimum.\n> Your patch would make more sense if \"static\" is gone and it's leaked\n> after every write_archive_entry call.\n\nThat's one of the reasons of the RFC. I know Junio thinks that minor\nthings shouldn't be fixed by themselfes because it takes up review\nbandwidth, so it's better to fix them once you touch that part of the\ncode anyway. (At least that's how I've understood him).\n\nThis leak is at about 4.1 kB so it's not huge.\n\n> > +               if (ATTR_TRUE(check[0].value)) {\n> > +                       to_ret = 0;\n> > +                       goto cleanup;\n> > +               }\n> \n> to_ret is already 0 so I think \"goto cleanup;\" is enough.\n\nAgree, fixed in next iteration.\n\n> >                 err = write_entry(args, sha1, path.buf, path.len, mode);\n> > -               if (err)\n> > -                       return err;\n> > -               return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n> > +               if (err) {\n> > +                       to_ret = err;\n> > +                       goto cleanup;\n> > +               }\n> > +               to_ret = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n> > +               goto cleanup;\n> \n> Maybe if (err) to_ret = ...; else to_ret = ...; so we only need one\n> \"goto cleanup\" statement. Going even further:\n> \n> to_ret = write_entry(...);\n> if (!to_ret) to_ret = (S_ISDIR(...));\n> goto cleanup;\n\nAgree, fixed in next iteration.\n\n-- \nMed vänlig hälsning\nFredrik Gustafsson\n\ntel: 0733-608274\ne-post: iveqy@iveqy.com\n"},{"id":"236523","messageId":"531F6697.1030703@web.de","threadId":"36118","inReplyTo":"1394541371-2960-1-git-send-email-iveqy@iveqy.com","subject":"Re: [[RFC memory leak, v.2]] Minor memory leak fix","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-03-11T19:40:07Z","receivedAt":"2014-03-11T19:40:07Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.03.2014 13:36, schrieb Fredrik Gustafsson:\n> Strbuf needs to be released even if it's locally declared.\n>\n> Signed-off-by: Fredrik Gustafsson <iveqy@iveqy.com>\n> ---\n>   archive.c | 16 ++++++++++------\n>   1 file changed, 10 insertions(+), 6 deletions(-)\n>\n> diff --git a/archive.c b/archive.c\n> index 346f3b2..dfc557d 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -113,6 +113,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>   \tstruct git_attr_check check[2];\n>   \tconst char *path_without_prefix;\n>   \tint err;\n> +\tint to_ret = 0;\n>\n>   \targs->convert = 0;\n>   \tstrbuf_reset(&path);\n> @@ -127,22 +128,25 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,\n>   \tsetup_archive_check(check);\n>   \tif (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {\n>   \t\tif (ATTR_TRUE(check[0].value))\n> -\t\t\treturn 0;\n> +\t\t\tgoto cleanup;\n>   \t\targs->convert = ATTR_TRUE(check[1].value);\n>   \t}\n>\n>   \tif (S_ISDIR(mode) || S_ISGITLINK(mode)) {\n>   \t\tif (args->verbose)\n>   \t\t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n> -\t\terr = write_entry(args, sha1, path.buf, path.len, mode);\n> -\t\tif (err)\n> -\t\t\treturn err;\n> -\t\treturn (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n> +\t\tto_ret = write_entry(args, sha1, path.buf, path.len, mode);\n> +\t\tif (!to_ret)\n> +\t\t\tto_ret = (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);\n> +\t\tgoto cleanup;\n\nWhy add to_ret when you can use the existing variable err directly?  Or \nat least remove it when it's not used anymore.\n\n>   \t}\n>\n>   \tif (args->verbose)\n>   \t\tfprintf(stderr, \"%.*s\\n\", (int)path.len, path.buf);\n> -\treturn write_entry(args, sha1, path.buf, path.len, mode);\n> +\tto_ret = write_entry(args, sha1, path.buf, path.len, mode);\n> +cleanup:\n> +\tstrbuf_release(&path);\n> +\treturn to_ret;\n\nIf you free the memory of the strbuf at the end of the function then \nthere is no point in keeping it static anymore.  Growing it to PATH_MAX \nalso doesn't make as much sense as before then.\n\nThe patch makes git archive allocate and free the path strbuf once per \narchive entry, while before it allocated only once per run and left \nfreeing to the OS.  What's the performance impact of this change?\n\n>   }\n>\n>   int write_archive_entries(struct archiver_args *args,\n>\n"},{"id":"236524","messageId":"xmqqmwgwpln5.fsf@gitster.dls.corp.google.com","threadId":"36118","inReplyTo":"20140311124009.GI14327@paksenarrion.iveqy.com","subject":"Re: [RFC memory leak?] Minor memory leak fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-11T19:42:38Z","receivedAt":"2014-03-11T19:42:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fredrik Gustafsson <iveqy@iveqy.com> writes:\n\n> On Tue, Mar 11, 2014 at 06:58:11PM +0700, Duy Nguyen wrote:\n>> On Tue, Mar 11, 2014 at 5:45 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:\n>> > Strbuf needs to be released even if it's locally declared.\n>> \n>> \"path\" is declared static. So yes it's a leak but the leak is minimum.\n>> Your patch would make more sense if \"static\" is gone and it's leaked\n>> after every write_archive_entry call.\n>\n> That's one of the reasons of the RFC. I know Junio thinks that minor\n> things shouldn't be fixed by themselfes because it takes up review\n> bandwidth, so it's better to fix them once you touch that part of the\n> code anyway. (At least that's how I've understood him).\n\nYes, but I at the same time think this \"static struct strbuf\" is a\nclear statement by the original author that this is not a leak\nper-se.  The trade-off, if I am reading the code right, is between\nkeeping a piece of memory that is large enough to hold the longest\npathname until exit() vs saving repeated allocations and frees for\neach of the thousands of paths in the resulting archive.\n\nI tend to think the original strikes a better balance between the\ntwo.\n"}]}