Merge branch 'feat/fatfs_posix_rename' into 'master'

feat(fatfs): add option to make rename() replace the destination

Closes IDF-16139

See merge request espressif/esp-idf!52382

components/fatfs/Kconfig

@@ -321,6 +321,56 @@ 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.
+
+            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_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

@@ -10,6 +10,7 @@
 #include <time.h>
 #include <sys/time.h>
 #include <sys/unistd.h>
+#include <errno.h>
 #include <sys/stat.h>
 #include "unity.h"
 #include "esp_partition.h"
@@ -265,6 +266,195 @@ 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_REJECTS_SELF_NESTING
+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));
+
+    /* 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"));
+    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_REJECTS_SELF_NESTING
+
 TEST_CASE("(WL) can create and remove directories", "[fatfs][wear_levelling]")
 {
     test_setup();

components/fatfs/test_apps/flash_wl/pytest_fatfs_flash_wl.py

@@ -15,6 +15,8 @@ from pytest_embedded_idf.utils import idf_parametrize
         'fastseek',
         'auto_fsync',
         'dyn_buffers',
+        'posix_rename',
+        'self_nesting',
     ],
 )
 @idf_parametrize('target', ['esp32', 'esp32c3'], indirect=['target'])

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

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

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

@@ -981,6 +981,153 @@ cleanup:
     return ret;
 }
 
+
+#ifdef CONFIG_FATFS_VFS_RENAME_REJECTS_SELF_NESTING
+/*
+ * 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 DWORD fat_dir_start_cluster(const char *path)
+{
+    FF_DIR dir;
+    if (f_opendir(&dir, path) != FR_OK) {
+        return 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
+ * 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);
+}
+#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;
@@ -988,11 +1135,14 @@ static int vfs_fat_rename(void* ctx, const char *src, const char *dst)
     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);
+
+    int posix_errno = vfs_fat_rename_locked(src, dst);
+
     _lock_release(&fat_ctx->lock);
-    if (res != FR_OK) {
-        ESP_LOGD(TAG, "%s: fresult=%d", __func__, res);
-        errno = fresult_to_errno(res);
+
+    if (posix_errno != 0) {
+        ESP_LOGD(TAG, "%s: errno=%d", __func__, posix_errno);
+        errno = posix_errno;
         return -1;
     }
     return 0;