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

On 28/06/18 18:45, Jeff King wrote:

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

[snip]

quoted

quoted

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

quoted

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