Re: [PATCH v4 1/8] finalize_object_file(): check for name collision before renaming

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> ... But in practice it is
> expanding the definition of "what is already on disk" to be the point
> that the function is called.

Yeah, it is a reasonable argument for this additional protection.
It does not make things worse.  All it takes is for the attacker to
come a bit earlier to defeat the link/unlink dance, so doing it "the
right way" does not make it fundamentally safer.

I hope all TOCTOU races can be explained away this way ;-).

> Co-authored-by: Jeff King <peff@peff.net>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  object-file.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 968da27cd41..38407f468a9 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1937,6 +1937,7 @@ static void write_object_file_prepare_literally(const struct git_hash_algo *algo
>   */
>  int finalize_object_file(const char *tmpfile, const char *filename)
>  {
> +	struct stat st;
>  	int ret = 0;
>  
>  	if (object_creation_mode == OBJECT_CREATION_USES_RENAMES)
> @@ -1957,9 +1958,12 @@ int finalize_object_file(const char *tmpfile, const char *filename)
>  	 */
>  	if (ret && ret != EEXIST) {
>  	try_rename:
> -		if (!rename(tmpfile, filename))
> +		if (!stat(filename, &st))
> +			ret = EEXIST;
> +		else if (!rename(tmpfile, filename))
>  			goto out;
> -		ret = errno;
> +		else
> +			ret = errno;
>  	}
>  	unlink_or_warn(tmpfile);
>  	if (ret) {