Re: [PATCH] object-file: fix race in object collision check

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Dec 30, 2024 at 06:40:53AM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> 
>> > By definition, two files cannot collide with each other when one of them
>> > has been removed. We can thus trivially fix the issue by ignoring ENOENT
>> > when opening either of the files we're about to check for collision.
>> 
>> Thanks for digging it down to the cause.
>> 
>> It is more like even if these two files collided (i.e. have the same
>> name based on what the hash function says, with different contents),
>> when one of them has been removed, we have no way to check if the
>> collision is benign, and even if it were not, we cannot do anything
>> about it, isn't it?
>
> Depends on what "benign" means in this context, I guess. We can only
> assert the most trivial case of it being "benign", namely that we have
> computed a packfile that actually is the exact same. This is also going
> to be the most common case, as everything else would depend on a
> cryptographic collision of the packfile contents. And in that case... we
> cannot do anything about it, yes.

Yes, the whole point of this collision check is to notice the rare
case where we are under attack with cryptographic collision, so
"most of the time it is OK so not being able to check is fine" is
not an impression we want to give readers.

> The idea would be to only handle ENOENT for the second case. But in the
> end I don't think it's worth the complexity because `check_collision()`
> is used before rename(3p)ing the former into place, and that function
> would already notice ENOENT anyway. So we would eventually just die the
> same.

Thanks for thinking it through.  Queued.