diff --git a/components/fatfs/Kconfig b/components/fatfs/Kconfig index 55069640fe0..e662dd49734 100644 --- a/components/fatfs/Kconfig +++ b/components/fatfs/Kconfig @@ -321,6 +321,38 @@ 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. + + Moving a directory into its own subtree is also rejected with EINVAL, as + POSIX requires. f_rename() does not check for this and would link the + directory into its own tree, losing its contents; without this option that + call still succeeds and corrupts the volume. + + 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_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 ca9aa172076..4ed4e860f9a 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" @@ -267,6 +268,171 @@ 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_REPLACES_DESTINATION +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)); + + /* 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_REPLACES_DESTINATION + 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..be2195737e5 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,7 @@ from pytest_embedded_idf.utils import idf_parametrize 'fastseek', 'auto_fsync', 'dyn_buffers', + 'posix_rename', ], ) @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/vfs/vfs_fat.c b/components/fatfs/vfs/vfs_fat.c index 939c4215ac9..55e95aed405 100644 --- a/components/fatfs/vfs/vfs_fat.c +++ b/components/fatfs/vfs/vfs_fat.c @@ -981,6 +981,139 @@ cleanup: return ret; } + +#ifdef CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION +/* + * True if `path` names something inside the directory `dir`, that is, `dir` is + * a prefix of `path` ending at a component boundary. FAT names are matched + * case-insensitively, and ASCII case is folded here; a prefix that differs + * only in the case of a non-ASCII character is not recognised, which merely + * leaves such a rename to fail the way it does without this check. + */ +static bool fat_path_is_within(const char *dir, const char *path) +{ + size_t i; + + for (i = 0; dir[i] != '\0'; i++) { + char a = dir[i]; + char b = path[i]; + + if (b == '\0') { + return false; + } + if (a >= 'a' && a <= 'z') { + a -= 'a' - 'A'; + } + if (b >= 'a' && b <= 'z') { + b -= 'a' - 'A'; + } + if (a != b) { + return false; + } + } + + /* `dir` is exhausted: what follows in `path` decides. A trailing separator + * on `dir` has already consumed the boundary. */ + if (i > 0 && dir[i - 1] == '/') { + return path[i] != '\0'; + } + return path[i] == '/' && path[i + 1] != '\0'; +} + +/* + * 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); +} + +static int vfs_fat_rename(void* ctx, const char *src, const char *dst) +{ + vfs_fat_ctx_t* fat_ctx = (vfs_fat_ctx_t*) ctx; + _lock_acquire(&fat_ctx->lock); + prepend_drive_to_path(fat_ctx, &src, &dst); + + int posix_errno = 0; + + if (fat_path_is_within(src, dst)) { + /* Moving a directory inside itself would detach its contents and link + * the directory into its own tree; f_rename() does not check for this + * and would corrupt the volume. POSIX asks for EINVAL, unless the + * source is not a directory at all, in which case the destination + * merely uses a file as a directory component. */ + FILINFO src_info; + FRESULT stat_res = f_stat(src, &src_info); + posix_errno = (stat_res != FR_OK) ? fresult_to_errno(stat_res) + : (src_info.fattrib & AM_DIR) ? EINVAL + : ENOTDIR; + } else { + FRESULT res = f_rename(src, dst); + if (res == FR_EXIST) { + posix_errno = vfs_fat_replace_destination(src, dst); + } else if (res != FR_OK) { + posix_errno = fresult_to_errno(res); + } + } + + _lock_release(&fat_ctx->lock); + + if (posix_errno != 0) { + ESP_LOGD(TAG, "%s: errno=%d", __func__, posix_errno); + errno = posix_errno; + return -1; + } + return 0; +} + +#else // CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION + static int vfs_fat_rename(void* ctx, const char *src, const char *dst) { vfs_fat_ctx_t* fat_ctx = (vfs_fat_ctx_t*) ctx; @@ -998,6 +1131,8 @@ static int vfs_fat_rename(void* ctx, const char *src, const char *dst) return 0; } +#endif // CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION + static DIR* vfs_fat_opendir(void* ctx, const char* name) { vfs_fat_ctx_t* fat_ctx = (vfs_fat_ctx_t*) ctx;