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

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

[Martin Ettl]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

Hi,

i have checked the source base of git with the static code analyis tool cppcheck. It brougt up an issue in file 1.6.3.3/builtin-fast-export.c at line 447.

The tool printed the following waring:

[git-1.6.3.3/builtin-fast-export.c:447]: (error) Resource leak: f

I have attached a patch to resolve this.


Best regards

Ettl Martin

-- 
Neu: GMX Doppel-FLAT mit Internet-Flatrate + Telefon-Flatrate
für nur 19,99 Euro/mtl.!* http://portal.gmx.net/de/go/dsl02

[-- Attachment #2: builtin-fast-export.c.patch --]
[-- Type: text/x-patch, Size: 387 bytes --]

--- 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);
 }
 
 static void import_marks(char *input_file)

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

[Thomas Rast]

[-- Attachment #1: Type: Text/Plain, Size: 880 bytes --]

Hi Martin

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:

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

>  }
>  
>  static void import_marks(char *input_file)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Johannes Schindelin]

Hi,

On Thu, 9 Jul 2009, Thomas Rast wrote:

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

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

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

[Thomas Rast]

[-- Attachment #1: Type: Text/Plain, Size: 918 bytes --]

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.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

[Andreas Ericsson]

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

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

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

Ciao,
Dscho

[PATCH] Fix export_marks() error handling.

[Matthias Andree]

- Don't leak one FILE * on error per export_marks() call. Found with
  cppcheck and reported by Martin Ettl.

- Abort the potentially long for(;idnums.size;) loop on write errors.

- Add a trailing full-stop to error message when fopen() fails.

Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
---
 builtin-fast-export.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 9a8a6fc..6c0956d 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -428,21 +428,30 @@ static void export_marks(char *file)
 	uint32_t mark;
 	struct object_decoration *deco = idnums.hash;
 	FILE *f;
+	int e;
 
 	f = fopen(file, "w");
 	if (!f)
-		error("Unable to open marks file %s for writing", file);
+		error("Unable to open marks file %s for writing.", file);
 
 	for (i = 0; i < idnums.size; i++) {
 		if (deco->base && deco->base->type == 1) {
 			mark = ptr_to_mark(deco->decoration);
-			fprintf(f, ":%"PRIu32" %s\n", mark,
+			e = fprintf(f, ":%"PRIu32" %s\n", mark,
 				sha1_to_hex(deco->base->sha1));
+			if (e < 0) break;
 		}
 		deco++;
 	}
 
-	if (ferror(f) || fclose(f))
+	/* do not optimize the next two lines - they must both be executed in
+	 * this order. || might short-circuit the fclose(), and combining them
+	 * into one statement might reverse the order of execution.
+	 * Also, fflush() may not be sufficient - on some file systems, the
+	 * error is still delayed until the final [f]close().  */
+	e  = ferror(f);
+	e |= fclose(f);
+	if (e)
 		error("Unable to write marks file %s.", file);
 }
 
-- 
1.6.3.3.385.g60647

Re: [PATCH] Fix export_marks() error handling.

[Stephen R. van den Berg]

Matthias Andree wrote:
>+	/* do not optimize the next two lines - they must both be executed in
>+	 * this order. || might short-circuit the fclose(), and combining them
>+	 * into one statement might reverse the order of execution.
>+	 * Also, fflush() may not be sufficient - on some file systems, the
>+	 * error is still delayed until the final [f]close().  */
>+	e  = ferror(f);
>+	e |= fclose(f);
>+	if (e)

The commentary above should be common knowledge for anyone familiar with
ANSI C.  So I'd suggest moving the comments into the description section of
the commit and removing them from the actual code.
-- 
Sincerely,
           Stephen R. van den Berg.

"And now for something *completely* different!"

Re: [PATCH] Fix export_marks() error handling.

[Matthias Andree]

Am 11.07.2009, 11:45 Uhr, schrieb Stephen R. van den Berg <srb@cuci.nl>:

> Matthias Andree wrote:
>> +	/* do not optimize the next two lines - they must both be executed in
>> +	 * this order. || might short-circuit the fclose(), and combining them
>> +	 * into one statement might reverse the order of execution.
>> +	 * Also, fflush() may not be sufficient - on some file systems, the
>> +	 * error is still delayed until the final [f]close().  */
>> +	e  = ferror(f);
>> +	e |= fclose(f);
>> +	if (e)
>
> The commentary above should be common knowledge for anyone familiar with
> ANSI C.  So I'd suggest moving the comments into the description section  
> of
> the commit and removing them from the actual code.

Feel free to do it and submit a patch, I'm not going to invest more time  
into a piece of code that runs seldomly.

-- 
Matthias Andree

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

[Matthias Andree]

Am 09.07.2009, 15:01 Uhr, schrieb Johannes Schindelin  
<Johannes.Schindelin@gmx.de>:

> 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

[PATCH] Fix export_marks() error handling.

[Matthias Andree]

- Don't leak one FILE * on error per export_marks() call. Found with
  cppcheck and reported by Martin Ettl.

- Abort the potentially long for(;idnums.size;) loop on write errors.

- Record error if fprintf() fails for reasons not required to set the
  stream error indicator, such as ENOMEM.

- Add a trailing full-stop to error message when fopen() fails.

Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
---
 builtin-fast-export.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 9a8a6fc..ca19825 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -428,21 +428,27 @@ static void export_marks(char *file)
 	uint32_t mark;
 	struct object_decoration *deco = idnums.hash;
 	FILE *f;
+	int e = 0;
 
 	f = fopen(file, "w");
 	if (!f)
-		error("Unable to open marks file %s for writing", file);
+		error("Unable to open marks file %s for writing.", file);
 
 	for (i = 0; i < idnums.size; i++) {
 		if (deco->base && deco->base->type == 1) {
 			mark = ptr_to_mark(deco->decoration);
-			fprintf(f, ":%"PRIu32" %s\n", mark,
-				sha1_to_hex(deco->base->sha1));
+			if (fprintf(f, ":%"PRIu32" %s\n", mark,
+				sha1_to_hex(deco->base->sha1)) < 0) {
+			    e = 1;
+			    break;
+			}
 		}
 		deco++;
 	}
 
-	if (ferror(f) || fclose(f))
+	e |= ferror(f);
+	e |= fclose(f);
+	if (e)
 		error("Unable to write marks file %s.", file);
 }
 
-- 
1.6.3.3.413.g91e7

Re: [PATCH] Fix export_marks() error handling.

[Junio C Hamano]

Thanks, will apply.