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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 30, 2024, 14:40 UTC
Message-ID: <xmqqy0zxz11m.fsf@gitster.g>
In-Reply-To: <20241230-b4-pks-object-file-racy-collision-check-v1-1-11571294e60a@pks.im>

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?

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?

Thanks, will queue.

Show 44 quoted lines

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> Hi,
>
> this patch is the follow-up for [1], where I've mentioned a couple of CI
> flakes that happen rather regularly. As it turns out, this race was a
> real bug hiding in the newly nitroduced object collision check in case
> one of the files got unlinked while performing the check.
>
> Thanks!
>
> Patrick
>
> [1]: <Z2-2dbYVuuLxpNmK@pks.im>
> ---
>  object-file.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 5b792b3dd42cecde43a1b18abc164fd368cbcd69..f84dcd2f2a7b88716ab47bc00ee7a605a82e8d21 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1978,13 +1978,15 @@ static int check_collision(const char *filename_a, const char *filename_b)
>  
>  	fd_a = open(filename_a, O_RDONLY);
>  	if (fd_a < 0) {
> -		ret = error_errno(_("unable to open %s"), filename_a);
> +		if (errno != ENOENT)
> +			ret = error_errno(_("unable to open %s"), filename_a);
>  		goto out;
>  	}
>  
>  	fd_b = open(filename_b, O_RDONLY);
>  	if (fd_b < 0) {
> -		ret = error_errno(_("unable to open %s"), filename_b);
> +		if (errno != ENOENT)
> +			ret = error_errno(_("unable to open %s"), filename_b);
>  		goto out;
>  	}
>  
>
> ---
> base-commit: 306ab352f4e98f6809ce52fc4e5d63fb947d0635
> change-id: 20241230-b4-pks-object-file-racy-collision-check-62a8d1588116