[PATCH] Fix segfault in fast-export

Mike Mueller· Jan 7, 2010, 03:58 UTC · lore

Hi all,

I'm working on a C++ static analyzer (Vigilant Sentry), and git is one of my test subjects. In git-1.6.6, I found a crash in the fast-export command:

The problem is in builtin-fast-export.c, function export_marks:

    f = fopen(file, "w");
    if (!f)
        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);
            if (fprintf(f, ":%"PRIu32" %s\n", mark,
                sha1_to_hex(deco->base->sha1)) < 0) {
                e = 1;
                break;
            }
        }
        deco++;
    }
   
    e |= ferror(f);
    e |= fclose(f);

If fopen() fails, the error message is printed, but the function
doesn't exit.  The subsequent calls to fprintf and/or ferror will
fail because f is NULL.  A simple way to reproduce is to export
to a path you don't have write access to:
   
    $ git fast-export --export-marks=/foo
    error: Unable to open marks file /foo for writing.
    Segmentation fault (core dumped)

I've attached a trivial patch that calls die_errno instead of error, so the program exits if f is NULL.

Regards, Mike

-- 
Mike Mueller
mmueller@vigilantsw.com

http://www.vigilantsw.com/


diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index b0a4029..963e89b 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -503,7 +503,7 @@ static void export_marks(char *file)
 
 	f = fopen(file, "w");
 	if (!f)
-		error("Unable to open marks file %s for writing.", file);
+		die_errno("Unable to open marks file %s for writing", file);
 
 	for (i = 0; i < idnums.size; i++) {
 		if (deco->base && deco->base->type == 1) {

Heiko Voigt· Jan 18, 2010, 17:44 UTC · re: Mike Mueller · lore

Re: [PATCH] Fix segfault in fast-export

Hi,

if want your change included in git you probably want to CC: Junio and inline your patch so its easier to comment. Please see the file Documentation/SubmittingPatches for tips on how to do it with your mailer.

cheers Heiko

P.S.: and include a commit message in your patch

On Wed, Jan 06, 2010 at 10:58:39PM -0500, Mike Mueller wrote:

Show 47 quoted lines

> Hi all,
> 
> I'm working on a C++ static analyzer (Vigilant Sentry), and git
> is one of my test subjects.  In git-1.6.6, I found a crash in the
> fast-export command:
> 
> The problem is in builtin-fast-export.c, function export_marks:
> 
>     f = fopen(file, "w");
>     if (!f)
>         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);
>             if (fprintf(f, ":%"PRIu32" %s\n", mark,
>                 sha1_to_hex(deco->base->sha1)) < 0) {
>                 e = 1;
>                 break;
>             }
>         }
>         deco++;
>     }
>    
>     e |= ferror(f);
>     e |= fclose(f);
> 
> If fopen() fails, the error message is printed, but the function
> doesn't exit.  The subsequent calls to fprintf and/or ferror will
> fail because f is NULL.  A simple way to reproduce is to export
> to a path you don't have write access to:
>    
>     $ git fast-export --export-marks=/foo
>     error: Unable to open marks file /foo for writing.
>     Segmentation fault (core dumped)
> 
> I've attached a trivial patch that calls die_errno instead of
> error, so the program exits if f is NULL.
> 
> Regards,
> Mike
> 
> -- 
> Mike Mueller
> mmueller@vigilantsw.com
> 
> http://www.vigilantsw.com/

Show 13 quoted lines

> diff --git a/builtin-fast-export.c b/builtin-fast-export.c
> index b0a4029..963e89b 100644
> --- a/builtin-fast-export.c
> +++ b/builtin-fast-export.c
> @@ -503,7 +503,7 @@ static void export_marks(char *file)
>  
>  	f = fopen(file, "w");
>  	if (!f)
> -		error("Unable to open marks file %s for writing.", file);
> +		die_errno("Unable to open marks file %s for writing", file);
>  
>  	for (i = 0; i < idnums.size; i++) {
>  		if (deco->base && deco->base->type == 1) {

patchFix segfault in fast-export