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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[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@xxxxxxxx>
> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> ---

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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux