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

[Patrick Steinhardt <ps@pks.im>]

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.

> I do like the simplicity of the solution.  I wonder given bad enough
> race, we could fall into a case where both files are missing?

I was wondering about that, too, but it would very much feel like a bug
to me if that were ever to happen. So I briefly considered whether I
should treat the passed-in filenames differently: 

  - One that must exist non-racily. This is our temporary object or
    packfile that we want to move into place.

  - And one that may have been removed racily. This is our target file
    path that we want to overwrite, unless there is a collision.

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.

Patrick