BUG: rev-parse segfault with invalid input

Hi,

Certain invalid input causes git rev-parse to crash rather than return a 'fatal: ambiguous argument ...' error.

This was reported against the Fedora git package:

    https://bugzilla.redhat.com/1581678

Simple reproduction recipe and analysis, from the bug:

    $ git init
    Initialized empty Git repository in /tmp/t/.git/
    $ git rev-parse ffffffffffffffffffffffffffffffffffffffff^@
    Segmentation fault (core dumped)

    gdb) break lookup_commit_reference
    Breakpoint 1 at 0x555555609f00: lookup_commit_reference. (3 locations)
    (gdb) r
    Starting program: /usr/bin/git rev-parse ffffffffffffffffffffffffffffffffffffffff\^@
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib64/libthread_db.so.1".

    Breakpoint 1, lookup_commit_reference (oid=oid@entry=0x7fffffffd550) at commit.c:34
    34              return lookup_commit_reference_gently(oid, 0);
    (gdb) finish
    Run till exit from #0  lookup_commit_reference (oid=oid@entry=0x7fffffffd550) at commit.c:34
    try_parent_shorthands (arg=0x7fffffffdd44 'f' <repeats 40 times>) at builtin/rev-parse.c:314
    314                     include_parents = 1;
    Value returned is $1 = (struct commit *) 0x0
    (gdb) c

    (gdb) c
    Continuing.

    Program received signal SIGSEGV, Segmentation fault.
    try_parent_shorthands (arg=0x7fffffffdd44 'f' <repeats 40 times>) at builtin/rev-parse.c:345
    345             for (parents = commit->parents, parent_number = 1;
    (gdb) l 336,+15
    336             commit = lookup_commit_reference(&oid);
    337             if (exclude_parent &&
    338                 exclude_parent > commit_list_count(commit->parents)) {
    339                     *dotdot = '^';
    340                     return 0;
    341             }
    342
    343             if (include_rev)
    344                     show_rev(NORMAL, &oid, arg);
    345             for (parents = commit->parents, parent_number = 1;
    346                  parents;
    347                  parents = parents->next, parent_number++) {
    348                     char *name = NULL;
    349
    350                     if (exclude_parent && parent_number != exclude_parent)
    351                             continue;

    Looks like a null pointer check is missing.

This occurs on master and as far back as 1.8.3.1 (what's in RHEL-6, I didn't try to test anything older). Only a string with 40 valid hex characters and ^@, @-, of ^! seems to trigger it.

Thanks for the detailed report. This apparently goes back to git-1.6.0 with commit 2122f8b963d4 ("rev-parse: Add support for the ^! and ^@ syntax", 2008-07-26). We aren't checking that the commit from lookup_commit_reference() is non-NULL before proceeding. Looks like it's simple to fix. I'll send a patch shortly...

In commit 2122f8b963d4 ("rev-parse: Add support for the ^! and ^@ syntax", 2008-07-26), try_parent_shorthands() was introduced to parse the special ^! and ^@ syntax. However, it did not check the commit returned from lookup_commit_reference() before proceeding to use it. If it is NULL, bail early and notify the caller that this cannot be a valid revision range.

Commits 2122f8b963d4 ("rev-parse: Add support for the ^! and ^@ syntax", 2008-07-26) and 3dd4e7320d ("Teach rev-parse the ... syntax.", 2006-07-04) taught rev-parse new syntax, and used lookup_commit_reference() as part of their logic. Neither usage checked the returned commit to see if it was non-NULL before using it. Check for NULL and ensure an appropriate error is reported to the user.

Reported by Florian Weimer and Todd Zullinger.

Helped-by Jeff King <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

---

I would have used a Reported-by tag for Florian and Todd, but looking at the bugzilla.redhat.com bug report doesn't show me Florian's email address. I grepped through git logs and found two associated with that name, but didn't know if they were still accurate, or were a different Florian. So I just went with the sentence instead.

 builtin/rev-parse.c          | 8 ++++++--
 t/t6101-rev-parse-parents.sh | 8 ++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

builtin/rev-parse.c +6 -2

index a1e680b5e9..a0a0ace38d 100644
@@ -282,6 +282,10 @@ static int try_difference(const char *arg)
 			struct commit *a, *b;
 			a = lookup_commit_reference(&start_oid);
 			b = lookup_commit_reference(&end_oid);
+			if (!a || !b) {
+				*dotdot = '.';
+				return 0;
+			}
 			exclude = get_merge_bases(a, b);
 			while (exclude) {
 				struct commit *commit = pop_commit(&exclude);

@@ -328,12 +332,12 @@ static int try_parent_shorthands(const char *arg)
 		return 0;
 
 	*dotdot = 0;
-	if (get_oid_committish(arg, &oid)) {
+	if (get_oid_committish(arg, &oid) ||
+	    !(commit = lookup_commit_reference(&oid))) {
 		*dotdot = '^';
 		return 0;
 	}
 
-	commit = lookup_commit_reference(&oid);
 	if (exclude_parent &&
 	    exclude_parent > commit_list_count(commit->parents)) {
 		*dotdot = '^';

t/t6101-rev-parse-parents.sh +8 -0

index 8c617981a3..7683e4a114 100755
@@ -214,4 +214,12 @@ test_expect_success 'rev-list merge^-1x (garbage after ^-1)' '
 	test_must_fail git rev-list merge^-1x
 '
 
+test_expect_success 'rev-parse $garbage^@ does not segfault' '
+	test_must_fail git rev-parse $EMPTY_TREE^@
+'
+
+test_expect_success 'rev-parse $garbage...$garbage does not segfault' '
+	test_must_fail git rev-parse $EMPTY_TREE...$EMPTY_BLOB
+'
+
 test_done

I added Florian to Cc, in case he wants to provide a preferred address.

Commits 2122f8b963d4 ("rev-parse: Add support for the ^! and ^@ syntax", 2008-07-26) and 3dd4e7320d ("Teach rev-parse the ... syntax.", 2006-07-04) taught rev-parse new syntax, and used lookup_commit_reference() as part of their logic. Neither usage checked the returned commit to see if it was non-NULL before using it. Check for NULL and ensure an appropriate error is reported to the user.

Reported by Florian Weimer and Todd Zullinger.

Helped-by Jeff King <[email protected]>

Signed-off-by Elijah Newren <[email protected]>

I would have used a Reported-by tag for Florian and Todd, but looking at the bugzilla.redhat.com bug report doesn't show me Florian's email address. I grepped through git logs and found two associated with that name, but didn't know if they were still accurate, or were a different Florian. So I just went with the sentence instead.

 builtin/rev-parse.c          | 8 ++++++--
 t/t6101-rev-parse-parents.sh | 8 ++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

builtin/rev-parse.c +0 -0

index a1e680b5e9..a0a0ace38d 100644

In commit 2122f8b963d4 ("rev-parse: Add support for the ^! and ^@ syntax", 2008-07-26), try_parent_shorthands() was introduced to parse the special ^! and ^@ syntax. However, it did not check the commit returned from lookup_commit_reference() before proceeding to use it. If it is NULL, bail early and notify the caller that this cannot be a valid revision range.

Thanks. This fixes the segfault. While I was testing this, I wondered if the following cases should differ:

# f*40

$ ./git-rev-parse ffffffffffffffffffffffffffffffffffffffff^@ ; echo $?
0

# f*39

$ ./git-rev-parse fffffffffffffffffffffffffffffffffffffff^@ ; echo $?
fffffffffffffffffffffffffffffffffffffff^@
fatal: ambiguous argument 'fffffffffffffffffffffffffffffffffffffff^@': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
128

Looking a little further, this is deeper than the rev-parse handling. The difference in how these invalid refs are handled appears in 'git show' as well. With 'git show' a (different) fatal error is returned in both cases.

# f*40

$ git show ffffffffffffffffffffffffffffffffffffffff
fatal: bad object ffffffffffffffffffffffffffffffffffffffff

# 39*f

$ git show fffffffffffffffffffffffffffffffffffffff
fatal: ambiguous argument 'fffffffffffffffffffffffffffffffffffffff': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

Should rev-parse return an error as well, rather than silenty succeeding?