From: Matthias Andree Date: Thu, 09 Jul 2009 13:36:13 GMT Subject: Re: found a resource leak in file builtin-fast-export.c Message-ID: In-Reply-To: Am 09.07.2009, 15:01 Uhr, schrieb Johannes Schindelin : > Hi, > > On Thu, 9 Jul 2009, Thomas Rast wrote: > >> Johannes Schindelin wrote: >> > On Thu, 9 Jul 2009, Thomas Rast wrote: >> > >> > > Martin Ettl wrote: >> > > > - if (ferror(f) || fclose(f)) >> > > > + if (ferror(f)) >> > > > error("Unable to write marks file %s.", file); >> > > > + fclose(f); >> > > >> > > You no longer check the error returned by fclose(). This is >> > > important, because the FILE* API may buffer writes, and a write >> error >> > > may only become apparent when fclose() flushes the file. >> > >> > Indeed. A better fix would be to replace the || by a |, but this >> must be >> > accompanied by a comment so it does not get removed due to overzealous >> > compiler warnings. >> >> Are you allowed to do that? IIRC using | no longer guarantees that >> ferror() is called before fclose(), and my local 'man 3p fclose' says >> that >> >> After the call to fclose(), any use of stream results in >> undefined behavior. > > Good point. So we really need something like > > err = ferror(f); > err |= fclose(f); /* call fclose() even if there was an error */ > if (err) > error... I've made such a patch, to appear soon on the list (sorry for not Cc:'ing it). -- Matthias Andree