pull: do not segfault when HEAD refers to missing object file

git pull --rebase on a corrupted HEAD used to segfault; it has been corrected to error out with a message. A test has also been added to verify this fix.

---

Notes:
When add_head_to_pending fails to add a pending object, git pull --rebase segfaults. This can happen if HEAD is referring to a corrupt or missing object.

I discovered this segfault on my machine after pulling a repo from GitHub, but I have been unable to reproduce the sequence of events that lead to the corrupted HEAD (I think it may have been caused by a lost network connection in my case).

The following commands use add_head_to_pending:

format-patch setup_revisions before add_head_to_pending
diff checks rev.pending.nr
shortlog checks rev.pending.nr
log uses resolve_ref_unsafe

All of the above return an error code of 128 and print "fatal: bad object HEAD" instead of segfaulting, which I think is correct behavior. The check and error message have been added to has_uncommitted_changes, where they were missing, as well as to diff-lib.c (without the error message).

 diff-lib.c      |  2 +-
 t/t5520-pull.sh | 12 ++++++++++++
 wt-status.c     |  5 +++++
 3 files changed, 18 insertions(+), 1 deletion(-)

index 52447466b..9d26b18c3 100644
@@ -512,7 +512,7 @@ int run_diff_index(struct rev_info *revs, int cached)
 	struct object_array_entry *ent;
 
 	ent = revs->pending.objects;
-	if (diff_cache(revs, ent->item->oid.hash, ent->name, cached))
+	if (!ent || diff_cache(revs, ent->item->oid.hash, ent->name, cached))
 		exit(128);
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");

index 17f4d0fe4..1edb6a97a 100755
@@ -664,4 +664,16 @@ test_expect_success 'git pull --rebase against local branch' '
 	test file = "$(cat file2)"
 '
 
+test_expect_success 'git pull --rebase with corrupt HEAD does not segfault' '
+	mkdir corrupted &&
+	(cd corrupted &&
+	git init &&
+	echo one >file && git add file &&
+	git commit -m one &&
+	REV=$(git rev-parse HEAD) &&
+	rm -f .git/objects/${REV:0:2}/${REV:2} &&
+	test_expect_code 128 git pull --rebase > /dev/null
+	)
+'
+
 test_done

index d47012048..3d60eaff5 100644
@@ -2252,6 +2252,11 @@ int has_uncommitted_changes(int ignore_submodules)
 		DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	add_head_to_pending(&rev_info);
+
+	/* The add_head_to_pending call might not have added anything. */
+	if (!rev_info.pending.nr)
+		die("bad object %s", "HEAD");
+
 	diff_setup_done(&rev_info.diffopt);
 	result = run_diff_index(&rev_info, 1);
 	return diff_result_code(&rev_info.diffopt, result);

2.12.0

Yeah, it is. You can do it easily with 'sed', of course, but if you want to avoid the extra process and do it in pure shell, it's more like:

  last38=${REV#??}
  first2=${REV%$last38}
  rm -f .git/objects/$first2/$last38

-Peff

Am 06.03.2017 um 04:51 schrieb Jeff King:

We usally indent this like so:

	(
		cd corrupted &&
		echo one >file &&
		git add file &&
...
	) &&

Is it "HEAD points to non-existent object" or ".git/HEAD contains junk"? In both cases there are simpler solutions than to remove an object. For example, echo "$_x40" >.git/HEAD or echo "this is junk" >.git/HEAD?

-- Hannes

Good point. Unfortunately, it's a little tricky. You can't use "this is junk" because then git does not recognize it as a git repo at all. You can't use $_x40 because the null sha1 is used internally to signal an unborn branch, so we mix it up with that case.

Something like "111..." for 40 characters would work, though at that point I think just simulating an actual corruption might be less convoluted.

-Peff

Your commit message mostly tells us the "what" that we can see from the diff. But why are we segfaulting? What assumption is being violated, and why is the fix the right thing?

You've stuck some of the details in your notes, and they should probably make it into the commit message.

The other obvious time when HEAD is not valid is when you are on an unborn branch. So we should also consider how such a case interacts with the callsites you are touching.

I think it is primarily this hunk:

	die(_("Updating an unborn branch with changes added to the index."));

Yikes. That should never lead to a corrupted HEAD (unless you are on a networked filesystem). I can't think of another read add_head_to_pending would fail, though, aside from a missing or corrupted object.

Arguably add_head_to_pending() should die loudly if it can't parse the object pointed to by HEAD, rather than quietly returning without adding anything.