Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees

[Derrick Stolee <stolee@gmail.com>]

On 9/6/2019 2:19 AM, Jeff King wrote:
> On Thu, Sep 05, 2019 at 06:04:57PM -0400, Taylor Blau wrote:
> 
>> @@ -846,7 +847,11 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,
>>  		if (parse_commit_no_graph(*list))
>>  			die(_("unable to parse commit %s"),
>>  				oid_to_hex(&(*list)->object.oid));
>> -		hashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);
>> +		tree = get_commit_tree_oid(*list);
>> +		if (!tree)
>> +			die(_("unable to get tree for %s"),
>> +				oid_to_hex(&(*list)->object.oid));
>> +		hashwrite(f, tree->hash, hash_len);
> 
> Yeah, I think this is a good stop-gap to protect ourselves, until a time
> when parse_commit() and friends consistently warn us about the breakage.
> 
>> diff --git a/commit.c b/commit.c
>> index a98de16e3d..fab22cb740 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -358,7 +358,8 @@ struct tree *repo_get_commit_tree(struct repository *r,
>>  
>>  struct object_id *get_commit_tree_oid(const struct commit *commit)
>>  {
>> -	return &get_commit_tree(commit)->object.oid;
>> +	struct tree *tree = get_commit_tree(commit);
>> +	return tree ? &tree->object.oid : NULL;
>>  }
> 
> This one in theory benefits lots of other callsites, too, since it means
> we'll actually return NULL instead of nonsense like "8". But grepping
> around for calls to this function, I found literally zero of them
> actually bother checking for a NULL result. So there are probably dozens
> of similar segfaults waiting to happen in other code paths.
> Discouraging.
>
> This is sort-of attributable to my 834876630b (get_commit_tree(): return
> NULL for broken tree, 2019-04-09). Before then it was a BUG(). However,
> that state was relatively short-lived. Before 7b8a21dba1 (commit-graph:
> lazy-load trees for commits, 2018-04-06), we'd have similarly returned
> NULL (and anyway, BUG() is clearly wrong since it's a data error).
> 
> None of which argues against your patches, but it's kind of sad that the
> issue is present in so many code paths. I wonder if we could be handling
> this in a more central way, but I don't see how short of dying.

This is due to the mechanical conversion from using commit->tree->oid to
get_commit_tree_oid(commit). Those consumers were not checking if the
tree pointer was NULL, either, but they probably assumed that the
parse_commit() call would have failed earlier. Now that we are using this
method (for performance reasons to avoid creating too many 'struct tree's)
it makes sense to convert some of them to checking the return value more
carefully.

If we wanted the more invasive change of modifying every caller to check
 NULL and respond appropriately, that would be _best_, but is probably
not worth the effort.

Thanks,
-Stolee