threads / patch / 11819

patchAvoid segfault when passed malformed refspec

Subject: [PATCH] Avoid segfault when passed malformed refspec

## tl;dr

3 messages between Feb 2, 2008 and Feb 2, 2008. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Sean· Feb 2, 2008, 00:00 UTC · lore

A refspec typo can cause a Null-pointer dereference and segmentation fault. For instance, the space before the colon in the following example results in a segfault:

   $ git fetch ../repo  refs/heads/* :refs/heads/*
   Segmentation fault (core dumped)

To avoid the segfault, set an empty refspec destination string if one isn't found by parsing.

Signed-off-by: Sean Estabrooks <seanlkml@sympatico.ca>
---
 remote.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Show changes to remote.c +2 −0

diff --git a/remote.c b/remote.c
index 0e00680..414c73a 100644
--- a/remote.c
+++ b/remote.c
@@ -336,6 +336,8 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
 			ep = gp;
 		}
 		rs[i].src = xstrndup(sp, ep - sp);
+		if (!rs[i].dst)
+			rs[i].dst = xstrdup("");
 	}
 	return rs;
 }

-- 
1.5.4.rc5.20.g4b806

Junio C Hamano· Feb 2, 2008, 01:03 UTC · re: Sean · lore

Re: [PATCH] Avoid segfault when passed malformed refspec

Sean <seanlkml@sympatico.ca> writes:

Show 28 quoted lines

> A refspec typo can cause a Null-pointer dereference and segmentation
> fault.  For instance, the space before the colon in the following
> example results in a segfault:
>
>    $ git fetch ../repo  refs/heads/* :refs/heads/*
>    Segmentation fault (core dumped)
>
> To avoid the segfault, set an empty refspec destination string
> if one isn't found by parsing.
>
> Signed-off-by: Sean Estabrooks <seanlkml@sympatico.ca>
> ---
>  remote.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 0e00680..414c73a 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -336,6 +336,8 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)
>  			ep = gp;
>  		}
>  		rs[i].src = xstrndup(sp, ep - sp);
> +		if (!rs[i].dst)
> +			rs[i].dst = xstrdup("");
>  	}
>  	return rs;
>  }

I haven't followed the codepath carefully before responding, it feels like sweeping the breakage under the carpet, without fixing the real issue.

If the problem is a badly formatted input, shouldn't the code die loudly with diagnostic message, instead of pretending as if the user said something different (and sensible), especially without telling the user that that is what the code is doing?

Sean· Feb 2, 2008, 01:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] Avoid segfault when passed malformed refspec

On Fri, 01 Feb 2008 17:03:04 -0800 Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Sean <seanlkml@sympatico.ca> writes:
> >
> > +		if (!rs[i].dst)
> > +			rs[i].dst = xstrdup("");
>
> I haven't followed the codepath carefully before responding, it
> feels like sweeping the breakage under the carpet, without
> fixing the real issue.
> 
> If the problem is a badly formatted input, shouldn't the code
> die loudly with diagnostic message, instead of pretending as if
> the user said something different (and sensible), especially
> without telling the user that that is what the code is doing?
>

Hey Junio,

You're probably right. It seemed like a reasonable fix at the time without having to understand the code too deeply. With the above patch, the code does complain to the user:

  $ git-fetch ../repo refs/heads/* :refs/heads/* 
  fatal: * refusing to create funny ref 'floop' locally

But surely a better error could be shown if fetch is made to squawk whenever a destination ref is omitted. I just wasn't confident enough in the code, or in knowing what refspec rules are universally applicable.

Sean