git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jim Meyering <jim@meyering.net>
Date
Dec 21, 2007, 22:54 UTC
Message-ID
<87hcibveut.fsf@rho.meyering.net>
In-Reply-To
<7v4peb4qbj.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
Show 37 quoted lines
> 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
Previous: Junio C HamanoNext: Junio C Hamano
Message 3 of 10 in “Don't dereference NULL upon lookup_tree failure.”
  1. Don't dereference NULL upon lookup_tree failure.Jim Meyering, Dec 21, 2007
  2. Junio C HamanoDec 21, 2007
  3. Jim MeyeringDec 21, 2007
  4. Junio C HamanoDec 21, 2007
  5. Jim MeyeringDec 21, 2007
  6. Junio C HamanoDec 21, 2007
  7. Jim MeyeringDec 22, 2007
  8. Junio C HamanoDec 22, 2007
  9. Jim MeyeringDec 22, 2007
  10. Junio C HamanoDec 22, 2007

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.