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

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 28/06/18 18:45, Jeff King wrote:
> On Thu, Jun 28, 2018 at 05:56:18PM +0100, Ramsay Jones wrote:
[snip]
>>> 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).

Yes, but this code is already very crufty - and I'm just
waiting for someone to want to have per-repo/submodule
config parsing i... ;-)

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

I was thinking of timing - the current patch could go
in quickly to solve the immediate problem (eg. in maint).

Also, it does not hurt to do this _as well as_ suppress
the config errors.

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

Oh, yes, I agree. You will have noticed that it was my
first suggested solution. (I have started writing that
patch a few times, but it just makes me want to throw
the current code away and start again)!

Hmm, BTW, the 'rejected push' problem would include *any*
'.gitmodules' blob that contained a syntax error, right?

Maybe it won't be as rare as all that! (Imagine not being
able to push due to a compiler error/warning in source files.
How irritating would that be! :-P ).

(if we fix this, you could hide some nefarious settings
after an obvious syntax error - then get the victim to
fix the syntax error ...)

ATB,
Ramsay Jones