Re: [PATCH] Avoid segfault when passed malformed refspec

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] Avoid segfault when passed malformed refspec

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:09

Sean [off-list ref] writes:
quoted hunk
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 <redacted>
---
 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?

Re: [PATCH] Avoid segfault when passed malformed refspec

From: Sean <hidden>
Date: 2016-06-15 22:44:09

On Fri, 01 Feb 2008 17:03:04 -0800
Junio C Hamano [off-list ref] wrote:
Sean [off-list ref] writes:
quoted
+		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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help