Subject: Re: found a resource leak in file builtin-fast-export.c

From: Andreas Ericsson <[email-masked]>
Date: Thu, 09 Jul 2009 13:30:56 +0200

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.
> 

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                   [email-masked]
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.