[01/23] parse_commit_buffer(): treat lookup_commit() failure as parse error
diff mbox series

Message ID 20191018044212.GA17879@sigill.intra.peff.net
State New
Headers show
Series
  • parsing and fsck cleanups
Related show

Commit Message

Jeff King Oct. 18, 2019, 4:42 a.m. UTC
While parsing the parents of a commit, if we are able to parse an actual
oid but lookup_commit() fails on it (because we previously saw it in
this process as a different object type), we silently omit the parent
and do not report any error to the caller.

The caller has no way of knowing this happened, because even an empty
parent list is a valid parse result. As a result, it's possible to fool
our "rev-list" connectivity check into accepting a corrupted set of
objects.

There's a test for this case already in t6102, but unfortunately it has
a slight error. It creates a broken commit with a parent line pointing
to a blob, and then checks that rev-list notices the problem in two
cases:

  1. the "lone" case: we traverse the broken commit by itself (here we
     try to actually load the blob from disk and find out that it's not
     a commit)

  2. the "seen" case: we parse the blob earlier in the process, and then
     when calling lookup_commit() we realize immediately that it's not a
     commit

The "seen" variant for this test mistakenly parsed another commit
instead of the blob, meaning that we were actually just testing the
"lone" case again. Changing that reveals the breakage (and shows that
this fixes it).

Signed-off-by: Jeff King <peff@peff.net>
---
 commit.c                               | 11 ++++++++---
 t/t6102-rev-list-unexpected-objects.sh |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Oct. 24, 2019, 3:37 a.m. UTC | #1
Jeff King <peff@peff.net> writes:

> ...
> The "seen" variant for this test mistakenly parsed another commit
> instead of the blob, meaning that we were actually just testing the
> "lone" case again. Changing that reveals the breakage (and shows that
> this fixes it).
> ...
> @@ -52,7 +52,7 @@ test_expect_success 'traverse unexpected non-commit parent (lone)' '
>  '
>  
>  test_expect_success 'traverse unexpected non-commit parent (seen)' '
> -	test_must_fail git rev-list --objects $commit $broken_commit \
> +	test_must_fail git rev-list --objects $blob $broken_commit \
>  		>output 2>&1 &&
>  	test_i18ngrep "not a commit" output
>  '

Yikes.  Thanks for spotting.
Jeff King Oct. 24, 2019, 6:01 p.m. UTC | #2
On Thu, Oct 24, 2019 at 12:37:32PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ...
> > The "seen" variant for this test mistakenly parsed another commit
> > instead of the blob, meaning that we were actually just testing the
> > "lone" case again. Changing that reveals the breakage (and shows that
> > this fixes it).
> > ...
> > @@ -52,7 +52,7 @@ test_expect_success 'traverse unexpected non-commit parent (lone)' '
> >  '
> >  
> >  test_expect_success 'traverse unexpected non-commit parent (seen)' '
> > -	test_must_fail git rev-list --objects $commit $broken_commit \
> > +	test_must_fail git rev-list --objects $blob $broken_commit \
> >  		>output 2>&1 &&
> >  	test_i18ngrep "not a commit" output
> >  '
> 
> Yikes.  Thanks for spotting.

By the way, I shuffled this one to the front so that it could be taken
separately for "maint" if you want (but it has been this way for at
least a decade, so I don't think it's urgent for v2.24).

-Peff

Patch
diff mbox series

diff --git a/commit.c b/commit.c
index 40890ae7ce..6467c9e175 100644
--- a/commit.c
+++ b/commit.c
@@ -432,8 +432,11 @@  int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
 		if (graft && (graft->nr_parent < 0 || grafts_replace_parents))
 			continue;
 		new_parent = lookup_commit(r, &parent);
-		if (new_parent)
-			pptr = &commit_list_insert(new_parent, pptr)->next;
+		if (!new_parent)
+			return error("bad parent %s in commit %s",
+				     oid_to_hex(&parent),
+				     oid_to_hex(&item->object.oid));
+		pptr = &commit_list_insert(new_parent, pptr)->next;
 	}
 	if (graft) {
 		int i;
@@ -442,7 +445,9 @@  int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
 			new_parent = lookup_commit(r,
 						   &graft->parent[i]);
 			if (!new_parent)
-				continue;
+				return error("bad graft parent %s in commit %s",
+					     oid_to_hex(&graft->parent[i]),
+					     oid_to_hex(&item->object.oid));
 			pptr = &commit_list_insert(new_parent, pptr)->next;
 		}
 	}
diff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh
index 28611c978e..52cde097dd 100755
--- a/t/t6102-rev-list-unexpected-objects.sh
+++ b/t/t6102-rev-list-unexpected-objects.sh
@@ -52,7 +52,7 @@  test_expect_success 'traverse unexpected non-commit parent (lone)' '
 '
 
 test_expect_success 'traverse unexpected non-commit parent (seen)' '
-	test_must_fail git rev-list --objects $commit $broken_commit \
+	test_must_fail git rev-list --objects $blob $broken_commit \
 		>output 2>&1 &&
 	test_i18ngrep "not a commit" output
 '