threads / patch / 22855

patchsha1_name: fix segfault caused by invalid index access

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

## tl;dr

5 messages between Feb 28, 2010 and Feb 28, 2010. Diffs are folded; open one to read it.

replies: 4people: 5as markdown or json

Markus Heidelberg· Feb 28, 2010, 15:49 UTC · lore
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(-)
Show changes to sha1_name.c +18 −14
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
Matthieu Moy· Feb 28, 2010, 16:20 UTC · re: Markus Heidelberg · lore

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

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/
Matthieu Moy· Feb 28, 2010, 16:38 UTC · re: Matthieu Moy · lore

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(-)
Show changes to sha1_name.c +6 −10
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
Junio C Hamano· Feb 28, 2010, 18:13 UTC · re: Matthieu Moy · lore

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

Matthieu Moy <Matthieu.Moy@imag.fr> writes:
Show 7 quoted lines
> -	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.

Jeff King· Feb 28, 2010, 16:25 UTC · re: Markus Heidelberg · lore

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

On Sun, Feb 28, 2010 at 04:49:15PM +0100, Markus Heidelberg wrote:
Show 6 quoted lines
> 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

← back to recent threads