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

[Jeff King <peff@peff.net>]

On Fri, Sep 06, 2019 at 10:11:09AM -0700, Junio C Hamano wrote:

> > Is there even a single caller that is prepared to react to NULL?
> >
> >     Answer. There is a single hit inside fsck.c that wants to report
> >     an error without killing ourselves in fsck_commit_buffer().  I
> >     however doubt its use of get_commit_tree() is correct in the
> >     first place.  The function is about validating the commit object
> >     payload manually, without trusting the result of parse_commit(),
> >     and it does read the object name of the tree object; the call to
> >     get_commit_tree() used for reporting the error there should
> >     probably become has_object() on the tree_oid.
> 
> At least we need to ensure, not just has_object(), but the object
> indeed claims to be a tree object.  It is OK if it is a corrupt
> tree object---we'll catch its brokenness separately anyway.
> 
>     Hmm, the should we also tolerate the pointed object to be broken
>     in a way that it is not even able to claim to be a tree object?
>     That would mean that has_object() is sufficient to check here.
> 
> OK, so... 

We'd do that check later, during fsck_walk_commit(). Which ironically
calls get_commit_tree() without checking for NULL, and would likely
segfault. ;)

-Peff