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] 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;