Re: [PATCH v4 1/8] finalize_object_file(): check for name collision before renaming
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 25, 2024, 17:02 UTC
- Message-ID
- <xmqq4j63psy0.fsf@gitster.g>
- In-Reply-To
- <6f1ee91fff315678fef39a54220eae91632d2df9.1727199118.git.me@ttaylorr.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 ;-).
Show 34 quoted lines
> 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) {