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;