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

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

Hi,

On Thu, 9 Jul 2009, Thomas Rast wrote:

Show 8 quoted lines

> Martin Ettl wrote:
> > 
> > I have attached a patch to resolve this.
> 
> Please read Documentation/SubmittingPatches in the source tree.  And
> use git to track git.git!
> 
> As for the actual patch:

Thanks for inlining it and sparing me (and others) the hassle.

Show 14 quoted lines

> > --- git-1.6.3.3/builtin-fast-export.c	2009-06-22 08:24:25.000000000 +0200
> > +++ git-1.6.3.3/builtin-fast-export_new.c	2009-07-09 09:44:28.000000000 +0200
> > @@ -442,8 +442,9 @@ static void export_marks(char *file)
> >  		deco++;
> >  	}
> >  
> > -	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.

Ciao, Dscho