Re: found a resource leak in file builtin-fast-export.c
- From
Andreas Ericsson <ae@op5.se>
- Date
- Jul 9, 2009, 11:30 UTC
- Message-ID
- <4A55D4F0.5020002@op5.se>
- In-Reply-To
- <200907091324.17643.trast@student.ethz.ch>
Thomas Rast wrote:
Show 22 quoted lines
> 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.
> A more important question; Do we really care? I haven't looked closely at the code, but afair the marks file is written once per invocation, so leaking its file descriptor sounds like something we won't really bother about.
-- Andreas Ericsson andreas.ericsson@op5.se OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231 Considering the successes of the wars on alcohol, poverty, drugs and terror, I think we should give some serious thought to declaring war on peace.