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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 6, 2019, 16:57 UTC
Message-ID: <xmqqo8zxnz0m.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190906061919.GA5122@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

> 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.

Well, either we explicitly die in here, or let the caller segfault. 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.

So, after fixing the above, we may safely be able to die inside get_commit_tree() instead of returning NULL.

By the way, I think get_commit_tree() and parse_commit() in fsck should always use the value obtained from the underlying object and bypass any caches like commit graph---if they pay attention to the caches, they should be fixed. Secondary caches like commit graph should of course be validated against what are recorded in the underlying object, but that should be done separately.