Re: [PATCH] fsck: check skiplist for object in fsck_blob()

From: Jeff King <peff@peff.net>
Date: Jun 28, 2018, 17:45 UTC
Message-ID: <20180628174501.GC31766@sigill.intra.peff.net>
In-Reply-To: <0a18acbd-0124-1c92-0046-05b8b035dd28@ramsayjones.plus.com>

On Thu, Jun 28, 2018 at 05:56:18PM +0100, Ramsay Jones wrote:

Show 7 quoted lines

> > Yeah, this solution seems sensible. Given that we would never report any
> > error for that blob, there is no point in even looking at it. I wonder
> > if we ought to do the same for other types, too. Is there any point in
> > opening a tree that is in the skiplist?
> 
> Note that the 'blob' object has already been 'loaded' at this
> point anyway (and the basic 'object' checks have been done).

Yeah, you're right. If we wanted to avoid that, we should prevent it from entering the gitmodules_found list in the first place.

Of course, we'd generally still load it once anyway in order to check the sha1. So really, the most we could do is prevent it from being loaded a _second_ time for no reason during the fsck_finish() stage.

But for the reasons I gave in the fsck_finish() patches (like pack ordering), we will quite often end up hitting it in the first pass anyway. So it's probably not worth spending too much time trying to optimize it.

And thinking on that, my "should we do this for trees" is pretty dumb, too. We load them and compute their sha1 anyway (I _think_ bitrot checks like that are totally independent of skiplist). So all we'd be saving is walking the buffer entries.

Show 12 quoted lines

> I did think about this, briefly, but decided that we should
> only 'skip' the leaf nodes (blobs). (So, when processing
> commits, trees and tags, we should not report an error for
> that object-id, but that should not stop us from checking
> the tree object associated with a commit, just because of
> a problem with the commit message).
> 
> [Oh, wait - Hmm, each object could be checked independently
> of all others and not used for any object traversal right?
> (e.g. using packfile index). I saw fsck_walk() and didn't
> look any further ... Ah, broken link check, ... I obviously
> need to read the code some more ... :-D ]

Yes, I've been confused by this code before. I'm still not sure I totally understand it. ;)

Show 8 quoted lines

> > One thing we could do is turn the parse failure into a noop. The main
> > point of the check is to protect people against the malicious
> > .gitmodules bug. If the file can't be parsed, then it also can't be an
> > attack vector (assuming the parser used for the fsck check and the
> > parser used by the victim behave identically).
> 
> Hmm, yeah, but I would have to provide a means of suppressing
> the config parser error messages. Something I wanted to avoid. :(

Yes, though I don't think it's too bad. We already have a "die_on_error" flag in the config code. I think it just needs to become a tristate: die/error/silent (and probably get passed in via config_options, since I think we tie it right now to the file/blob source).

> Junio, do you want me to address the above 'rejected push'
> issue in this patch, or with a follow-up patch? (It should
> be a pretty rare problem - famous last words!)

Hmm, if we end up doing the config thing above, then this patch would become unnecessary.

And I think doing that would help _all_ cases, even ones without a skiplist. They don't need to see random config error messages either, even if we do eventually report an fsck error.

-Peff