diff --git a/components/fatfs/Kconfig b/components/fatfs/Kconfig index 55069640fe0..9748be04c5c 100644 --- a/components/fatfs/Kconfig +++ b/components/fatfs/Kconfig @@ -321,6 +321,56 @@ menu "FAT Filesystem support" This ensures that the link operation is atomic, but may cause performance for large files. It may create less fragmented file copy. + config FATFS_VFS_RENAME_REPLACES_DESTINATION + bool "Make rename() replace an existing destination (POSIX behavior)" + default n + help + POSIX rename() silently replaces the destination if it already exists, + while FatFs' f_rename() refuses with FR_EXIST, so by default rename() on a + FAT mount fails with EEXIST where it would succeed on other file systems. + + Enable this option to emulate the POSIX behavior: when the destination + exists, it is removed and the rename is retried, with the whole sequence + performed under the same lock as the rename itself. The POSIX rules on what + may replace what are applied first, so that a directory is never silently + removed to make way for a file: + + - a directory can only replace an empty directory (ENOTEMPTY otherwise), + - renaming a directory onto a file fails with ENOTDIR, + - renaming a file onto a directory fails with EISDIR. + + Without this option all of these fail with EEXIST instead. + + Note that the emulation is not atomic, and that it cannot honour the POSIX + guarantee that a failed rename leaves an instance of the destination in + place. FAT offers no way to replace a directory entry in a single step, so + an interruption (such as a power loss) between removing the destination and + completing the rename can leave neither name present. Code that must not + lose the destination should copy to a temporary name and rename over it. + + config FATFS_VFS_RENAME_REJECTS_SELF_NESTING + bool "Make rename() reject moving a directory into its own subtree" + default n + help + Moving a directory into its own subtree, such as renaming "/a" to "/a/b", + cannot be represented on FAT. f_rename() does not check for it and links + the directory into its own tree, after which the directory is reachable + only from inside itself and the volume is corrupt. + + Enable this option to detect the case and fail with EINVAL, as POSIX + requires, leaving the volume untouched. + + FatFs matches names by directory entry rather than by path bytes, so a + destination spelled differently from the source - through an 8.3 alias, or + through a name differing only in the case of a non-ASCII character - still + denotes the same directory. The check therefore resolves each component of + the destination and compares start clusters, rather than comparing the two + paths as strings. + + The check only runs when the source is a directory, so renaming a file + costs one extra directory open that fails immediately. Renaming a directory + additionally opens each component of the destination path. + config FATFS_USE_DYN_BUFFERS bool "Use dynamic buffers" default y diff --git a/components/fatfs/test_apps/flash_wl/main/test_fatfs_flash_wl.c b/components/fatfs/test_apps/flash_wl/main/test_fatfs_flash_wl.c index d5ac955fd81..9fb8f6a4666 100644 --- a/components/fatfs/test_apps/flash_wl/main/test_fatfs_flash_wl.c +++ b/components/fatfs/test_apps/flash_wl/main/test_fatfs_flash_wl.c @@ -10,6 +10,7 @@ #include #include #include +#include #include #include "unity.h" #include "esp_partition.h" @@ -265,6 +266,195 @@ TEST_CASE("(WL) link copies a file, rename moves a file", "[fatfs][wear_levellin test_teardown(); } +TEST_CASE("(WL) rename to an existing destination", "[fatfs][wear_levelling]") +{ + test_setup(); + + const char *src = "/spiflash/ren_src.txt"; + const char *dst = "/spiflash/ren_dst.txt"; + + FILE *f = fopen(src, "w"); + TEST_ASSERT_NOT_NULL(f); + TEST_ASSERT_TRUE(fputs("source", f) >= 0); + TEST_ASSERT_EQUAL(0, fclose(f)); + + f = fopen(dst, "w"); + TEST_ASSERT_NOT_NULL(f); + TEST_ASSERT_TRUE(fputs("destination", f) >= 0); + TEST_ASSERT_EQUAL(0, fclose(f)); + + char buf[32]; + errno = 0; + int ret = rename(src, dst); + +#ifdef CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION + /* POSIX behavior: the destination is replaced. */ + TEST_ASSERT_EQUAL(0, ret); + + memset(buf, 0, sizeof(buf)); + f = fopen(dst, "r"); + TEST_ASSERT_NOT_NULL(f); + TEST_ASSERT_NOT_NULL(fgets(buf, sizeof(buf), f)); + TEST_ASSERT_EQUAL(0, fclose(f)); + TEST_ASSERT_EQUAL_STRING("source", buf); + + TEST_ASSERT_NULL(fopen(src, "r")); + + TEST_ASSERT_EQUAL(0, unlink(dst)); +#else + /* Default FatFs behavior: the rename is refused and nothing changes. */ + TEST_ASSERT_EQUAL(-1, ret); + TEST_ASSERT_EQUAL(EEXIST, errno); + + memset(buf, 0, sizeof(buf)); + f = fopen(dst, "r"); + TEST_ASSERT_NOT_NULL(f); + TEST_ASSERT_NOT_NULL(fgets(buf, sizeof(buf), f)); + TEST_ASSERT_EQUAL(0, fclose(f)); + TEST_ASSERT_EQUAL_STRING("destination", buf); + + memset(buf, 0, sizeof(buf)); + f = fopen(src, "r"); + TEST_ASSERT_NOT_NULL(f); + TEST_ASSERT_NOT_NULL(fgets(buf, sizeof(buf), f)); + TEST_ASSERT_EQUAL(0, fclose(f)); + TEST_ASSERT_EQUAL_STRING("source", buf); + + TEST_ASSERT_EQUAL(0, unlink(src)); + TEST_ASSERT_EQUAL(0, unlink(dst)); +#endif + + test_teardown(); +} + +TEST_CASE("(WL) rename obeys the POSIX rules on directories", "[fatfs][wear_levelling]") +{ + test_setup(); + + const char *file = "/spiflash/ren_f.txt"; + const char *dir = "/spiflash/ren_d"; + const char *dir2 = "/spiflash/ren_d2"; + + FILE *f = fopen(file, "w"); + TEST_ASSERT_NOT_NULL(f); + TEST_ASSERT_TRUE(fputs("payload", f) >= 0); + TEST_ASSERT_EQUAL(0, fclose(f)); + TEST_ASSERT_EQUAL(0, mkdir(dir, 0755)); + + struct stat st; + + /* A file may not replace a directory, and the directory must survive. */ + errno = 0; + TEST_ASSERT_EQUAL(-1, rename(file, dir)); +#ifdef CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION + TEST_ASSERT_EQUAL(EISDIR, errno); +#else + TEST_ASSERT_EQUAL(EEXIST, errno); +#endif + TEST_ASSERT_EQUAL(0, stat(dir, &st)); + TEST_ASSERT_TRUE(S_ISDIR(st.st_mode)); + + /* A directory may not replace a file, and the file must survive. */ + errno = 0; + TEST_ASSERT_EQUAL(-1, rename(dir, file)); +#ifdef CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION + TEST_ASSERT_EQUAL(ENOTDIR, errno); +#else + TEST_ASSERT_EQUAL(EEXIST, errno); +#endif + TEST_ASSERT_EQUAL(0, stat(file, &st)); + TEST_ASSERT_FALSE(S_ISDIR(st.st_mode)); + + /* A directory may not replace a non-empty directory. */ + TEST_ASSERT_EQUAL(0, mkdir(dir2, 0755)); + f = fopen("/spiflash/ren_d2/occupant.txt", "w"); + TEST_ASSERT_NOT_NULL(f); + TEST_ASSERT_EQUAL(0, fclose(f)); + errno = 0; + TEST_ASSERT_EQUAL(-1, rename(dir, dir2)); +#ifdef CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION + TEST_ASSERT_EQUAL(ENOTEMPTY, errno); +#else + TEST_ASSERT_EQUAL(EEXIST, errno); +#endif + TEST_ASSERT_EQUAL(0, stat("/spiflash/ren_d2/occupant.txt", &st)); + + TEST_ASSERT_EQUAL(0, unlink("/spiflash/ren_d2/occupant.txt")); + + /* A directory may replace an empty one. */ + errno = 0; + int ret = rename(dir, dir2); +#ifdef CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION + TEST_ASSERT_EQUAL(0, ret); + TEST_ASSERT_EQUAL(0, stat(dir2, &st)); + TEST_ASSERT_TRUE(S_ISDIR(st.st_mode)); + TEST_ASSERT_EQUAL(-1, stat(dir, &st)); + TEST_ASSERT_EQUAL(0, rmdir(dir2)); +#else + TEST_ASSERT_EQUAL(-1, ret); + TEST_ASSERT_EQUAL(EEXIST, errno); + TEST_ASSERT_EQUAL(0, rmdir(dir)); + TEST_ASSERT_EQUAL(0, rmdir(dir2)); +#endif + TEST_ASSERT_EQUAL(0, unlink(file)); + + test_teardown(); +} + +/* Only meaningful with the option enabled: without it f_rename() happily moves + * the directory into its own tree, which corrupts the volume, so there is no + * safe way to exercise the case. */ +#ifdef CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING +TEST_CASE("(WL) rename refuses to move a directory into itself", "[fatfs][wear_levelling]") +{ + test_setup(); + + const char *dir = "/spiflash/mv_d"; + TEST_ASSERT_EQUAL(0, mkdir(dir, 0755)); + + struct stat st; + + errno = 0; + TEST_ASSERT_EQUAL(-1, rename(dir, "/spiflash/mv_d/child")); + TEST_ASSERT_EQUAL(EINVAL, errno); + TEST_ASSERT_EQUAL(0, stat(dir, &st)); + TEST_ASSERT_TRUE(S_ISDIR(st.st_mode)); + + /* Nesting is rejected at any depth, not just directly below the source. */ + errno = 0; + TEST_ASSERT_EQUAL(-1, rename(dir, "/spiflash/mv_d/a/b")); + TEST_ASSERT_EQUAL(EINVAL, errno); + + /* The destination is matched by directory entry rather than by path bytes, + * so a spelling that differs only in case is caught as well. */ + errno = 0; + TEST_ASSERT_EQUAL(-1, rename(dir, "/spiflash/MV_D/child")); + TEST_ASSERT_EQUAL(EINVAL, errno); + TEST_ASSERT_EQUAL(0, stat(dir, &st)); + TEST_ASSERT_TRUE(S_ISDIR(st.st_mode)); + + /* The 8.3 alias of a long name denotes the same directory, which a + * comparison of path bytes would not recognise. */ + const char *long_dir = "/spiflash/longdirname"; + TEST_ASSERT_EQUAL(0, mkdir(long_dir, 0755)); + errno = 0; + TEST_ASSERT_EQUAL(-1, rename(long_dir, "/spiflash/LONGDI~1/child")); + TEST_ASSERT_EQUAL(EINVAL, errno); + TEST_ASSERT_EQUAL(0, stat(long_dir, &st)); + TEST_ASSERT_TRUE(S_ISDIR(st.st_mode)); + TEST_ASSERT_EQUAL(0, rmdir(long_dir)); + + /* A name that merely shares a prefix is a different directory, and moving + * the directory elsewhere stays allowed. */ + TEST_ASSERT_EQUAL(0, rename(dir, "/spiflash/mv_dd")); + TEST_ASSERT_EQUAL(0, stat("/spiflash/mv_dd", &st)); + TEST_ASSERT_TRUE(S_ISDIR(st.st_mode)); + TEST_ASSERT_EQUAL(0, rmdir("/spiflash/mv_dd")); + + test_teardown(); +} +#endif // CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING + TEST_CASE("(WL) can create and remove directories", "[fatfs][wear_levelling]") { test_setup(); diff --git a/components/fatfs/test_apps/flash_wl/pytest_fatfs_flash_wl.py b/components/fatfs/test_apps/flash_wl/pytest_fatfs_flash_wl.py index 93a100891be..2a976be16d2 100644 --- a/components/fatfs/test_apps/flash_wl/pytest_fatfs_flash_wl.py +++ b/components/fatfs/test_apps/flash_wl/pytest_fatfs_flash_wl.py @@ -15,6 +15,8 @@ from pytest_embedded_idf.utils import idf_parametrize 'fastseek', 'auto_fsync', 'dyn_buffers', + 'posix_rename', + 'self_nesting', ], ) @idf_parametrize('target', ['esp32', 'esp32c3'], indirect=['target']) diff --git a/components/fatfs/test_apps/flash_wl/sdkconfig.ci.posix_rename b/components/fatfs/test_apps/flash_wl/sdkconfig.ci.posix_rename new file mode 100644 index 00000000000..1a13c70d40c --- /dev/null +++ b/components/fatfs/test_apps/flash_wl/sdkconfig.ci.posix_rename @@ -0,0 +1 @@ +CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION=y diff --git a/components/fatfs/test_apps/flash_wl/sdkconfig.ci.self_nesting b/components/fatfs/test_apps/flash_wl/sdkconfig.ci.self_nesting new file mode 100644 index 00000000000..b85bea1d971 --- /dev/null +++ b/components/fatfs/test_apps/flash_wl/sdkconfig.ci.self_nesting @@ -0,0 +1 @@ +CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING=y diff --git a/components/fatfs/vfs/vfs_fat.c b/components/fatfs/vfs/vfs_fat.c index 939c4215ac9..3279e360c1b 100644 --- a/components/fatfs/vfs/vfs_fat.c +++ b/components/fatfs/vfs/vfs_fat.c @@ -981,6 +981,153 @@ cleanup: return ret; } + +#ifdef CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING +/* + * Start cluster of the directory named by `path`, or 0 if `path` does not name + * a directory. A FAT12/FAT16 root directory has no cluster and reports 0 as + * well, so a zero result carries no identity and must not be compared. + */ +static DWORD fat_dir_start_cluster(const char *path) +{ + FF_DIR dir; + if (f_opendir(&dir, path) != FR_OK) { + return 0; + } + DWORD start_cluster = dir.obj.sclust; + f_closedir(&dir); + return start_cluster; +} + +/* + * Reject moving a directory into its own subtree. f_rename() does not check for + * this and would leave the directory reachable only from inside itself, which + * corrupts the volume. Called with the context lock held and with paths that + * already carry the drive prefix. Returns 0 if the rename may proceed, or the + * errno to report. + * + * FatFs resolves names through directory entries, so a destination spelled + * differently from the source, through an 8.3 alias or a case difference FatFs + * folds, still leads back to the same directory. Each ancestor of `dst` is + * therefore resolved and compared by start cluster rather than by path bytes. + */ +static int vfs_fat_check_self_nesting(const char *src, const char *dst) +{ + DWORD src_cluster = fat_dir_start_cluster(src); + if (src_cluster == 0) { + /* Only a directory has a subtree to be moved into. */ + return 0; + } + + char dst_buf[FILENAME_MAX + 3]; + size_t dst_len = strlen(dst); + if (dst_len >= sizeof(dst_buf)) { + return ENAMETOOLONG; + } + memcpy(dst_buf, dst, dst_len + 1); + + char *cursor = dst_buf; + if (cursor[0] != '\0' && cursor[1] == ':') { + cursor += 2; + } + while (*cursor == '/') { + cursor++; + } + + /* Each separator ends an ancestor of `dst`. `dst` itself is not examined: + * renaming an entry onto itself is not nesting. */ + for (char *sep = strchr(cursor, '/'); sep != NULL; sep = strchr(sep + 1, '/')) { + *sep = '\0'; + DWORD ancestor_cluster = fat_dir_start_cluster(dst_buf); + *sep = '/'; + if (ancestor_cluster != 0 && ancestor_cluster == src_cluster) { + return EINVAL; + } + } + + return 0; +} +#endif // CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING + +#ifdef CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION +/* + * Handle f_rename() refusing an existing destination, applying the POSIX rules + * for what may replace what. Called with the context lock held and with paths + * that already carry the drive prefix. Returns 0 once the rename has been + * carried out, or the errno to report. + * + * f_rename() reports FR_EXIST only when the destination resolves to a + * different directory entry than the source, so renaming an entry to itself, + * including through another spelling of the same name, never gets here and + * succeeds on its own. + */ +static int vfs_fat_replace_destination(const char *src, const char *dst) +{ + FILINFO src_info; + FRESULT fr = f_stat(src, &src_info); + if (fr != FR_OK) { + return fresult_to_errno(fr); + } + FILINFO dst_info; + fr = f_stat(dst, &dst_info); + if (fr != FR_OK) { + return fresult_to_errno(fr); + } + + bool src_is_dir = (src_info.fattrib & AM_DIR) != 0; + bool dst_is_dir = (dst_info.fattrib & AM_DIR) != 0; + + /* POSIX only allows an entry to be replaced by one of the same kind. This + * has to be checked before removing anything: f_unlink() would happily + * delete an empty directory to make way for a file. */ + if (src_is_dir && !dst_is_dir) { + return ENOTDIR; + } + if (!src_is_dir && dst_is_dir) { + return EISDIR; + } + if (dst_info.fattrib & AM_RDO) { + return EACCES; + } + + fr = f_unlink(dst); + if (fr == FR_DENIED && dst_is_dir) { + /* A writable directory is only denied removal while it still has + * entries. */ + return ENOTEMPTY; + } + if (fr != FR_OK) { + return fresult_to_errno(fr); + } + + fr = f_rename(src, dst); + return (fr == FR_OK) ? 0 : fresult_to_errno(fr); +} +#endif // CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION + +/* + * Carry out the rename itself. Called with the context lock held and with paths + * that already carry the drive prefix. Returns 0 on success, or the errno to + * report. + */ +static int vfs_fat_rename_locked(const char *src, const char *dst) +{ +#ifdef CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING + int nesting_errno = vfs_fat_check_self_nesting(src, dst); + if (nesting_errno != 0) { + return nesting_errno; + } +#endif + + FRESULT res = f_rename(src, dst); +#ifdef CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION + if (res == FR_EXIST) { + return vfs_fat_replace_destination(src, dst); + } +#endif + return (res == FR_OK) ? 0 : fresult_to_errno(res); +} + static int vfs_fat_rename(void* ctx, const char *src, const char *dst) { vfs_fat_ctx_t* fat_ctx = (vfs_fat_ctx_t*) ctx; @@ -988,11 +1135,14 @@ static int vfs_fat_rename(void* ctx, const char *src, const char *dst) vfs_fat_invalidate_stat_cache(fat_ctx, src); vfs_fat_invalidate_stat_cache(fat_ctx, dst); prepend_drive_to_path(fat_ctx, &src, &dst); - FRESULT res = f_rename(src, dst); + + int posix_errno = vfs_fat_rename_locked(src, dst); + _lock_release(&fat_ctx->lock); - if (res != FR_OK) { - ESP_LOGD(TAG, "%s: fresult=%d", __func__, res); - errno = fresult_to_errno(res); + + if (posix_errno != 0) { + ESP_LOGD(TAG, "%s: errno=%d", __func__, posix_errno); + errno = posix_errno; return -1; } return 0;