Re: [PATCH 2/2] object-file: retry linking file into place when occluding file vanishes

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 3, 2025, 20:25 UTC
Message-ID: <xmqqsepzprul.fsf@gitster.g>
In-Reply-To: <20250103194058.GE3208749@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> I share Junio's uneasiness with looping forever based on external input
> from the filesystem (even though you _should_ eventually win the race,
> that's not guaranteed, and of course a weird filesystem might confuse
> us).

Yeah, "a weird filesystem" would be a lot more plausible than a determined and accurate attacker to break it. The only thing they have to do is to yield EEXIST when failing link() for some other reason.

Show 27 quoted lines

> Could we put a stop-gap in it like:
>
> diff --git a/object-file.c b/object-file.c
> index 88432cc9c0..262a2f3df2 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -2038,6 +2038,7 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,
>  			       enum finalize_object_file_flags flags)
>  {
>  	int ret;
> +	int retries = 0;
>  
>  retry:
>  	ret = 0;
> @@ -2080,8 +2081,11 @@ int finalize_object_file_flags(const char *tmpfile, const char *filename,
>  		}
>  		if (!(flags & FOF_SKIP_COLLISION_CHECK)) {
>  			ret = check_collision(tmpfile, filename);
> -			if (ret == CHECK_COLLISION_DEST_VANISHED)
> +			if (ret == CHECK_COLLISION_DEST_VANISHED) {
> +				if (retries++ > 5)
> +					return error(_("unable to write repeatedly vanishing file %s"), filename);
>  				goto retry;
> +			}
>  			else if (ret)
>  				return -1;
>  		}

Sounds sensible.

> Otherwise, I think the logic looks good.
>
> -Peff

Thanks.