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.

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 55c0b90441..4e9ba9641a 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -334,6 +334,8 @@ static int try_parent_shorthands(const char *arg)
>  	}
>  
>  	commit = lookup_commit_reference(&oid);
> +	if (!commit)
> +		return 1;
>  	if (exclude_parent &&
>  	    exclude_parent > commit_list_count(commit->parents)) {
>  		*dotdot = '^';

> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
> index 7b1b2dbdf2..f91cc417bd 100755
> --- a/t/t6101-rev-parse-parents.sh
> +++ b/t/t6101-rev-parse-parents.sh
> @@ -214,7 +214,7 @@ test_expect_success 'rev-list merge^-1x (garbage after ^-1)' '
>  	test_must_fail git rev-list merge^-1x
>  '
>  
> -test_expect_failure 'rev-parse $garbage^@ should not segfault' '
> +test_expect_success 'rev-parse $garbage^@ should not segfault' '
>  	git rev-parse ffffffffffffffffffffffffffffffffffffffff^@
>  '

> 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 <peff@peff.net>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---

> 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(-)
> 
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index a1e680b5e9..a0a0ace38d 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -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 = '^';
> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
> index 8c617981a3..7683e4a114 100755
> --- a/t/t6101-rev-parse-parents.sh
> +++ b/t/t6101-rev-parse-parents.sh
> @@ -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

> 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 <peff@peff.net>
> Signed-off-by: Elijah Newren <newren@gmail.com>

> 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(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index a1e680b5e9..a0a0ace38d 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -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;
> +			}

> @@ -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);

> 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.

> #          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?

> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh
> index 8c617981a3..7b1b2dbdf2 100755
> --- a/t/t6101-rev-parse-parents.sh
> +++ b/t/t6101-rev-parse-parents.sh
> @@ -214,4 +214,8 @@ test_expect_success 'rev-list merge^-1x (garbage after ^-1)' '
>  	test_must_fail git rev-list merge^-1x
>  '
>  
> +test_expect_failure 'rev-parse $garbage^@ should not segfault' '
> +	git rev-parse ffffffffffffffffffffffffffffffffffffffff^@
> +'