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

On Mon, Dec 30, 2024 at 06:40:53AM -0800, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

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