[PATCH] Don't dereference NULL upon lookup_tree failure.

[PATCH] Don't dereference NULL upon lookup_tree failure.

[Jim Meyering]

While Matthew Farrellee was working on converting the Condor repository
from cvs to git, he managed to make git segfault (with help from parsecvs)
while producing a 100MB .git repository.  He found the single offending
,v file that led parsecvs to generate a bogus repository, and deduced
that adding a single well-placed branch tag[*] was enough to avoid
the problem.  I fixed the bug in git along with a few in parsecvs that
were exposed while paring the ,v file down from a 150KB monstrosity to
something manageable.

[*] Adding this tag avoids the problem: FOO:1.30.2.5.0.8

First, here's how to reproduce the git segfault, using the file, "min,v"
included below.  Run this in an empty directory:

  rm -rf k .git .git-cvs
  parsecvs min,v >& log
  git clone -q . k

I noticed that while I get a segfault both on x86 and x86_64, I see
clear evidence of it only on x86:

  0 blocks
  error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree
  Segmentation fault
  fatal: The remote end hung up unexpectedly
  [Exit 1]

When running on an x86_64 system (either debian unstable or rawhide)
I see only this:

  0 blocks
  error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree
  fatal: The remote end hung up unexpectedly

Of course, valgrind shows you the segfault in both cases.

The above was using git version 1.5.4.rc1.3.gec692
and the latest parsecvs from here:
git://people.freedesktop.org/~keithp/parsecvs

    commit 2b0113ffb0055620193397c025d6f6bca3b110cd
    Author: Finn Arne Gangstad <finnag@pvv.org>
    Date:   Sun Nov 18 15:26:35 2007 -0800

This patch avoids the NULL dereference by treating a failed lookup_tree the
same way an invalid "type" is handled in the "else" block just below.
The only difference is that for a failed lookup_tree, the failing
function has already produced a diagnostic.

