{"thread":{"id":"22773","subject":"[PATCH] rerere: fix memory leak if rerere images can't be read","startedAt":"2010-02-23T20:11:53Z","lastAt":"2010-02-23T22:53:24Z","messageCount":5,"participants":["Bert Wesarg","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"135456","messageId":"1266955913-4943-1-git-send-email-bert.wesarg@googlemail.com","threadId":"22773","inReplyTo":null,"subject":"[PATCH] rerere: fix memory leak if rerere images can't be read","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2010-02-23T20:11:53Z","receivedAt":"2010-02-23T20:11:53Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>\n---\n rerere.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex d1d3e75..9ca4cb8 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -364,16 +364,17 @@ static int find_conflict(struct string_list *conflict)\n static int merge(const char *name, const char *path)\n {\n \tint ret;\n-\tmmfile_t cur, base, other;\n+\tmmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};\n \tmmbuffer_t result = {NULL, 0};\n \n \tif (handle_file(path, NULL, rerere_path(name, \"thisimage\")) < 0)\n \t\treturn 1;\n \n+\tret = 1;\n \tif (read_mmfile(&cur, rerere_path(name, \"thisimage\")) ||\n \t\t\tread_mmfile(&base, rerere_path(name, \"preimage\")) ||\n \t\t\tread_mmfile(&other, rerere_path(name, \"postimage\")))\n-\t\treturn 1;\n+\t\tgoto out;\n \tret = ll_merge(&result, path, &base, &cur, \"\", &other, \"\", 0);\n \tif (!ret) {\n \t\tFILE *f = fopen(path, \"w\");\n@@ -387,6 +388,7 @@ static int merge(const char *name, const char *path)\n \t\t\t\t     strerror(errno));\n \t}\n \n+out:\n \tfree(cur.ptr);\n \tfree(base.ptr);\n \tfree(other.ptr);\n-- \n1.7.0.500.ge51cb\n"},{"id":"135469","messageId":"alpine.DEB.1.00.1002232225480.3980@intel-tinevez-2-302","threadId":"22773","inReplyTo":"1266955913-4943-1-git-send-email-bert.wesarg@googlemail.com","subject":"Re: [PATCH] rerere: fix memory leak if rerere images can't be read","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-02-23T21:26:40Z","receivedAt":"2010-02-23T21:26:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 23 Feb 2010, Bert Wesarg wrote:\n\n> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>\n> ---\n>  rerere.c |    6 ++++--\n>  1 files changed, 4 insertions(+), 2 deletions(-)\n\nLooks good to me, except...\n\n> diff --git a/rerere.c b/rerere.c\n> index d1d3e75..9ca4cb8 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -364,16 +364,17 @@ static int find_conflict(struct string_list *conflict)\n>  static int merge(const char *name, const char *path)\n>  {\n>  \tint ret;\n> -\tmmfile_t cur, base, other;\n> +\tmmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};\n>  \tmmbuffer_t result = {NULL, 0};\n>  \n>  \tif (handle_file(path, NULL, rerere_path(name, \"thisimage\")) < 0)\n>  \t\treturn 1;\n>  \n> +\tret = 1;\n\nThis initialization can come earlier, at declaration time.\n\n>  \tif (read_mmfile(&cur, rerere_path(name, \"thisimage\")) ||\n>  \t\t\tread_mmfile(&base, rerere_path(name, \"preimage\")) ||\n>  \t\t\tread_mmfile(&other, rerere_path(name, \"postimage\")))\n> -\t\treturn 1;\n> +\t\tgoto out;\n>  \tret = ll_merge(&result, path, &base, &cur, \"\", &other, \"\", 0);\n>  \tif (!ret) {\n>  \t\tFILE *f = fopen(path, \"w\");\n\nCiao,\nDscho\n"},{"id":"135476","messageId":"36ca99e91002231356u189b80ebka7ae8caf89ab9c10@mail.gmail.com","threadId":"22773","inReplyTo":"alpine.DEB.1.00.1002232225480.3980@intel-tinevez-2-302","subject":"Re: [PATCH] rerere: fix memory leak if rerere images can't be read","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2010-02-23T21:56:20Z","receivedAt":"2010-02-23T21:56:20Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Feb 23, 2010 at 22:26, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> Hi,\n>\n> On Tue, 23 Feb 2010, Bert Wesarg wrote:\n>\n>> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>\n>> ---\n>>  rerere.c |    6 ++++--\n>>  1 files changed, 4 insertions(+), 2 deletions(-)\n>\n> Looks good to me, except...\n>\n>> diff --git a/rerere.c b/rerere.c\n>> index d1d3e75..9ca4cb8 100644\n>> --- a/rerere.c\n>> +++ b/rerere.c\n>> @@ -364,16 +364,17 @@ static int find_conflict(struct string_list *conflict)\n>>  static int merge(const char *name, const char *path)\n>>  {\n>>       int ret;\n>> -     mmfile_t cur, base, other;\n>> +     mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};\n>>       mmbuffer_t result = {NULL, 0};\n>>\n>>       if (handle_file(path, NULL, rerere_path(name, \"thisimage\")) < 0)\n>>               return 1;\n>>\n>> +     ret = 1;\n>\n> This initialization can come earlier, at declaration time.\nI thought about it, but I think it is clearer to put just in front of\nthe condition which may fail.\n\nBye,\nBert\n"},{"id":"135482","messageId":"alpine.DEB.1.00.1002232312090.3980@intel-tinevez-2-302","threadId":"22773","inReplyTo":"36ca99e91002231356u189b80ebka7ae8caf89ab9c10@mail.gmail.com","subject":"Re: [PATCH] rerere: fix memory leak if rerere images can't be read","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-02-23T22:12:54Z","receivedAt":"2010-02-23T22:12:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 23 Feb 2010, Bert Wesarg wrote:\n\n> On Tue, Feb 23, 2010 at 22:26, Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n>\n> > On Tue, 23 Feb 2010, Bert Wesarg wrote:\n> >\n> >> Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>\n> >> ---\n> >>  rerere.c |    6 ++++--\n> >>  1 files changed, 4 insertions(+), 2 deletions(-)\n> >\n> > Looks good to me, except...\n> >\n> >> diff --git a/rerere.c b/rerere.c\n> >> index d1d3e75..9ca4cb8 100644\n> >> --- a/rerere.c\n> >> +++ b/rerere.c\n> >> @@ -364,16 +364,17 @@ static int find_conflict(struct string_list *conflict)\n> >>  static int merge(const char *name, const char *path)\n> >>  {\n> >>       int ret;\n> >> -     mmfile_t cur, base, other;\n> >> +     mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};\n> >>       mmbuffer_t result = {NULL, 0};\n> >>\n> >>       if (handle_file(path, NULL, rerere_path(name, \"thisimage\")) < 0)\n> >>               return 1;\n> >>\n> >> +     ret = 1;\n> >\n> > This initialization can come earlier, at declaration time.\n> I thought about it, but I think it is clearer to put just in front of\n> the condition which may fail.\n\nWell, to _this_ developer, it is clearer when a variable has been \ninitialized in any case. No need to think about how it could be used \nuninitialized. But if you insist...\n\nCiao,\nDscho\n"},{"id":"135488","messageId":"7vd3zv7ee3.fsf@alter.siamese.dyndns.org","threadId":"22773","inReplyTo":"36ca99e91002231356u189b80ebka7ae8caf89ab9c10@mail.gmail.com","subject":"Re: [PATCH] rerere: fix memory leak if rerere images can't be read","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-23T22:53:24Z","receivedAt":"2010-02-23T22:53:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bert Wesarg <bert.wesarg@googlemail.com> writes:\n\n> On Tue, Feb 23, 2010 at 22:26, Johannes Schindelin\n>>> diff --git a/rerere.c b/rerere.c\n>>> index d1d3e75..9ca4cb8 100644\n>>> --- a/rerere.c\n>>> +++ b/rerere.c\n>>> @@ -364,16 +364,17 @@ static int find_conflict(struct string_list *conflict)\n>>>  static int merge(const char *name, const char *path)\n>>>  {\n>>>       int ret;\n>>> -     mmfile_t cur, base, other;\n>>> +     mmfile_t cur = {NULL, 0}, base = {NULL, 0}, other = {NULL, 0};\n>>>       mmbuffer_t result = {NULL, 0};\n>>>\n>>>       if (handle_file(path, NULL, rerere_path(name, \"thisimage\")) < 0)\n>>>               return 1;\n>>>\n>>> +     ret = 1;\n>>\n>> This initialization can come earlier, at declaration time.\n\nYes it can, but I do not think it makes it any easier to read.\n\n> I thought about it, but I think it is clearer to put just in front of\n> the condition which may fail.\n\nAnd I do not think yours is much easier to follow, either.\n\nThe function looks like:\n\n\tstatic int merge()\n        {\n        \tint ret; /* uninitialized */\n                ...\n\n\t\tif (something)\n                \treturn 1; /* error */\n\n\t\tif (some thing\n                    that does allocation and is\n                    fairly large)\n\t\t\treturn 1; /* error */\n\t\tret = operation_that_return_status();\n                free resources\n                return ret;\n\t}\n\nIf you initialize ret early, it might make \"ret\" always defined, but it\nalso makes the first \"return 1\" give you \"huh, why not use ret?\".\n\n\tstatic int merge()\n        {\n        \tint ret = 1; /* assume bad things will happen */\n                ...\n\n\t\tif (something)\n                \treturn 1; /* error */\n\n\t\tif (some thing\n                    that does allocation and is\n                    fairly large)\n\t\t\tgoto out; /* error */\n\t\tret = operation_that_return_status();\n\tout:\n                free resources\n                return ret;\n\t}\n\nAlso it makes clearing of assumed error harder to spot.\n\nBert's version is not much better.  That \"set ret to positive before going\nto the exit codepath\" logically belongs to the error case.  IOW:\n\n\tstatic int merge()\n        {\n        \tint ret; /* uninitialized */\n                ...\n\n\t\tif (something)\n                \treturn 1; /* error */\n\n\t\tif (some thing\n                    that does allocation and is\n                    fairly large) {\n\t\t\tret = 1;\n\t\t\tgoto out; /* error */\n\t\t}\n\t\tret = operation_that_return_status();\n\tout:\n                free resources\n                return ret;\n\t}\n\nWe could initialize ret to 0 at the beginning, to signal the people who\nmight be tempted to touch the code later that you are supposed to flip it\nto non-zero when you find an error and jump to out.  An immediate follow\nup to such a change would be to do something like:\n\n\tstatic int merge()\n        {\n        \tint ret = 0; /* no error yet */\n                ...\n\n\t\tif (something) {\n                \tret = 1;\n                        goto out; /* error */\n\t\t}\n\n\t\tif (some thing\n                    that does allocation and is\n                    fairly large) {\n\t\t\tret = 1;\n\t\t\tgoto out; /* error */\n\t\t}\n\t\tret = operation_that_return_status();\n\tout:\n                free resources\n                return ret;\n\t}\n\nbut the first condition hasn't even allocated anything to be freed, so\nthere isn't much point doing this either.\n"}]}