[PATCH] sha1_name: fix segfault caused by invalid index access

[PATCH] sha1_name: fix segfault caused by invalid index access

[Markus Heidelberg]

It can be reproduced in a bare repository with
    $ git show :anyfile

I didn't find a recipe for reliably reproducing it in a repository with
working tree, it happened depending on the filename and the repository.
    $ git show :nonexistentfile

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---

It seemed to happen more likely with high letters (x, y, z) as the first
character of the filename. This always worked for me:
    $ git show :z
But I found this to be too strange to be added to the commit message.

The affected code path was introduced by commit 009fee477 (Detailed diagnosis
when parsing an object name fails., 2009-12-07).

 sha1_name.c |   32 ++++++++++++++++++--------------
 1 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 43884c6..bf92417 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -992,13 +992,15 @@ static void diagnose_invalid_index_path(int stage,
 	pos = cache_name_pos(filename, namelen);
 	if (pos < 0)
 		pos = -pos - 1;
-	ce = active_cache[pos];
-	if (ce_namelen(ce) == namelen &&
-	    !memcmp(ce->name, filename, namelen))
-		die("Path '%s' is in the index, but not at stage %d.\n"
-		    "Did you mean ':%d:%s'?",
-		    filename, stage,
-		    ce_stage(ce), filename);
+	if (pos < active_nr) {
+		ce = active_cache[pos];
+		if (ce_namelen(ce) == namelen &&
+		    !memcmp(ce->name, filename, namelen))
+			die("Path '%s' is in the index, but not at stage %d.\n"
+			    "Did you mean ':%d:%s'?",
+			    filename, stage,
+			    ce_stage(ce), filename);
+	}
 
 	/* Confusion between relative and absolute filenames? */
 	fullnamelen = namelen + strlen(prefix);
@@ -1008,13 +1010,15 @@ static void diagnose_invalid_index_path(int stage,
 	pos = cache_name_pos(fullname, fullnamelen);
 	if (pos < 0)
 		pos = -pos - 1;
-	ce = active_cache[pos];
-	if (ce_namelen(ce) == fullnamelen &&
-	    !memcmp(ce->name, fullname, fullnamelen))
-		die("Path '%s' is in the index, but not '%s'.\n"
-		    "Did you mean ':%d:%s'?",
-		    fullname, filename,
-		    ce_stage(ce), fullname);
+	if (pos < active_nr) {
+		ce = active_cache[pos];
+		if (ce_namelen(ce) == fullnamelen &&
+		    !memcmp(ce->name, fullname, fullnamelen))
+			die("Path '%s' is in the index, but not '%s'.\n"
+			    "Did you mean ':%d:%s'?",
+			    fullname, filename,
+			    ce_stage(ce), fullname);
+	}
 
 	if (!lstat(filename, &st))
 		die("Path '%s' exists on disk, but not in the index.", filename);
-- 
1.7.0.97.g2d6a2

Re: [PATCH] sha1_name: fix segfault caused by invalid index access

[Matthieu Moy]

Markus Heidelberg <markus.heidelberg@web.de> writes:

> I didn't find a recipe for reliably reproducing it in a repository with
> working tree, it happened depending on the filename and the repository.
>     $ git show :nonexistentfile

> It seemed to happen more likely with high letters (x, y, z) as the first
> character of the filename. This always worked for me:
>     $ git show :z

It happens when you ask for a filename that is after the last index
entry by alphabetical order, yes. pos will contain an index which is
after the previous entry, that is, after the last entry. And then, the
active_cache[pos] crashes.

> The affected code path was introduced by commit 009fee477 (Detailed diagnosis
> when parsing an object name fails., 2009-12-07).

Yes, my bad :-(. Thanks for the report and the fix :-).

>  	pos = cache_name_pos(filename, namelen);
>  	if (pos < 0)
>  		pos = -pos - 1;

Actually, if pos < 0, then cache_name_pos didn't find the entry,
and we shouldn't try any complex thing to find out.

A simpler fix is comming in a separate email. I'm still not familiar
enough with the index to be 100% confident, but it should do the same
as yours in a much simpler way. Reviews welcome.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH] sha1_name: fix segfault caused by invalid index access

[Matthieu Moy]

009fee477 (Detailed diagnosis when parsing an object name fails,
2009-12-07) introduced some invalid index access, inspired by the code of
get_sha1_with_mode_1, which loops over the index entries having the same
name. In the diagnosis, we just want to find whether one entry with the
name is in the index, which is the case iff cache_name_pos's return value
is positive.

Trying anything complex on negative value is not only useless, but also
buggy here, since pos could end up being greater than active_nr, causing
a segfault in active_cache[pos]. This is always the case in bare
repositories, and happens when calling "git show :inexistant" if
"inexistant" is greater than the last index entry in alphabetical order.

Bug report and initial fix by Markus Heidelberg
<markus.heidelberg@web.de>.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 sha1_name.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 43884c6..fbbe3b4 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -990,15 +990,13 @@ static void diagnose_invalid_index_path(int stage,
 
 	/* Wrong stage number? */
 	pos = cache_name_pos(filename, namelen);
-	if (pos < 0)
-		pos = -pos - 1;
-	ce = active_cache[pos];
-	if (ce_namelen(ce) == namelen &&
-	    !memcmp(ce->name, filename, namelen))
+	if (pos >= 0) {
+		ce = active_cache[pos];
 		die("Path '%s' is in the index, but not at stage %d.\n"
 		    "Did you mean ':%d:%s'?",
 		    filename, stage,
 		    ce_stage(ce), filename);
+	}
 
 	/* Confusion between relative and absolute filenames? */
 	fullnamelen = namelen + strlen(prefix);
@@ -1006,15 +1004,13 @@ static void diagnose_invalid_index_path(int stage,
 	strcpy(fullname, prefix);
 	strcat(fullname, filename);
 	pos = cache_name_pos(fullname, fullnamelen);
-	if (pos < 0)
-		pos = -pos - 1;
-	ce = active_cache[pos];
-	if (ce_namelen(ce) == fullnamelen &&
-	    !memcmp(ce->name, fullname, fullnamelen))
+	if (pos >= 0) {
+		ce = active_cache[pos];
 		die("Path '%s' is in the index, but not '%s'.\n"
 		    "Did you mean ':%d:%s'?",
 		    fullname, filename,
 		    ce_stage(ce), fullname);
+	}
 
 	if (!lstat(filename, &st))
 		die("Path '%s' exists on disk, but not in the index.", filename);
-- 
1.7.0.231.g97960.dirty

Re: [PATCH] sha1_name: fix segfault caused by invalid index access

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> -	if (pos < 0)
> -		pos = -pos - 1;
> -	ce = active_cache[pos];
> -	if (ce_namelen(ce) == namelen &&
> -	    !memcmp(ce->name, filename, namelen))
> +	if (pos >= 0) {
> +		ce = active_cache[pos];

A positive return value of cache_name_pos() is "I found a merged entry
with that name at this index" while a negative is "I would insert at this
index if you give me a merged entry with that name".

The latter is what the name comparison logic is about.  The caller asked
for filename, and the return value may point at an existing entry (in
which case you do not even need to memcmp(), but it doesn't hurt and
simplifies the code).  The nagative one with compensation could be
pointing at an unrelated entry, or an unmerged entry with filename you
asked, which sorts higher than a merged entry with the same name.

Re: [PATCH] sha1_name: fix segfault caused by invalid index access

[Jeff King]

On Sun, Feb 28, 2010 at 04:49:15PM +0100, Markus Heidelberg wrote:

> It can be reproduced in a bare repository with
>     $ git show :anyfile
> 
> I didn't find a recipe for reliably reproducing it in a repository with
> working tree, it happened depending on the filename and the repository.
>     $ git show :nonexistentfile

I can confirm the bug here. It is not about bareness, but having no
index makes it easy to trigger, since it is easy to walk past the end of
a zero-length index. But it is not restricted to that case:

> It seemed to happen more likely with high letters (x, y, z) as the first
> character of the filename. This always worked for me:
>     $ git show :z
> But I found this to be too strange to be added to the commit message.

That's because cache_name_pos returns the position where the entry
_would_ be if it existed (well, the negation minus one, but the intent
is you can reconstruct that position if you did want to insert it). The
diagnose_invalid function then looks at that entry to see if it is a
missing filename or a missing stage. But of course, if it would be
inserted past the end of what exists in the index, there is nothing to
look at. So your ":z" is simply about being at the end of the index,
which is sorted alphabetically.

Which means your fix (to make sure we are not at the end) is correct.

-Peff