From 4262553c56866276c66dbccad3698fbc460418ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Rohl=C3=ADnek?= Date: Thu, 3 Sep 2026 09:44:24 +0200 Subject: [PATCH 1/2] feat(fatfs): add option to make rename() replace the destination POSIX rename() silently replaces the destination when it already exists, while FatFs' f_rename() refuses with FR_EXIST. rename() on a FAT mount therefore fails with EEXIST where the same call succeeds on other file systems, and every caller that wants portable behaviour has to remove the destination itself. Add CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION (default n, so existing behaviour is unchanged) to emulate the POSIX semantics: when f_rename() reports FR_EXIST, remove the destination and retry, all under the lock already held for the rename so no other VFS caller observes the gap. The POSIX rules on what may replace what are applied before anything is removed, because f_unlink() deletes empty directories as readily as files and would otherwise discard a directory to make way for a file: renaming a file onto a directory fails with EISDIR, a directory onto a file with ENOTDIR, and a directory onto a non-empty directory with ENOTEMPTY. Without the option all of these keep failing with EEXIST. Moving a directory into its own subtree is rejected with EINVAL as well. f_rename() does not check for this and links the directory into its own tree, losing its contents, so this is a correctness fix rather than a matter of which error is reported. Renaming an entry to itself needs no special handling: f_rename() reports FR_EXIST only when the destination resolves to a different directory entry, so a self-rename, including one written with a different spelling of the same name, already succeeds. The emulation cannot be atomic, nor can it honour the POSIX guarantee that a failed rename leaves an instance of the destination in place: FAT cannot replace a directory entry in one step, so an interruption between removing the destination and completing the rename can leave neither name. This is documented in the option's help text. --- components/fatfs/Kconfig | 32 ++++ .../flash_wl/main/test_fatfs_flash_wl.c | 166 ++++++++++++++++++ .../flash_wl/pytest_fatfs_flash_wl.py | 1 + .../flash_wl/sdkconfig.ci.posix_rename | 1 + components/fatfs/vfs/vfs_fat.c | 135 ++++++++++++++ 5 files changed, 335 insertions(+) create mode 100644 components/fatfs/test_apps/flash_wl/sdkconfig.ci.posix_rename 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; From a9b052a4af51dd082bf52c2cf288a9db5070479d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Rohl=C3=ADnek?= Date: Wed, 16 Sep 2026 10:41:51 +0200 Subject: [PATCH 2/2] feat(fatfs): gate the rename self-nesting guard behind its own option The check that rejects moving a directory into its own subtree was tied to CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION, so a build that only wanted POSIX replacement semantics got the guard as a side effect, and the default build kept the exposure the guard exists for: f_rename() does not detect the case and links the directory into its own tree, after which the directory is reachable only from inside itself and the volume is corrupt. The two behaviours are unrelated, so give the guard its own option, CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING (default n), and let either be enabled without the other. Resolve the destination by start cluster rather than comparing path bytes. FatFs matches names through directory entries, so a destination spelled as an 8.3 alias, or differing only in the case of a non-ASCII character, denotes the same directory as the source and slipped past the previous string prefix check, which folded ASCII case only. Long file names are enabled by default, so the aliases exist in the default configuration. The walk runs only when the source is a directory, leaving a file rename one directory open that fails immediately. Fold the two conditional variants of vfs_fat_rename() into a single implementation. The variant guarded by the replace option had lost the stat cache invalidation the unguarded one performs, so a readdir-cached entry could answer a later stat() for a path that had just been renamed; the invalidation now applies to every configuration. Co-authored-by: Cursor --- components/fatfs/Kconfig | 28 ++- .../flash_wl/main/test_fatfs_flash_wl.c | 28 ++- .../flash_wl/pytest_fatfs_flash_wl.py | 1 + .../flash_wl/sdkconfig.ci.self_nesting | 1 + components/fatfs/vfs/vfs_fat.c | 163 ++++++++++-------- 5 files changed, 140 insertions(+), 81 deletions(-) create mode 100644 components/fatfs/test_apps/flash_wl/sdkconfig.ci.self_nesting diff --git a/components/fatfs/Kconfig b/components/fatfs/Kconfig index e662dd49734..9748be04c5c 100644 --- a/components/fatfs/Kconfig +++ b/components/fatfs/Kconfig @@ -341,11 +341,6 @@ menu "FAT Filesystem support" 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 @@ -353,6 +348,29 @@ menu "FAT Filesystem support" 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 4ed4e860f9a..3ad4b922740 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 @@ -406,7 +406,7 @@ TEST_CASE("(WL) rename obeys the POSIX rules on directories", "[fatfs][wear_leve /* 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 +#ifdef CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING TEST_CASE("(WL) rename refuses to move a directory into itself", "[fatfs][wear_levelling]") { test_setup(); @@ -422,6 +422,30 @@ TEST_CASE("(WL) rename refuses to move a directory into itself", "[fatfs][wear_l 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")); @@ -431,7 +455,7 @@ TEST_CASE("(WL) rename refuses to move a directory into itself", "[fatfs][wear_l test_teardown(); } -#endif // CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION +#endif // CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING TEST_CASE("(WL) can create and remove directories", "[fatfs][wear_levelling]") { 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 be2195737e5..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 @@ -16,6 +16,7 @@ from pytest_embedded_idf.utils import idf_parametrize '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.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 55e95aed405..3279e360c1b 100644 --- a/components/fatfs/vfs/vfs_fat.c +++ b/components/fatfs/vfs/vfs_fat.c @@ -982,44 +982,74 @@ cleanup: } -#ifdef CONFIG_FATFS_VFS_RENAME_REPLACES_DESTINATION +#ifdef CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING /* - * 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. + * 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 bool fat_path_is_within(const char *dir, const char *path) +static DWORD fat_dir_start_cluster(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; - } + FF_DIR dir; + if (f_opendir(&dir, path) != FR_OK) { + return 0; } - - /* `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'; + 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 @@ -1073,34 +1103,40 @@ static int vfs_fat_replace_destination(const char *src, const char *dst) 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; _lock_acquire(&fat_ctx->lock); + vfs_fat_invalidate_stat_cache(fat_ctx, src); + vfs_fat_invalidate_stat_cache(fat_ctx, dst); 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); - } - } + int posix_errno = vfs_fat_rename_locked(src, dst); _lock_release(&fat_ctx->lock); @@ -1112,27 +1148,6 @@ static int vfs_fat_rename(void* ctx, const char *src, const char *dst) 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; - _lock_acquire(&fat_ctx->lock); - 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); - _lock_release(&fat_ctx->lock); - if (res != FR_OK) { - ESP_LOGD(TAG, "%s: fresult=%d", __func__, res); - errno = fresult_to_errno(res); - return -1; - } - 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;