diff --git a/intern/utfconv/utf_winfunc.cc b/intern/utfconv/utf_winfunc.cc index 673be18b139..70697b11b84 100644 --- a/intern/utfconv/utf_winfunc.cc +++ b/intern/utfconv/utf_winfunc.cc @@ -72,14 +72,17 @@ int uaccess(const char *filename, int mode) return r; } -int urename(const char *oldname, const char *newname) +int urename(const char *oldname, const char *newname, const bool do_replace) { int r = -1; UTF16_ENCODE(oldname); UTF16_ENCODE(newname); if (oldname_16 && newname_16) { - r = _wrename(oldname_16, newname_16); + /* Closer to UNIX `rename` behavior, as it at least allows to replace an existing file. + * Return value logic is inverted however (returns non-zero on sucess, 0 on failure). + * Note that the operation will still fail if the 'newname' existing file is opened anywhere. */ + r = (MoveFileExW(oldname_16, newname_16, do_replace ? MOVEFILE_REPLACE_EXISTING : 0) == 0); } UTF16_UN_ENCODE(newname); diff --git a/intern/utfconv/utf_winfunc.hh b/intern/utfconv/utf_winfunc.hh index a5255adeb47..9d0f8b9c23a 100644 --- a/intern/utfconv/utf_winfunc.hh +++ b/intern/utfconv/utf_winfunc.hh @@ -18,7 +18,7 @@ FILE *ufopen(const char *filename, const char *mode); int uopen(const char *filename, int oflag, int pmode); int uaccess(const char *filename, int mode); -int urename(const char *oldname, const char *newname); +int urename(const char *oldname, const char *newname, const bool do_replace); char *u_alloc_getenv(const char *varname); void u_free_getenv(char *val); diff --git a/source/blender/blenlib/BLI_fileops.h b/source/blender/blenlib/BLI_fileops.h index 9c76c4a2731..09e487d9f5a 100644 --- a/source/blender/blenlib/BLI_fileops.h +++ b/source/blender/blenlib/BLI_fileops.h @@ -52,18 +52,36 @@ int BLI_copy(const char *path_src, const char *path_dst) ATTR_NONNULL(); int BLI_path_move(const char *path_src, const char *path_dst) ATTR_NONNULL(); /** - * Rename a file or directory. + * Rename a file or directory, unless `to` already exists. * + * \note This matches Windows `rename` logic, _not_ Unix one. It does not allow to replace an + * existing target. Use #BLI_rename_overwrite instead if existing file should be replaced. + * + * \param from: The path to rename from (return failure if it does not exist). + * \param to: The destination path. * \return zero on success (matching 'rename' behavior). */ -int BLI_rename(const char *from, const char *to); +int BLI_rename(const char *from, const char *to) ATTR_NONNULL(); /** - * Rename a file or directory. + * Rename a file or directory, replacing target `to` path if it exists. * - * \warning It's up to the caller to ensure `from` & `to` don't point to the same file - * as this will result in `to` being deleted to make room for `from` - * (which will then also be deleted). + * \note This matches Unix `rename` logic. It does allow to replace an existing target. Use + * #BLI_rename instead if existing file should never be replaced. However, if `to` is an existing, + * non-empty directory, the operation will fail. + * + * \note There is still no feature-parity between behaviors on Windows and Unix, in case the target + * `to` exists and is opened by some process in the system: + * - On Unix, it will typically succeed + * (see https://man7.org/linux/man-pages/man2/rename.2.html for details). + * - On Windows, it will always fail + * (see https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-movefileexw for + * details). + * + * \warning Due to internal limitation/implementation, on Windows, in case paths point to + * directories, it's up to the caller to ensure that `from` and `to` are not the same directory. + * Since `to` is being deleted to make room for `from`, this will result in `from` being deleted as + * well. * * See #BLI_path_move to move directories. * diff --git a/source/blender/blenlib/intern/fileops_c.cc b/source/blender/blenlib/intern/fileops_c.cc index c51f2b2a74c..91eb3dec0a3 100644 --- a/source/blender/blenlib/intern/fileops_c.cc +++ b/source/blender/blenlib/intern/fileops_c.cc @@ -435,15 +435,6 @@ bool BLI_file_ensure_parent_dir_exists(const char *filepath) } int BLI_rename(const char *from, const char *to) -{ -#ifdef WIN32 - return urename(from, to); -#else - return rename(from, to); -#endif -} - -int BLI_rename_overwrite(const char *from, const char *to) { if (!BLI_exists(from)) { return 1; @@ -462,13 +453,60 @@ int BLI_rename_overwrite(const char *from, const char *to) * In this particular case we would not want to follow symbolic-links as well. * Since this functionality isn't required at the moment, leave this as-is. * Noting it as a potential improvement. */ + + /* NOTE: To avoid the concurrency 'time of check/time of use' (TOC/TOU) issue, this code attemps + * to use available solutions for an 'atomic' (file-system wise) rename operation, instead of + * first checking for an existing `to` target path, and then doing the rename operation if it + * does not exists at the time of check. + * + * Windows (through `MoveFileExW`) by default does not allow replacing an existing path. It is + * however not clear whether its API is exposed to the TOC/TOU issue or not. + * + * On Linux or OSX, to keep operations atomic, special non-standardized variants of `rename` must + * be used, depending on the OS. Note that there may also be failure due to file system not + * supporting this operation, although in practice this should not be a problem in modern + * systems. + * - https://man7.org/linux/man-pages/man2/rename.2.html + * - https://www.unix.com/man-page/mojave/2/renameatx_np/ + * + * BSD systems do not have any such thing currently, and are therefore exposed to the TOC/TOU + * issue. */ + +#ifdef WIN32 + return urename(from, to, false); +#elif defined(__APPLE__) + return renamex_np(from, to, RENAME_EXCL); +#elif defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 28) + /* Most common Linux cases. */ + return renameat2(AT_FDCWD, from, AT_FDCWD, to, RENAME_NOREPLACE); +#else + /* At least all BSD's currently. */ if (BLI_exists(to)) { - if (BLI_delete(to, false, false)) { + return 1; + } + return rename(from, to); +#endif +} + +int BLI_rename_overwrite(const char *from, const char *to) +{ + if (!BLI_exists(from)) { + return 1; + } + +#ifdef WIN32 + /* `urename` from `utfconv` intern utils uses `MoveFileExW`, which allows to replace an existing + * file, but not an existing directory, even if empty. This will only delete empty directories. + */ + if (BLI_is_dir(to)) { + if (BLI_delete(to, true, false)) { return 1; } } - - return BLI_rename(from, to); + return urename(from, to, true); +#else + return rename(from, to); +#endif } #ifdef WIN32 diff --git a/source/blender/blenlib/tests/BLI_fileops_test.cc b/source/blender/blenlib/tests/BLI_fileops_test.cc index 72c927a8c15..fd01e19cd2d 100644 --- a/source/blender/blenlib/tests/BLI_fileops_test.cc +++ b/source/blender/blenlib/tests/BLI_fileops_test.cc @@ -67,15 +67,9 @@ TEST_F(FileOpsTest, rename) BLI_file_touch(test_filepath_src.c_str()); ASSERT_TRUE(BLI_exists(test_filepath_src.c_str())); - /* `test_filepath_dst` does exist now, so regular rename should succeed on Unix, but fail on - * Windows. */ -#ifdef WIN32 + /* `test_filepath_dst` does exist now, so regular rename should fail. */ ASSERT_NE(0, BLI_rename(test_filepath_src.c_str(), test_filepath_dst.c_str())); ASSERT_TRUE(BLI_exists(test_filepath_src.c_str())); -#else - ASSERT_EQ(0, BLI_rename(test_filepath_src.c_str(), test_filepath_dst.c_str())); - ASSERT_FALSE(BLI_exists(test_filepath_src.c_str())); -#endif ASSERT_TRUE(BLI_exists(test_filepath_dst.c_str())); BLI_file_touch(test_filepath_src.c_str()); @@ -94,18 +88,6 @@ TEST_F(FileOpsTest, rename) * * This is expected to succeed on Unix, but fail on Windows. */ int fd_dst = BLI_open(test_filepath_dst.c_str(), O_BINARY | O_RDONLY, 0); -#ifdef WIN32 - ASSERT_NE(0, BLI_rename(test_filepath_src.c_str(), test_filepath_dst.c_str())); - ASSERT_TRUE(BLI_exists(test_filepath_src.c_str())); -#else - ASSERT_EQ(0, BLI_rename(test_filepath_src.c_str(), test_filepath_dst.c_str())); - ASSERT_FALSE(BLI_exists(test_filepath_src.c_str())); -#endif - ASSERT_TRUE(BLI_exists(test_filepath_dst.c_str())); - - BLI_file_touch(test_filepath_src.c_str()); - ASSERT_TRUE(BLI_exists(test_filepath_src.c_str())); - #ifdef WIN32 ASSERT_NE(0, BLI_rename_overwrite(test_filepath_src.c_str(), test_filepath_dst.c_str())); ASSERT_TRUE(BLI_exists(test_filepath_src.c_str())); @@ -138,20 +120,17 @@ TEST_F(FileOpsTest, rename) BLI_dir_create_recursive(test_dirpath_src.c_str()); ASSERT_TRUE(BLI_exists(test_dirpath_src.c_str())); - /* `test_dirpath_dst` now exists, so regular rename should succeed on Unix, but fail on Windows. - */ -#ifdef WIN32 + /* `test_dirpath_dst` now exists, so regular rename should fail. */ ASSERT_NE(0, BLI_rename(test_dirpath_src.c_str(), test_dirpath_dst.c_str())); ASSERT_TRUE(BLI_exists(test_dirpath_src.c_str())); -#else - ASSERT_EQ(0, BLI_rename(test_dirpath_src.c_str(), test_dirpath_dst.c_str())); - ASSERT_FALSE(BLI_exists(test_dirpath_src.c_str())); -#endif ASSERT_TRUE(BLI_exists(test_dirpath_dst.c_str())); -#ifndef WIN32 + /* `test_dirpath_dst` now exists, but is empty, so overwrite rename should suceed. */ + ASSERT_EQ(0, BLI_rename_overwrite(test_dirpath_src.c_str(), test_dirpath_dst.c_str())); + ASSERT_FALSE(BLI_exists(test_dirpath_src.c_str())); + ASSERT_TRUE(BLI_exists(test_dirpath_dst.c_str())); + BLI_dir_create_recursive(test_dirpath_src.c_str()); -#endif ASSERT_TRUE(BLI_exists(test_dirpath_src.c_str())); const std::string test_dir_filepath_src = test_dirpath_src + SEP_STR + file_name_src; @@ -167,13 +146,12 @@ TEST_F(FileOpsTest, rename) ASSERT_FALSE(BLI_exists(test_dir_filepath_src.c_str())); ASSERT_TRUE(BLI_exists(test_dir_filepath_dst.c_str())); - /* `test_dirpath_dst` exists and is not empty, so regular rename should fail on all platforms. */ + /* `test_dirpath_dst` exists and is not empty, so regular rename should fail. */ ASSERT_NE(0, BLI_rename(test_dirpath_src.c_str(), test_dirpath_dst.c_str())); ASSERT_TRUE(BLI_exists(test_dirpath_src.c_str())); ASSERT_TRUE(BLI_exists(test_dirpath_dst.c_str())); - /* `test_dirpath_dst` exists and is not empty, so even overwrite rename should fail on all - * platforms. */ + /* `test_dirpath_dst` exists and is not empty, so even overwrite rename should fail. */ ASSERT_NE(0, BLI_rename_overwrite(test_dirpath_src.c_str(), test_dirpath_dst.c_str())); ASSERT_TRUE(BLI_exists(test_dirpath_src.c_str())); ASSERT_TRUE(BLI_exists(test_dirpath_dst.c_str()));