From: Junio C Hamano Date: Wed, 25 Sep 2024 17:02:47 GMT Subject: Re: [PATCH v4 1/8] finalize_object_file(): check for name collision before renaming Message-ID: In-Reply-To: <6f1ee91fff315678fef39a54220eae91632d2df9.1727199118.git.me@ttaylorr.com> Taylor Blau 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 > Signed-off-by: Jeff King > Signed-off-by: Taylor Blau > --- > 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) {