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 <cursoragent@cursor.com>

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

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]")
 {

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'])

components/fatfs/test_apps/flash_wl/sdkconfig.ci.self_nesting

@@ -0,0 +1 @@
+CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING=y

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;