BUG: rev-parse segfault with invalid input

BUG: rev-parse segfault with invalid input

[Todd Zullinger]

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.

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I don't mind arguing with myself. It's when I lose that it bothers me.
    -- Richard Powers

Re: BUG: rev-parse segfault with invalid input

[Elijah Newren]

On Wed, May 23, 2018 at 12:52 PM, Todd Zullinger <tmz@pobox.com> wrote:
> 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...

Re: BUG: rev-parse segfault with invalid input

[Todd Zullinger]

Hi,

Elijah Newren wrote:
> 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...

Thanks Elijah!  I thought it was likely to be a simple fix.
But I also don't know the area well and that kept me from
being too ambitious about suggesting a fix or the difficulty
of one. :)

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I believe in the noble, aristocratic art of doing absolutely nothing.
And someday, I hope to be in a position where I can do even less.

[PATCH 1/2] t6101: add a test for rev-parse $garbage^@

[Elijah Newren]

Reported by Florian Weimer and Todd Zullinger.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t6101-rev-parse-parents.sh | 4 ++++
 1 file changed, 4 insertions(+)

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^@
+'
+
 test_done
-- 
2.17.0.1025.g36b5c64692

[PATCH 2/2] rev-parse: verify that commit looked up is not NULL

[Elijah Newren]

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.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rev-parse.c          | 2 ++
 t/t6101-rev-parse-parents.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

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^@
 '
 
-- 
2.17.0.1025.g36b5c64692

Re: [PATCH 2/2] rev-parse: verify that commit looked up is not NULL

[Jeff King]

On Wed, May 23, 2018 at 01:46:13PM -0700, Elijah Newren wrote:

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

Yep, this is definitely the right track. But...

> 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 = '^';

...I don't think this is quite right. I see two issues:

  1. We need to restore "*dotdot" like the other exit code-paths do.

  2. I think a return of 1 means "yes, I handled this". We want to
     return 0 so that the bogus name eventually triggers an error.

I also wondered if we need to print an error message, but since we are
using the non-gentle form of lookup_commit_reference(), it will complain
for us (and then the caller will issue some errors as well).

It might make sense to just lump this into the get_oid check above.
E.g., something like:

  if (get_oid_committish(arg, &oid) ||
      !(commit = lookup_commit_reference(&oid))) {
          *dotdot = '^';
	  return 0;
  }

though I am fine with it either way.

> 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^@
>  '

Once we flip the return value as above, I think this needs to be
test_must_fail, which matches how I'd expect it to behave.

This code (sadly) duplicates the functionality in revision.c. I checked
there to see if it has the same problem, but it's fine.

Unfortunately I think rev-parse has one other instance, though:

  bogus=ffffffffffffffffffffffffffffffffffffffff

  # this is ok; we just normalize to "$bogus ^$bogus" without looking at
  # the object, which is OK
  git rev-parse $bogus..$bogus

  # this segfaults, because we try to feed NULL to get_merge_bases()
  git rev-parse $bogus...$bogus

We should probably fix that at the same time.

-Peff

Re: [PATCH 1/2] t6101: add a test for rev-parse $garbage^@

[Jeff King]

On Wed, May 23, 2018 at 01:46:12PM -0700, Elijah Newren wrote:

> 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^@
> +'

Two small nits. :)

It may just be me, but for a trivial test+fix like this, I'd rather see
them in the same commit (both for reviewing, and when I'm digging in the
history later).

The second nit is that we may want to use something a little more
symbolic and easier to read here. Thirty-nine f's behaves quite
differently than forty. And eventually we'd like to move away from
having hard-coded commit ids anyway (this is obviously a fake one, but
the length may end up changing).

Perhaps "git rev-parse $EMPTY_TREE^@", which triggers the same bug?

-Peff

Re: [PATCH 2/2] rev-parse: verify that commit looked up is not NULL

[Todd Zullinger]

Elijah Newren wrote:
> 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?

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I refuse to spend my life worrying about what I eat. There is no
pleasure worth foregoing just for an extra three years in the
geriatric ward.
    -- John Mortimer

Re: [PATCH 2/2] rev-parse: verify that commit looked up is not NULL

[Todd Zullinger]

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

Nevermind me.  Jeff beat me to a reply and included much
more useful details about why this occurs and suggestions
for fixing it. :)

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

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
How can I tell that the past isn't a fiction designed to account for
the discrepancy between my immediate physical sensation and my state
of mind?
    -- Douglas Adams

[PATCH v2] rev-parse: check lookup'ed commit references for NULL

[Elijah Newren]

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

Re: [PATCH v2] rev-parse: check lookup'ed commit references for NULL

[Todd Zullinger]

[Added Florian to Cc]

Elijah Newren wrote:
> 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>
> ---

The output is now much more consistent with other invalid
input.  The only (minor) difference I noticed was when using
the fff...fff form.  With exactly 40 chars, rev-parse prints
both refs separately and then the full input string before
the "fatal:" error.  I doubt it's terribly important.

# exactly 40 chars
$ ./git-rev-parse ffffffffffffffffffffffffffffffffffffffff...ffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffff
ffffffffffffffffffffffffffffffffffffffff...ffffffffffffffffffffffffffffffffffffffff
fatal: ambiguous argument 'ffffffffffffffffffffffffffffffffffffffff...ffffffffffffffffffffffffffffffffffffffff': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

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

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

I added Florian to Cc, in case he wants to provide a
preferred address.  (The Red Hat Bugzilla only shows
email addresses if you're logged in.)

Thanks Elijah and Peff.

>  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

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
If the triangles were to make a God they would give him three sides.
    -- Montesquieu

Re: [PATCH v2] rev-parse: check lookup'ed commit references for NULL

[Florian Weimer]

On 05/24/2018 04:04 PM, Todd Zullinger wrote:
> I added Florian to Cc, in case he wants to provide a
> preferred address.

Sorry, using this address is fine.

Thanks,
Florian

Re: [PATCH v2] rev-parse: check lookup'ed commit references for NULL

[Jeff King]

On Wed, May 23, 2018 at 11:27:33PM -0700, Elijah Newren wrote:

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

This version looks good to me. Thanks for taking care of this!

-Peff

Re: [PATCH v2] rev-parse: check lookup'ed commit references for NULL

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

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

Or write names after reported-by without any address?  There is no
law that says that a trailer's contents must be proper e-mail
addresses.  People are already known to put garbage on Cc:, for
example.

>  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;
> +			}

We thought A..B or X...Y were a commit range, but it turns out that
it is not the case, since at least one end is not a committish.  We
simply restore the original and tell "No, this is not a range, try
to parse it as something else" to the caller by returning 0.

Makes sense.

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

OK, the logic flows the same way for things like foo^@ here, which
makes sense.

Looks good.  Thanks.