[PATCH] Fix export_marks() error handling.

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

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