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

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

On 03/07/18 15:34, Jeff King wrote:
> On Fri, Jun 29, 2018 at 02:10:59AM +0100, Ramsay Jones wrote:
> 
>> On 28/06/18 23:03, Jeff King wrote:
>>> On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote:
>> [snip]
>>> Yes, it can go in quickly. But I'd prefer not to keep it in the long
>>> term if it's literally doing nothing.
>>
>> Hmm, I don't think you can say its doing nothing!
>>
>>     "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."
>>
>> ... is no less true, with or without additional patches! ;-)
> 
> True that we don't even bother doing the parsing with your patch. But I
> think I talked myself out of that part being a significant savings
> elsewhere.

[Sorry for the late reply - watching football again!]

I'm not interested in any savings - it would have to be a pretty
wacky repo for there to be much in the way of savings!

Simply, I have found (for many different reasons) that, if there
is no good reason to execute some code, it is _far_ better to not
do so! ;-)

> I guess it would be OK to leave it in. It just feels like it would be
> vestigial after the rest of the patches.
> 
[snip]

>>> Yes, it would include any syntax error. I also have a slight worry about
>>> that, but nobody seems to have screamed _yet_. :)
>>
>> Hmm, I don't think we can ignore this. :(
> 
> I'm not sure. This has been running on every push to GitHub for the past
> 6 weeks, and this is the first report. It's hard to say what that means,
> and technically speaking of course this _is_ a regression.

Hmm, are you concerned about old clients 'transmitting' the
bad data via large hosting sites? (New clients would complain
about a dodgy .gitmodules file, no matter were it came from,
right?)

Has the definition of the config file syntax changed recently?
If not, then old client versions will see the same syntax errors
as recently 'fixed' versions. So they should error out without
'looking' at the bad data, right? (ignoring the 'lets fix the
obvious syntax error' idea).

> There's a nearby thread of interest, too, which I cc'd you on:
> 
>   https://public-inbox.org/git/20180703070650.b3drk5a6kb4k4tnp@glandium.org/

Yeah, I don't quite follow what's going on there - I would have
to read up some more. ;-)

>> So, FWIW, Ack!
>>
>> [I still think my original patch, with the 'to_be_skipped'
>> function name changed to 'object_on_skiplist', should be
>> the first patch of the series!]
> 
> Thanks. If we're going to do any loosening, I think we may want to
> address that _first_, so it can go directly on top of the patches in
> v2.17.1 (because it's a bigger issue than the stray message, IMHO).

Agreed. I probably haven't given it sufficient thought, but my
immediate reaction is to loosen the check - I don't see how
this could be exploited to significantly reduce security. (My lack
of imagination has been noted several times in the past, however!)

Having said that, I am no security expert, so I will let those who
have security expertise decide what is best to do in this situation.

Thanks!

ATB,
Ramsay Jones