{"thread":{"id":"20062","subject":"found a resource leak in file builtin-fast-export.c","startedAt":"2009-07-09T07:57:28Z","lastAt":"2009-07-13T08:01:57Z","messageCount":10,"participants":["Martin Ettl","Thomas Rast","Johannes Schindelin","Andreas Ericsson","Matthias Andree","Stephen R. van den Berg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"117654","messageId":"20090709075728.137880@gmx.net","threadId":"20062","inReplyTo":null,"subject":"found a resource leak in file builtin-fast-export.c","fromName":"Martin Ettl","fromEmail":"ettl.martin@gmx.de","sentAt":"2009-07-09T07:57:28Z","receivedAt":"2009-07-09T07:57:28Z","isPatch":false,"sender":{"key":"ettl.martin@gmx.de","avatar":null},"body":"Hi,\n\ni have checked the source base of git with the static code analyis tool cppcheck. It brougt up an issue in file 1.6.3.3/builtin-fast-export.c at line 447.\n\nThe tool printed the following waring:\n\n[git-1.6.3.3/builtin-fast-export.c:447]: (error) Resource leak: f\n\nI have attached a patch to resolve this.\n\n\nBest regards\n\nEttl Martin\n\n-- \nNeu: GMX Doppel-FLAT mit Internet-Flatrate + Telefon-Flatrate\nfür nur 19,99 Euro/mtl.!* http://portal.gmx.net/de/go/dsl02\n\n\n--- git-1.6.3.3/builtin-fast-export.c\t2009-06-22 08:24:25.000000000 +0200\n+++ git-1.6.3.3/builtin-fast-export_new.c\t2009-07-09 09:44:28.000000000 +0200\n@@ -442,8 +442,9 @@ static void export_marks(char *file)\n \t\tdeco++;\n \t}\n \n-\tif (ferror(f) || fclose(f))\n+\tif (ferror(f))\n \t\terror(\"Unable to write marks file %s.\", file);\n+ \tfclose(f);\n }\n \n static void import_marks(char *input_file)\n"},{"id":"117658","messageId":"200907091031.43494.trast@student.ethz.ch","threadId":"20062","inReplyTo":"20090709075728.137880@gmx.net","subject":"Re: found a resource leak in file builtin-fast-export.c","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-07-09T08:31:37Z","receivedAt":"2009-07-09T08:31:37Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Hi Martin\n\nMartin Ettl wrote:\n> \n> I have attached a patch to resolve this.\n\nPlease read Documentation/SubmittingPatches in the source tree. And\nuse git to track git.git!\n\nAs for the actual patch:\n\n> --- git-1.6.3.3/builtin-fast-export.c\t2009-06-22 08:24:25.000000000 +0200\n> +++ git-1.6.3.3/builtin-fast-export_new.c\t2009-07-09 09:44:28.000000000 +0200\n> @@ -442,8 +442,9 @@ static void export_marks(char *file)\n> \t\tdeco++;\n> \t}\n> \n> -\tif (ferror(f) || fclose(f))\n> +\tif (ferror(f))\n> \t\terror(\"Unable to write marks file %s.\", file);\n> + \tfclose(f);\n\nYou no longer check the error returned by fclose(). This is\nimportant, because the FILE* API may buffer writes, and a write error\nmay only become apparent when fclose() flushes the file.\n\n> }\n> \n> static void import_marks(char *input_file)\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"117681","messageId":"alpine.DEB.1.00.0907091302520.4339@intel-tinevez-2-302","threadId":"20062","inReplyTo":"200907091031.43494.trast@student.ethz.ch","subject":"Re: found a resource leak in file builtin-fast-export.c","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-09T11:04:19Z","receivedAt":"2009-07-09T11:04:19Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 9 Jul 2009, Thomas Rast wrote:\n\n> Martin Ettl wrote:\n> > \n> > I have attached a patch to resolve this.\n> \n> Please read Documentation/SubmittingPatches in the source tree. And\n> use git to track git.git!\n> \n> As for the actual patch:\n\nThanks for inlining it and sparing me (and others) the hassle.\n\n> > --- git-1.6.3.3/builtin-fast-export.c\t2009-06-22 08:24:25.000000000 +0200\n> > +++ git-1.6.3.3/builtin-fast-export_new.c\t2009-07-09 09:44:28.000000000 +0200\n> > @@ -442,8 +442,9 @@ static void export_marks(char *file)\n> > \t\tdeco++;\n> > \t}\n> > \n> > -\tif (ferror(f) || fclose(f))\n> > +\tif (ferror(f))\n> > \t\terror(\"Unable to write marks file %s.\", file);\n> > + \tfclose(f);\n> \n> You no longer check the error returned by fclose(). This is\n> important, because the FILE* API may buffer writes, and a write error\n> may only become apparent when fclose() flushes the file.\n\nIndeed. A better fix would be to replace the || by a |, but this must be \naccompanied by a comment so it does not get removed due to overzealous \ncompiler warnings.\n\nCiao,\nDscho\n"},{"id":"117682","messageId":"200907091324.17643.trast@student.ethz.ch","threadId":"20062","inReplyTo":"alpine.DEB.1.00.0907091302520.4339@intel-tinevez-2-302","subject":"Re: found a resource leak in file builtin-fast-export.c","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-07-09T11:24:14Z","receivedAt":"2009-07-09T11:24:14Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Johannes Schindelin wrote:\n> On Thu, 9 Jul 2009, Thomas Rast wrote:\n> \n> > Martin Ettl wrote:\n> > > -\tif (ferror(f) || fclose(f))\n> > > +\tif (ferror(f))\n> > > \t\terror(\"Unable to write marks file %s.\", file);\n> > > + \tfclose(f);\n> > \n> > You no longer check the error returned by fclose(). This is\n> > important, because the FILE* API may buffer writes, and a write error\n> > may only become apparent when fclose() flushes the file.\n> \n> Indeed. A better fix would be to replace the || by a |, but this must be \n> accompanied by a comment so it does not get removed due to overzealous \n> compiler warnings.\n\nAre you allowed to do that? IIRC using | no longer guarantees that\nferror() is called before fclose(), and my local 'man 3p fclose' says\nthat\n\n After the call to fclose(), any use of stream results in\n undefined behavior.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"117683","messageId":"4A55D4F0.5020002@op5.se","threadId":"20062","inReplyTo":"200907091324.17643.trast@student.ethz.ch","subject":"Re: found a resource leak in file builtin-fast-export.c","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2009-07-09T11:30:56Z","receivedAt":"2009-07-09T11:30:56Z","isPatch":false,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"Thomas Rast wrote:\n> Johannes Schindelin wrote:\n>> On Thu, 9 Jul 2009, Thomas Rast wrote:\n>>\n>>> Martin Ettl wrote:\n>>>> -\tif (ferror(f) || fclose(f))\n>>>> +\tif (ferror(f))\n>>>> \t\terror(\"Unable to write marks file %s.\", file);\n>>>> + \tfclose(f);\n>>> You no longer check the error returned by fclose(). This is\n>>> important, because the FILE* API may buffer writes, and a write error\n>>> may only become apparent when fclose() flushes the file.\n>> Indeed. A better fix would be to replace the || by a |, but this must be \n>> accompanied by a comment so it does not get removed due to overzealous \n>> compiler warnings.\n> \n> Are you allowed to do that? IIRC using | no longer guarantees that\n> ferror() is called before fclose(), and my local 'man 3p fclose' says\n> that\n> \n> After the call to fclose(), any use of stream results in\n> undefined behavior.\n> \n\nA more important question; Do we really care? I haven't looked closely\nat the code, but afair the marks file is written once per invocation,\nso leaking its file descriptor sounds like something we won't really\nbother about.\n\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"117687","messageId":"alpine.DEB.1.00.0907091500420.4339@intel-tinevez-2-302","threadId":"20062","inReplyTo":"200907091324.17643.trast@student.ethz.ch","subject":"Re: found a resource leak in file builtin-fast-export.c","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-09T13:01:46Z","receivedAt":"2009-07-09T13:01:46Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 9 Jul 2009, Thomas Rast wrote:\n\n> Johannes Schindelin wrote:\n> > On Thu, 9 Jul 2009, Thomas Rast wrote:\n> > \n> > > Martin Ettl wrote:\n> > > > -\tif (ferror(f) || fclose(f))\n> > > > +\tif (ferror(f))\n> > > > \t\terror(\"Unable to write marks file %s.\", file);\n> > > > + \tfclose(f);\n> > > \n> > > You no longer check the error returned by fclose(). This is\n> > > important, because the FILE* API may buffer writes, and a write error\n> > > may only become apparent when fclose() flushes the file.\n> > \n> > Indeed. A better fix would be to replace the || by a |, but this must be \n> > accompanied by a comment so it does not get removed due to overzealous \n> > compiler warnings.\n> \n> Are you allowed to do that? IIRC using | no longer guarantees that\n> ferror() is called before fclose(), and my local 'man 3p fclose' says\n> that\n> \n> After the call to fclose(), any use of stream results in\n> undefined behavior.\n\nGood point. So we really need something like\n\n\terr = ferror(f);\n\terr |= fclose(f); /* call fclose() even if there was an error */\n\tif (err)\n\t\terror...\n\nCiao,\nDscho\n"},{"id":"117689","messageId":"1247146081-4692-1-git-send-email-matthias.andree@gmx.de","threadId":"20062","inReplyTo":"alpine.DEB.1.00.0907091500420.4339@intel-tinevez-2-302","subject":"[PATCH] Fix export_marks() error handling.","fromName":"Matthias Andree","fromEmail":"matthias.andree@gmx.de","sentAt":"2009-07-09T13:28:01Z","receivedAt":"2009-07-09T13:28:01Z","isPatch":true,"sender":{"key":"matthias.andree@gmx.de","avatar":null},"body":"- Don't leak one FILE * on error per export_marks() call. Found with\n cppcheck and reported by Martin Ettl.\n\n- Abort the potentially long for(;idnums.size;) loop on write errors.\n\n- Add a trailing full-stop to error message when fopen() fails.\n\nSigned-off-by: Matthias Andree \n---\n builtin-fast-export.c | 15 ++++++++++++---\n 1 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-fast-export.c b/builtin-fast-export.c\nindex 9a8a6fc..6c0956d 100644\n--- a/builtin-fast-export.c\n+++ b/builtin-fast-export.c\n@@ -428,21 +428,30 @@ static void export_marks(char *file)\n \tuint32_t mark;\n \tstruct object_decoration *deco = idnums.hash;\n \tFILE *f;\n+\tint e;\n \n \tf = fopen(file, \"w\");\n \tif (!f)\n-\t\terror(\"Unable to open marks file %s for writing\", file);\n+\t\terror(\"Unable to open marks file %s for writing.\", file);\n \n \tfor (i = 0; i < idnums.size; i++) {\n \t\tif (deco->base && deco->base->type == 1) {\n \t\t\tmark = ptr_to_mark(deco->decoration);\n-\t\t\tfprintf(f, \":%\"PRIu32\" %s\\n\", mark,\n+\t\t\te = fprintf(f, \":%\"PRIu32\" %s\\n\", mark,\n \t\t\t\tsha1_to_hex(deco->base->sha1));\n+\t\t\tif (e < 0) break;\n \t\t}\n \t\tdeco++;\n \t}\n \n-\tif (ferror(f) || fclose(f))\n+\t/* do not optimize the next two lines - they must both be executed in\n+\t * this order. || might short-circuit the fclose(), and combining them\n+\t * into one statement might reverse the order of execution.\n+\t * Also, fflush() may not be sufficient - on some file systems, the\n+\t * error is still delayed until the final [f]close(). */\n+\te = ferror(f);\n+\te |= fclose(f);\n+\tif (e)\n \t\terror(\"Unable to write marks file %s.\", file);\n }\n \n-- \n1.6.3.3.385.g60647\n"},{"id":"117688","messageId":"op.uwsyqnwt1e62zd@balu.cs.uni-paderborn.de","threadId":"20062","inReplyTo":"alpine.DEB.1.00.0907091500420.4339@intel-tinevez-2-302","subject":"Re: found a resource leak in file builtin-fast-export.c","fromName":"Matthias Andree","fromEmail":"matthias.andree@gmx.de","sentAt":"2009-07-09T13:36:13Z","receivedAt":"2009-07-09T13:36:13Z","isPatch":false,"sender":{"key":"matthias.andree@gmx.de","avatar":null},"body":"Am 09.07.2009, 15:01 Uhr, schrieb Johannes Schindelin \n:\n\n> Hi,\n>\n> On Thu, 9 Jul 2009, Thomas Rast wrote:\n>\n>> Johannes Schindelin wrote:\n>> > On Thu, 9 Jul 2009, Thomas Rast wrote:\n>> >\n>> > > Martin Ettl wrote:\n>> > > > -\tif (ferror(f) || fclose(f))\n>> > > > +\tif (ferror(f))\n>> > > > \t\terror(\"Unable to write marks file %s.\", file);\n>> > > > + \tfclose(f);\n>> > >\n>> > > You no longer check the error returned by fclose(). This is\n>> > > important, because the FILE* API may buffer writes, and a write \n>> error\n>> > > may only become apparent when fclose() flushes the file.\n>> >\n>> > Indeed. A better fix would be to replace the || by a |, but this \n>> must be\n>> > accompanied by a comment so it does not get removed due to overzealous\n>> > compiler warnings.\n>>\n>> Are you allowed to do that? IIRC using | no longer guarantees that\n>> ferror() is called before fclose(), and my local 'man 3p fclose' says\n>> that\n>>\n>> After the call to fclose(), any use of stream results in\n>> undefined behavior.\n>\n> Good point. So we really need something like\n>\n> \terr = ferror(f);\n> \terr |= fclose(f); /* call fclose() even if there was an error */\n> \tif (err)\n> \t\terror...\n\nI've made such a patch, to appear soon on the list (sorry for not Cc:'ing \nit).\n\n-- \nMatthias Andree\n"},{"id":"117821","messageId":"20090711094546.GA12399@cuci.nl","threadId":"20062","inReplyTo":"1247146081-4692-1-git-send-email-matthias.andree@gmx.de","subject":"Re: [PATCH] Fix export_marks() error handling.","fromName":"Stephen R. van den Berg","fromEmail":"srb@cuci.nl","sentAt":"2009-07-11T09:45:46Z","receivedAt":"2009-07-11T09:45:46Z","isPatch":true,"sender":{"key":"srb@cuci.nl","avatar":"https://gravatar.com/avatar/f75389059e827634d38e9df2a9b6ecbd50028b5a454442efa1c7205b7ff29c6a?d=mp&s=160"},"body":"Matthias Andree wrote:\n>+\t/* do not optimize the next two lines - they must both be executed in\n>+\t * this order. || might short-circuit the fclose(), and combining them\n>+\t * into one statement might reverse the order of execution.\n>+\t * Also, fflush() may not be sufficient - on some file systems, the\n>+\t * error is still delayed until the final [f]close(). */\n>+\te = ferror(f);\n>+\te |= fclose(f);\n>+\tif (e)\n\nThe commentary above should be common knowledge for anyone familiar with\nANSI C. So I'd suggest moving the comments into the description section of\nthe commit and removing them from the actual code.\n-- \nSincerely,\n Stephen R. van den Berg.\n\n\"And now for something *completely* different!\"\n"},{"id":"117887","messageId":"op.uwzxxj2m1e62zd@merlin.emma.line.org","threadId":"20062","inReplyTo":"20090711094546.GA12399@cuci.nl","subject":"Re: [PATCH] Fix export_marks() error handling.","fromName":"Matthias Andree","fromEmail":"matthias.andree@gmx.de","sentAt":"2009-07-13T08:01:57Z","receivedAt":"2009-07-13T08:01:57Z","isPatch":true,"sender":{"key":"matthias.andree@gmx.de","avatar":null},"body":"Am 11.07.2009, 11:45 Uhr, schrieb Stephen R. van den Berg :\n\n> Matthias Andree wrote:\n>> +\t/* do not optimize the next two lines - they must both be executed in\n>> +\t * this order. || might short-circuit the fclose(), and combining them\n>> +\t * into one statement might reverse the order of execution.\n>> +\t * Also, fflush() may not be sufficient - on some file systems, the\n>> +\t * error is still delayed until the final [f]close(). */\n>> +\te = ferror(f);\n>> +\te |= fclose(f);\n>> +\tif (e)\n>\n> The commentary above should be common knowledge for anyone familiar with\n> ANSI C. So I'd suggest moving the comments into the description section \n> of\n> the commit and removing them from the actual code.\n\nFeel free to do it and submit a patch, I'm not going to invest more time \ninto a piece of code that runs seldomly.\n\n-- \nMatthias Andree\n"}]}