-----------------------------------------------------
From 4cd649160d8174b23727b3d7276f1bd7246d0aff Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Fri, 21 Dec 2007 11:56:32 +0100
Subject: [PATCH] Don't dereference NULL upon lookup_tree failure.


Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 object.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/object.c b/object.c
index 16793d9..eb59550 100644
--- a/object.c
+++ b/object.c
@@ -142,10 +142,14 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 		obj = &blob->object;
 	} else if (type == OBJ_TREE) {
 		struct tree *tree = lookup_tree(sha1);
-		obj = &tree->object;
-		if (!tree->object.parsed) {
-			parse_tree_buffer(tree, buffer, size);
-			eaten = 1;
+		if (!tree)
+		    obj = NULL;
+		else {
+		    obj = &tree->object;
+		    if (!tree->object.parsed) {
+			    parse_tree_buffer(tree, buffer, size);
+			    eaten = 1;
+		    }
 		}
 	} else if (type == OBJ_COMMIT) {
 		struct commit *commit = lookup_commit(sha1);
--
1.5.4.rc0.76.g55ee


Here's the contents of min,v
------------------------------------------------
head	1.31;
access;
symbols
	a:1.30.2.18.4.14.4.29.0.6
	b:1.30.2.18.4.14.4.29.0.4
	c:1.30.2.18.4.14.4.29.0.2
	d:1.30.2.18.4.14.4.27.0.6
	e:1.30.2.18.4.14.4.27.0.4
	f:1.30.2.18.4.14.4.27.0.2
	g:1.30.2.18.4.14.4.25.0.6
	h:1.30.2.18.4.14.4.25.0.4
	i:1.30.2.18.4.14.4.25.0.2
	j:1.30.2.18.4.14.4.24.0.24
	k:1.30.2.18.4.14.4.24.0.22
	l:1.30.2.18.4.14.4.24.0.20
	m:1.30.2.18.4.14.4.24.0.18
	n:1.30.2.18.4.14.4.24.0.16
	o:1.30.2.18.4.14.4.24.0.14
	p:1.30.2.18.4.14.4.24.0.12
	q:1.30.2.18.4.14.4.24.0.10
	r:1.30.2.18.4.14.4.24.0.8
	s:1.30.2.18.4.14.4.24.0.6
	t:1.30.2.18.4.14.4.24.0.4
	u:1.30.2.18.4.14.4.24.0.2
	a0:1.30.2.18.4.14.4.23.0.10
	a1:1.30.2.18.4.14.4.23.0.8
	a2:1.30.2.18.4.14.4.23.0.6
	a3:1.30.2.18.4.14.4.23.0.4
	a4:1.30.2.18.4.14.4.23.0.2
	a5:1.30.2.18.4.14.4.22.0.2
	a6:1.30.2.18.4.14.4.21.0.2
	a7:1.30.2.18.4.14.4.18.4.1.0.2
	a8:1.30.2.18.4.14.4.18.0.6
	a9:1.30.2.18.4.14.4.18.0.4
	a10:1.30.2.18.4.14.4.17.0.12
	a11:1.30.2.18.4.14.4.18.0.2
	a12:1.30.2.18.4.14.4.17.0.10
	a13:1.30.2.18.4.14.4.17.0.8
	a14:1.30.2.18.4.14.4.17.0.6
	a15:1.30.2.18.4.14.4.17.0.4
	a16:1.30.2.18.4.14.4.17.0.2
	a17:1.30.2.18.4.14.4.16.0.2
	a18:1.30.2.18.4.14.4.15.0.2
	a19:1.30.2.18.4.14.4.14.0.10
	a20:1.30.2.18.4.14.4.14.0.8
	a21:1.30.2.18.4.14.4.14.0.6
	a22:1.30.2.18.4.14.4.14.0.4
	a23:1.30.2.18.4.14.4.14.0.2
	a24:1.30.2.18.4.14.4.13.0.4
	a25:1.30.2.18.4.14.4.13.0.2
	a26:1.30.2.18.4.14.4.7.0.6
	a27:1.30.2.18.4.14.4.12.0.8
	a28:1.30.2.18.4.14.4.12.0.6
	a29:1.30.2.18.4.14.4.12.0.4
	a30:1.30.2.18.4.14.4.12.0.2
	a31:1.30.2.18.4.14.4.11.0.4
	a32:1.30.2.18.4.14.4.11.0.2
	a33:1.30.2.18.4.14.4.9.0.16
	a34:1.30.2.18.4.14.4.9.0.14
	a35:1.30.2.18.4.14.4.9.0.12
	a36:1.30.2.18.4.14.4.9.0.10
	a37:1.30.2.18.4.14.4.9.0.8
	a38:1.30.2.18.4.14.4.9.0.6
	a39:1.30.2.18.4.14.4.9.0.4
	a40:1.30.2.18.4.14.4.9.0.2
	a41:1.30.2.18.4.14.4.8.0.16
	a42:1.30.2.18.4.14.4.8.0.14
	a43:1.30.2.18.4.14.4.8.0.12
	a44:1.30.2.18.4.14.4.8.0.10
	a45:1.30.2.18.4.14.4.8.0.8
	a46:1.30.2.18.4.14.4.8.0.6
	a47:1.30.2.18.4.14.4.8.0.4
	a48:1.30.2.18.4.14.4.8.0.2
	a49:1.30.2.18.4.14.4.7.0.4
	a50:1.30.2.18.4.14.4.7.0.2
	a51:1.30.2.18.4.14.4.6.0.2
	a52:1.30.2.18.4.14.4.3.4.1.0.2
	a53:1.30.2.18.4.14.4.5.0.2
	a54:1.30.2.18.4.14.4.5.0.14
	a55:1.30.2.18.4.14.4.5.0.12
	a56:1.30.2.18.4.14.4.5.0.10
	a57:1.30.2.18.4.14.4.5.0.8
	a58:1.30.2.18.4.14.4.5.0.6
	a59:1.30.2.18.4.14.4.5.0.4
	a60:1.30.2.18.4.14.4.3.0.4
	a61:1.30.2.18.4.14.4.3.0.2
	a62:1.30.2.18.4.14.4.2.0.16
	a63:1.30.2.18.4.14.4.2.0.14
	a64:1.30.2.18.4.14.4.2.0.12
	a65:1.30.2.18.4.14.4.2.0.10
	a66:1.30.2.18.4.14.4.2.0.8
	a67:1.30.2.18.4.14.4.2.0.6
	a68:1.30.2.18.4.14.4.2.0.4
	a69:1.30.2.18.4.14.4.2.0.2
	a70:1.30.2.18.4.14.4.1.0.4
	a71:1.30.2.18.4.14.4.1.0.2
	a72:1.30.2.18.4.14.0.4
	a73:1.30.2.18.4.14.0.2
	a74:1.30.2.18.4.13.0.2
	a75:1.30.2.18.4.11.0.4
	a76:1.30.2.18.4.11.0.2
	a77:1.30.2.18.4.10.0.2
	a78:1.30.2.18.4.6.0.2
	a79:1.30.2.18.4.3.0.4
	a80:1.30.2.18.4.5.0.4
	a81:1.30.2.18.4.5.0.2
	a82:1.30.2.18.4.3.0.2
	a83:1.30.2.18.2.8.0.2
	a84:1.30.2.18.4.1.0.8
	a85:1.30.2.18.4.1.0.6
	a86:1.30.2.18.4.1.0.4
	a87:1.30.2.18.4.1.0.2
	a88:1.30.2.18.0.4
	a89:1.30.2.18.0.2
	a90:1.30.2.14.0.2
	a91:1.31.0.4
	a92:1.31.0.2
	a93:1.30.2.5.0.14
	a94:1.30.2.5.0.12
	a95:1.30.2.5.0.10
	a96:1.30.2.5.0.6
	a97:1.30.2.5.0.4
	a98:1.30.2.5.0.2
	a99:1.30.2.4.0.2
	a100:1.30.2.3.0.8
	a101:1.30.2.3.0.6
	a102:1.30.2.3.0.2
	a103:1.30.2.3.0.4
	a104:1.30.2.1.0.2
	a105:1.30.0.2
	new-syscall-branch:1.14.0.2
	V6_0-branch:1.6.0.2;
locks; strict;
comment	@ * @;


1.31
date	2000.07.06.19.46.55;	author x;	state Exp;
branches
	1.31.4.1;
next	1.30;

1.30
date	2000.07.06.17.21.26;	author x;	state Exp;
branches
	1.30.2.1;
next	1.14;

1.14
date	99.03.09.23.33.53;	author x;	state Exp;
branches
	1.14.6.1;
next	;

1.14.6.1
date	99.03.17.05.07.10;	author x;	state Exp;
branches;
next	;

1.30.2.1
date	2000.12.13.20.13.52;	author x;	state Exp;
branches;
next	1.30.2.3;

1.30.2.3
date	2001.03.28.18.40.01;	author x;	state Exp;
branches;
next	1.30.2.4;

1.30.2.4
date	2001.08.21.21.14.44;	author x;	state Exp;
branches;
next	1.30.2.5;

1.30.2.5
date	2001.10.17.20.05.09;	author x;	state Exp;
branches
	1.30.2.5.8.1;
next	1.30.2.14;

1.30.2.14
date	2002.06.13.17.02.53;	author x;	state Exp;
branches;
next	1.30.2.18;

1.30.2.18
date	2002.07.18.22.25.03;	author x;	state Exp;
branches;
next	;

1.30.2.5.8.1
date	2002.01.29.00.03.25;	author x;	state Exp;
branches;
next	;

1.31.4.1
date	2002.04.16.22.46.40;	author x;	state Exp;
branches;
next	;


desc
@@


1.31
log
@6.5.0 version string, on the trunk.
@
text
@



















































































@


1.31.4.1
log
@
@
text
@d35 1
a35 1

@


1.30
log
@
@
text
@d35 1
a35 1

@


1.30.2.1
log
@
@
text
@d35 1
a35 1

@


1.30.2.3
log
@
@
text
@d35 1
a35 1

@


1.30.2.4
log
@
@
text
@d35 1
a35 1

@


1.30.2.5
log
@
@
text
@d35 1
a35 1

@


1.30.2.14
log
@
@
text
@d35 1
a35 1

@


1.30.2.18
log
@
@
text
@d35 1
a35 1

@


1.30.2.5.8.1
log
@
@
text
@d35 1
a35 1

@


1.14
log
@
@
text
@d25 43
a67 1
s
d72 1
a72 1
C
d77 6
d84 1
@


1.14.6.1
log
@
@
text
@d25 1
a25 1

@

Re: [PATCH] Don't dereference NULL upon lookup_tree failure.

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> When running on an x86_64 system (either debian unstable or rawhide)
> I see only this:
>
>   0 blocks
>   error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree
>   fatal: The remote end hung up unexpectedly
>...
> diff --git a/object.c b/object.c
> index 16793d9..eb59550 100644
> --- a/object.c
> +++ b/object.c
> @@ -142,10 +142,14 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
>  		obj = &blob->object;
>  	} else if (type == OBJ_TREE) {
>  		struct tree *tree = lookup_tree(sha1);
> -		obj = &tree->object;
> -		if (!tree->object.parsed) {
> -			parse_tree_buffer(tree, buffer, size);
> -			eaten = 1;
> +		if (!tree)
> +		    obj = NULL;
> +		else {
> +		    obj = &tree->object;
> +		    if (!tree->object.parsed) {
> +			    parse_tree_buffer(tree, buffer, size);
> +			    eaten = 1;
> +		    }
>  		}
>  	} else if (type == OBJ_COMMIT) {
>  		struct commit *commit = lookup_commit(sha1);

While this change may be a prudent safeguard, there is something
else going on.  Can you provide the callchain that led to the
parse_object_buffer() that gave SHA1 of a commit object with
type set to OBJ_TREE?  Which caller does that bogus combination?

Re: [PATCH] Don't dereference NULL upon lookup_tree failure.

[Jim Meyering]

Junio C Hamano <gitster@pobox.com> wrote:

> Jim Meyering <jim@meyering.net> writes:
>
>> When running on an x86_64 system (either debian unstable or rawhide)
>> I see only this:
>>
>>   0 blocks
>>   error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree
>>   fatal: The remote end hung up unexpectedly
>>...
>> diff --git a/object.c b/object.c
>> index 16793d9..eb59550 100644
>> --- a/object.c
>> +++ b/object.c
>> @@ -142,10 +142,14 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
>>  		obj = &blob->object;
>>  	} else if (type == OBJ_TREE) {
>>  		struct tree *tree = lookup_tree(sha1);
>> -		obj = &tree->object;
>> -		if (!tree->object.parsed) {
>> -			parse_tree_buffer(tree, buffer, size);
>> -			eaten = 1;
>> +		if (!tree)
>> +		    obj = NULL;
>> +		else {
>> +		    obj = &tree->object;
>> +		    if (!tree->object.parsed) {
>> +			    parse_tree_buffer(tree, buffer, size);
>> +			    eaten = 1;
>> +		    }
>>  		}
>>  	} else if (type == OBJ_COMMIT) {
>>  		struct commit *commit = lookup_commit(sha1);
>
> While this change may be a prudent safeguard, there is something
> else going on.  Can you provide the callchain that led to the
> parse_object_buffer() that gave SHA1 of a commit object with
> type set to OBJ_TREE?  Which caller does that bogus combination?

Sure.
Here's valgrind output from running this (from my reproducer):

    valgrind --trace-children=yes git clone . k
---------------
  error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree
  ==9483== Invalid read of size 1
  ==9483==    at 0x405C27: parse_object_buffer (object.c:146)
  ==9483==    by 0x405CE4: parse_object (object.c:187)
  ==9483==    by 0x403185: send_ref (upload-pack.c:561)
  ==9483==    by 0x408EEF: do_for_each_ref (refs.c:546)
  ==9483==    by 0x4036EC: main (upload-pack.c:587)
  ==9483==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
  ==9483==
  ==9483== Process terminating with default action of signal 11 (SIGSEGV)
  ==9483==  Access not within mapped region at address 0x0
  ==9483==    at 0x405C27: parse_object_buffer (object.c:146)
  ==9483==    by 0x405CE4: parse_object (object.c:187)
  ==9483==    by 0x403185: send_ref (upload-pack.c:561)
  ==9483==    by 0x408EEF: do_for_each_ref (refs.c:546)
  ==9483==    by 0x4036EC: main (upload-pack.c:587)
  ...
  fatal: The remote end hung up unexpectedly

Re: [PATCH] Don't dereference NULL upon lookup_tree failure.

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

>> While this change may be a prudent safeguard, there is something
>> else going on.  Can you provide the callchain that led to the
>> parse_object_buffer() that gave SHA1 of a commit object with
>> type set to OBJ_TREE?  Which caller does that bogus combination?
>
> Sure.
> Here's valgrind output from running this (from my reproducer):
>
>     valgrind --trace-children=yes git clone . k
> ---------------
>   error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree
>   ==9483== Invalid read of size 1
>   ==9483==    at 0x405C27: parse_object_buffer (object.c:146)
>   ==9483==    by 0x405CE4: parse_object (object.c:187)
>   ==9483==    by 0x403185: send_ref (upload-pack.c:561)
>   ==9483==    by 0x408EEF: do_for_each_ref (refs.c:546)
>   ==9483==    by 0x4036EC: main (upload-pack.c:587)

Sorry, I asked for a wrong thing.  parse_object() reads and
finds out the type, so type there is presumably the right type
of the object (which is OBJ_TREE).  Then parse_object_buffer()
checks if it has already seen the object of the same SHA-1 and
finds that somebody had earlier told that SHA-1 name is of a
commit object.  Either you found a SHA-1 collision (highly
unlikely) or the earlier caller had lied.  And I think what
really needs to be fixed is that lying caller.  That is not in
the above call chain.

Re: [PATCH] Don't dereference NULL upon lookup_tree failure.

[Jim Meyering]

Junio C Hamano <gitster@pobox.com> wrote:
> Jim Meyering <jim@meyering.net> writes:
>
>>> While this change may be a prudent safeguard, there is something
>>> else going on.  Can you provide the callchain that led to the
>>> parse_object_buffer() that gave SHA1 of a commit object with
>>> type set to OBJ_TREE?  Which caller does that bogus combination?
>>
>> Sure.
>> Here's valgrind output from running this (from my reproducer):
>>
>>     valgrind --trace-children=yes git clone . k
>> ---------------
>>   error: Object 0d57588da39d10795486bd5451bc2660832228e6 is a commit, not a tree
>>   ==9483== Invalid read of size 1
>>   ==9483==    at 0x405C27: parse_object_buffer (object.c:146)
>>   ==9483==    by 0x405CE4: parse_object (object.c:187)
>>   ==9483==    by 0x403185: send_ref (upload-pack.c:561)
>>   ==9483==    by 0x408EEF: do_for_each_ref (refs.c:546)
>>   ==9483==    by 0x4036EC: main (upload-pack.c:587)
>
> Sorry, I asked for a wrong thing.  parse_object() reads and
> finds out the type, so type there is presumably the right type
> of the object (which is OBJ_TREE).  Then parse_object_buffer()
> checks if it has already seen the object of the same SHA-1 and
> finds that somebody had earlier told that SHA-1 name is of a
> commit object.  Either you found a SHA-1 collision (highly
> unlikely) or the earlier caller had lied.  And I think what
> really needs to be fixed is that lying caller.  That is not in
> the above call chain.

I haven't debugged it enough to find/fix the bug in parsecvs.

I presume that parsecvs is the culprit, in constructing
an invalid repository -- especially, since inserting the
single missing tag into the ,v file is enough to make it so
parsecvs creates a repository that no longer triggers the git bug.

Re: [PATCH] Don't dereference NULL upon lookup_tree failure.

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> Sorry, I asked for a wrong thing.  parse_object() reads and
>> finds out the type, so type there is presumably the right type
>> of the object (which is OBJ_TREE).  Then parse_object_buffer()
>> checks if it has already seen the object of the same SHA-1 and
>> finds that somebody had earlier told that SHA-1 name is of a
>> commit object.  Either you found a SHA-1 collision (highly
>> unlikely) or the earlier caller had lied.  And I think what
>> really needs to be fixed is that lying caller.  That is not in
>> the above call chain.
>
> I presume that parsecvs is the culprit, in constructing
> an invalid repository...

Independent of who creates a "invalid repository", our tools
should be prepared to be fed bad data and not get upset.
Somebody in our code read a non-commit (or it did not read
anything) and told the object layer it is a commit.  That
codepath needs to be tightened up, no?

Re: [PATCH] Don't dereference NULL upon lookup_tree failure.

[Jim Meyering]

Junio C Hamano <gitster@pobox.com> wrote:

> Jim Meyering <jim@meyering.net> writes:
>
>> Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>> Sorry, I asked for a wrong thing.  parse_object() reads and
>>> finds out the type, so type there is presumably the right type
>>> of the object (which is OBJ_TREE).  Then parse_object_buffer()
>>> checks if it has already seen the object of the same SHA-1 and
>>> finds that somebody had earlier told that SHA-1 name is of a
>>> commit object.  Either you found a SHA-1 collision (highly
>>> unlikely) or the earlier caller had lied.  And I think what
>>> really needs to be fixed is that lying caller.  That is not in
>>> the above call chain.
>>
>> I presume that parsecvs is the culprit, in constructing
>> an invalid repository...
>
> Independent of who creates a "invalid repository", our tools
> should be prepared to be fed bad data and not get upset.
> Somebody in our code read a non-commit (or it did not read
> anything) and told the object layer it is a commit.  That
> codepath needs to be tightened up, no?

Yes, of course.
Unfortunately, I won't have time to investigate for a while.

Re: [PATCH] Don't dereference NULL upon lookup_tree failure.

[Junio C Hamano]

Jim Meyering <jim@meyering.net> writes:

>>> Junio C Hamano <gitster@pobox.com> wrote:
>>> ...
>> Independent of who creates a "invalid repository", our tools
>> should be prepared to be fed bad data and not get upset.
>> Somebody in our code read a non-commit (or it did not read
>> anything) and told the object layer it is a commit.  That
>> codepath needs to be tightened up, no?
>
> Yes, of course.
> Unfortunately, I won't have time to investigate for a while.

Although I haven't seen the problematic repository, I think what
is going on is that the repository has objects with pointers to
other objects labelled with wrong types.

For example, a tag can tag any type of object.

    $ git cat-file tag v1.5.4-rc1
    object 74f6b03c5c8fceef416de9f9a18e5d64712b6d96
    type commit
    tag v1.5.4-rc1
    tagger Junio C Hamano <gitster@pobox.com> 1198114975 -0800

    GIT 1.5.4-rc1
    ...

When parsing such a tag object, it instantiates an in-core tag
object that has a pointer to an in-core commit object, because
the tag says that it points at commit 74f6b0, without reading
the tagged object itself.  This is to allow the user of that
in-core tag object to ask "what do you point at" and get an
in-core object that represents the pointee.  If (and only if)
the user choose to follow that pointer and look at the contents
of the pointee, it can then call parse_object() on that in-core
object.  In other words, the object layer is designed to work
lazily.

So if you earlier parsed such a tag but if the object 74f6b0
were an object of different type, we would hit the codepath you
identified that leads to a NULL dereference.  And "lying caller"
cannot really do much better (except perhaps actually validating
the presense and the type of the object, which would incur
additional overhead).  The situation is the same if:

 - a tree object records an entry with: 
   - mode bits 040000 pointing at an object that is not a tree; or
   - mode bits 100(644|755) pointing at an object that is not a blob; or
   - mode bits 160000 pointing at an object that is not a commit; or
 - a commit object has a "parent" line that is not a commit; or
 - a commit object has a "tree" line that is not a tree.

Also we cannot just say "then we would take safety over
overhead" and make the lying callers validate the pointee;
subproject commits pointed by gitlinks (i.e. entry with 160000
mode bits in a tree object) are not even required to be
available.

Your fix is all we can sensibly do. I however think you would
need similar fix to the same function for other object types, as
they dereference a potentially NULL pointer the same way.

Re: [PATCH] Don't dereference NULL upon lookup_tree failure.

[Jim Meyering]

Junio C Hamano <gitster@pobox.com> wrote:
...
> Your fix is all we can sensibly do. I however think you would
> need similar fix to the same function for other object types, as
> they dereference a potentially NULL pointer the same way.

Good point.  Patch below.  I wondered if these lookup functions have
always been able to return NULL, since there are many remaining uses
where the possibility of a NULL return value is not handled.
Here are a few that stood out in the output of a quick search:

  git-grep -E -A3 'lookup_(commit|tag|blob|tree) *\('

Note: I did not look at cases where the return value is an argument
to some other function.

------------------
builtin-fmt-merge-msg.c:
		head = lookup_commit(head_sha1);
		...
		for (i = 0; i < origins.nr; i++)
			shortlog(origins.list[i], origins.payload[i],
					head, &rev, limit);
------------------
builtin-read-tree.c:                    struct tree *subtree = lookup_tree(entry.
sha1);
builtin-read-tree.c-                    if (!subtree->object.parsed)

------------------
combine-diff.c: struct commit *commit = lookup_commit(sha1);
combine-diff.c- struct commit_list *parents;

------------------
http-push.c:    struct commit *head = lookup_commit(head_sha1);
http-push.c:    struct commit *branch = lookup_commit(branch_sha1);
http-push.c-    struct commit_list *merge_bases = get_merge_bases(head, branch, 1);

------------------
reachable.c:    struct tree *tree = lookup_tree(sha1);
reachable.c-    add_pending_object(revs, &tree->object, "");

------------------
tag.c:          item->tagged = &lookup_blob(sha1)->object;
tag.c-  } else if (!strcmp(type, tree_type)) {
tag.c:          item->tagged = &lookup_tree(sha1)->object;
tag.c-  } else if (!strcmp(type, commit_type)) {
tag.c:          item->tagged = &lookup_commit(sha1)->object;
tag.c-  } else if (!strcmp(type, tag_type)) {
tag.c:          item->tagged = &lookup_tag(sha1)->object;
tag.c-  } else {

--------------------------
unpack-trees.c:                         struct tree *tree = lookup_tree(posns[i]->sha1);
...
unpack-trees.c-                         parse_tree(tree);


Here's the revised patch:

==================================================================
From 94152217e8e57d3932b4ba6f7ee014da1f4346d3 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Fri, 21 Dec 2007 11:56:32 +0100
Subject: [PATCH] Don't dereference NULL upon lookup failure.


Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 object.c |   42 +++++++++++++++++++++++++++++-------------
 1 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/object.c b/object.c
index 16793d9..9945b25 100644
--- a/object.c
+++ b/object.c
@@ -138,27 +138,43 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t

 	if (type == OBJ_BLOB) {
 		struct blob *blob = lookup_blob(sha1);
-		parse_blob_buffer(blob, buffer, size);
-		obj = &blob->object;
+		if (!blob)
+		    obj = NULL;
+		else {
+		    parse_blob_buffer(blob, buffer, size);
+		    obj = &blob->object;
+		}
 	} else if (type == OBJ_TREE) {
 		struct tree *tree = lookup_tree(sha1);
-		obj = &tree->object;
-		if (!tree->object.parsed) {
-			parse_tree_buffer(tree, buffer, size);
-			eaten = 1;
+		if (!tree)
+		    obj = NULL;
+		else {
+		    obj = &tree->object;
+		    if (!tree->object.parsed) {
+			    parse_tree_buffer(tree, buffer, size);
+			    eaten = 1;
+		    }
 		}
 	} else if (type == OBJ_COMMIT) {
 		struct commit *commit = lookup_commit(sha1);
-		parse_commit_buffer(commit, buffer, size);
-		if (!commit->buffer) {
-			commit->buffer = buffer;
-			eaten = 1;
+		if (!commit)
+		    obj = NULL;
+		else {
+		    parse_commit_buffer(commit, buffer, size);
+		    if (!commit->buffer) {
+			    commit->buffer = buffer;
+			    eaten = 1;
+		    }
+		    obj = &commit->object;
 		}
-		obj = &commit->object;
 	} else if (type == OBJ_TAG) {
 		struct tag *tag = lookup_tag(sha1);
-		parse_tag_buffer(tag, buffer, size);
-		obj = &tag->object;
+		if (!tag)
+		    obj = NULL;
+		else {
+		    parse_tag_buffer(tag, buffer, size);
+		    obj = &tag->object;
+		}
 	} else {
 		warning("object %s has unknown type id %d\n", sha1_to_hex(sha1), type);
 		obj = NULL;
--
1.5.4.rc1.16.g60f3b

Re: [PATCH] Don't dereference NULL upon lookup_tree failure.

[Junio C Hamano]

Thanks.