[PATCH v3 1/9] finalize_object_file(): check for name collision before renaming
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Sep 6, 2024, 19:46 UTC
- Message-ID
- <738b1eb17b47c11ae0185b5328bac549dbf05081.1725651952.git.me@ttaylorr.com>
- In-Reply-To
- <cover.1725651952.git.me@ttaylorr.com>
We prefer link()/unlink() to rename() for object files, with the idea that we should prefer the data that is already on disk to what is incoming. But we may fall back to rename() if the user has configured us to do so, or if the filesystem seems not to support cross-directory links. This loses the "prefer what is on disk" property.
We can mitigate this somewhat by trying to stat() the destination filename before doing the rename. This is racy, since the object could be created between the stat() and rename() calls. But in practice it is expanding the definition of "what is already on disk" to be the point that the function is called. That is enough to deal with any potential attacks where an attacker is trying to collide hashes with what's already in the repository.
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 c5994202ba0..683e6b2a0bd 100644 --- a/object-file.c +++ b/object-file.c @@ -1904,6 +1904,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) @@ -1924,9 +1925,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) {
-- 2.46.0.430.gca674632b70