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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 9, 2009, 13:01 UTC
Message-ID: <alpine.DEB.1.00.0907091500420.4339@intel-tinevez-2-302>
In-Reply-To: <200907091324.17643.trast@student.ethz.ch>

Hi,

On Thu, 9 Jul 2009, Thomas Rast wrote:

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

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

Ciao, Dscho