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
