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

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 30, 2024, 14:50 UTC
Message-ID: <Z3KzHJagr_3Fkz67@pks.im>
In-Reply-To: <xmqqy0zxz11m.fsf@gitster.g>

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

Show 13 quoted lines

> 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