[RFC PATCH] unpack-trees: watch for out-of-range index position

[Emily Shaffer <emilyshaffer@google.com>]

It's possible in a case where the index file contains a tree extension
but no blobs within that tree exist for index_pos_by_traverse_info() to
segfault. If the name_entry passed into index_pos_by_traverse_info() has
no blobs inside, AND is alphabetically later than all blobs currently in
the index file, index_pos_by_traverse_info() will segfault. For example,
an index file which looks something like this:

  aaa#0
  bbb/aaa#0
  [Extensions]
  TREE: zzz

In this example, 'index_name_pos(..., "zzz/", ...)' will return '-4',
indicating that "zzz/" could be inserted at position 3. However, when
the checks which ensure that the insertion position of "zzz/" look for a
blob at that position beginning with "zzz/", the index cache is accessed
out of range, causing a segfault.

This kind of index state is not typically generated during user
operations, and is in fact an edge case of the state being checked for
in the conditional where it was added. However, since the entry for the
BUG() line is ambiguous, tell some additional context to help Git
developers debug the failure later. When we know the name of the dir we
were trying to look up, it becomes possible to examine the index file
in a hex util to determine what went wrong; the position gives a hint
about where to start looking.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
This issue came in via a bugreport from a user who had done some nasty
things like deleting various files in .git/ (and then couldn't remember
how they had done it). The concern was primarily that a segfault is ugly
and scary, and possibly dangerous; I didn't see much problem with
checking for index-out-of-range if the result is a fatal error
regardless.

The index file they shared for reproduction was very similar to the one
that I proposed in the commit message. However, though I had a repo
where I could reproduce, since the user wasn't sure how they had gotten
there I struggled reasoning about how to produce these exact conditions.
It seems like during normal operation the index shouldn't learn about a
tree extension where it doesn't know any blobs (in fact, I've become
irritated before about being unable to stage/commit only directory
structure :) ).

I also didn't find any test cases looking for the BUG() as it exists
now; I guess that's because BUG()s are supposed to be unreachable during
normal operation (or else they'd be a die()). So, it's marked RFC only
because I couldn't think of a way to reliably reproduce or test this
change.

 - Emily

 unpack-trees.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index a75b068d33..d1ac1e179c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -694,9 +694,11 @@ static int index_pos_by_traverse_info(struct name_entry *names,
 	if (pos >= 0)
 		BUG("This is a directory and should not exist in index");
 	pos = -pos - 1;
-	if (!starts_with(o->src_index->cache[pos]->name, name.buf) ||
+	if (pos >= o->src_index->cache_nr ||
+	    !starts_with(o->src_index->cache[pos]->name, name.buf) ||
 	    (pos > 0 && starts_with(o->src_index->cache[pos-1]->name, name.buf)))
-		BUG("pos must point at the first entry in this directory");
+		BUG("pos %d doesn't point to the first entry of %s in index",
+		    pos, name.buf);
 	strbuf_release(&name);
 	return pos;
 }
-- 
2.25.0.rc1.283.g88dfdc4193-goog