[FL-3891] Folder rename fails (#3896)

* fix, refactor: storage is_subdir API * docs: fix incorrect comment * test: new storage apis * test: use temporary path * style: fix formatting * UnitTest: storage path macros naming * UnitTest: storage path macros naming part 2 Co-authored-by: あく <[email protected]>
flipperdevices · Sep 15, 2024 · 19a3736 · 19a3736
1 parent b670d5b
commit 19a3736
Show file tree

Hide file tree

Showing 7 changed files with 120 additions and 40 deletions.
diff --git a/applications/debug/unit_tests/tests/storage/storage_test.c b/applications/debug/unit_tests/tests/storage/storage_test.c
@@ -6,9 +6,10 @@
 // This is a hack to access internal storage functions and definitions
 #include <storage/storage_i.h>
 
-#define UNIT_TESTS_PATH(path) EXT_PATH("unit_tests/" path)
+#define UNIT_TESTS_RESOURCES_PATH(path) EXT_PATH("unit_tests/" path)
+#define UNIT_TESTS_PATH(path)           EXT_PATH(".tmp/unit_tests/" path)
 
-#define STORAGE_LOCKED_FILE EXT_PATH("locked_file.test")
+#define STORAGE_LOCKED_FILE UNIT_TESTS_PATH("locked_file.test")
 #define STORAGE_LOCKED_DIR  STORAGE_INT_PATH_PREFIX
 
 #define STORAGE_TEST_DIR UNIT_TESTS_PATH("test_dir")
@@ -369,44 +370,92 @@ MU_TEST(storage_file_rename) {
     Storage* storage = furi_record_open(RECORD_STORAGE);
     File* file = storage_file_alloc(storage);
 
-    mu_check(write_file_13DA(storage, EXT_PATH("file.old")));
-    mu_check(check_file_13DA(storage, EXT_PATH("file.old")));
+    mu_check(write_file_13DA(storage, UNIT_TESTS_PATH("file.old")));
+    mu_check(check_file_13DA(storage, UNIT_TESTS_PATH("file.old")));
     mu_assert_int_eq(
-        FSE_OK, storage_common_rename(storage, EXT_PATH("file.old"), EXT_PATH("file.new")));
-    mu_assert_int_eq(FSE_NOT_EXIST, storage_common_stat(storage, EXT_PATH("file.old"), NULL));
-    mu_assert_int_eq(FSE_OK, storage_common_stat(storage, EXT_PATH("file.new"), NULL));
-    mu_check(check_file_13DA(storage, EXT_PATH("file.new")));
-    mu_assert_int_eq(FSE_OK, storage_common_remove(storage, EXT_PATH("file.new")));
+        FSE_OK,
+        storage_common_rename(storage, UNIT_TESTS_PATH("file.old"), UNIT_TESTS_PATH("file.new")));
+    mu_assert_int_eq(
+        FSE_NOT_EXIST, storage_common_stat(storage, UNIT_TESTS_PATH("file.old"), NULL));
+    mu_assert_int_eq(FSE_OK, storage_common_stat(storage, UNIT_TESTS_PATH("file.new"), NULL));
+    mu_check(check_file_13DA(storage, UNIT_TESTS_PATH("file.new")));
+    mu_assert_int_eq(FSE_OK, storage_common_remove(storage, UNIT_TESTS_PATH("file.new")));
 
     storage_file_free(file);
     furi_record_close(RECORD_STORAGE);
 }
 
+static const char* dir_rename_tests[][2] = {
+    {UNIT_TESTS_PATH("dir.old"), UNIT_TESTS_PATH("dir.new")},
+    {UNIT_TESTS_PATH("test_dir"), UNIT_TESTS_PATH("test_dir-new")},
+    {UNIT_TESTS_PATH("test"), UNIT_TESTS_PATH("test-test")},
+};
+
 MU_TEST(storage_dir_rename) {
     Storage* storage = furi_record_open(RECORD_STORAGE);
 
-    storage_dir_create(storage, EXT_PATH("dir.old"));
+    for(size_t i = 0; i < COUNT_OF(dir_rename_tests); i++) {
+        const char* old_path = dir_rename_tests[i][0];
+        const char* new_path = dir_rename_tests[i][1];
+
+        storage_dir_create(storage, old_path);
+        mu_check(storage_dir_rename_check(storage, old_path));
+
+        mu_assert_int_eq(FSE_OK, storage_common_rename(storage, old_path, new_path));
+        mu_assert_int_eq(FSE_NOT_EXIST, storage_common_stat(storage, old_path, NULL));
+        mu_check(storage_dir_rename_check(storage, new_path));
+
+        storage_dir_remove(storage, new_path);
+        mu_assert_int_eq(FSE_NOT_EXIST, storage_common_stat(storage, new_path, NULL));
+    }
+
+    furi_record_close(RECORD_STORAGE);
+}
 
-    mu_check(storage_dir_rename_check(storage, EXT_PATH("dir.old")));
+MU_TEST(storage_equiv_and_subdir) {
+    Storage* storage = furi_record_open(RECORD_STORAGE);
 
     mu_assert_int_eq(
-        FSE_OK, storage_common_rename(storage, EXT_PATH("dir.old"), EXT_PATH("dir.new")));
-    mu_assert_int_eq(FSE_NOT_EXIST, storage_common_stat(storage, EXT_PATH("dir.old"), NULL));
-    mu_check(storage_dir_rename_check(storage, EXT_PATH("dir.new")));
+        true,
+        storage_common_equivalent_path(storage, UNIT_TESTS_PATH("blah"), UNIT_TESTS_PATH("blah")));
+    mu_assert_int_eq(
+        true,
+        storage_common_equivalent_path(
+            storage, UNIT_TESTS_PATH("blah/"), UNIT_TESTS_PATH("blah/")));
+    mu_assert_int_eq(
+        false,
+        storage_common_equivalent_path(
+            storage, UNIT_TESTS_PATH("blah"), UNIT_TESTS_PATH("blah-blah")));
+    mu_assert_int_eq(
+        false,
+        storage_common_equivalent_path(
+            storage, UNIT_TESTS_PATH("blah/"), UNIT_TESTS_PATH("blah-blah/")));
 
-    storage_dir_remove(storage, EXT_PATH("dir.new"));
-    mu_assert_int_eq(FSE_NOT_EXIST, storage_common_stat(storage, EXT_PATH("dir.new"), NULL));
+    mu_assert_int_eq(
+        true, storage_common_is_subdir(storage, UNIT_TESTS_PATH("blah"), UNIT_TESTS_PATH("blah")));
+    mu_assert_int_eq(
+        true,
+        storage_common_is_subdir(storage, UNIT_TESTS_PATH("blah"), UNIT_TESTS_PATH("blah/blah")));
+    mu_assert_int_eq(
+        false,
+        storage_common_is_subdir(storage, UNIT_TESTS_PATH("blah/blah"), UNIT_TESTS_PATH("blah")));
+    mu_assert_int_eq(
+        false,
+        storage_common_is_subdir(storage, UNIT_TESTS_PATH("blah"), UNIT_TESTS_PATH("blah-blah")));
 
     furi_record_close(RECORD_STORAGE);
 }
 
 MU_TEST_SUITE(storage_rename) {
     MU_RUN_TEST(storage_file_rename);
     MU_RUN_TEST(storage_dir_rename);
+    MU_RUN_TEST(storage_equiv_and_subdir);
 
     Storage* storage = furi_record_open(RECORD_STORAGE);
-    storage_dir_remove(storage, EXT_PATH("dir.old"));
-    storage_dir_remove(storage, EXT_PATH("dir.new"));
+    for(size_t i = 0; i < COUNT_OF(dir_rename_tests); i++) {
+        storage_dir_remove(storage, dir_rename_tests[i][0]);
+        storage_dir_remove(storage, dir_rename_tests[i][1]);
+    }
     furi_record_close(RECORD_STORAGE);
 }
 
@@ -653,7 +702,7 @@ MU_TEST(test_md5_calc) {
     Storage* storage = furi_record_open(RECORD_STORAGE);
     File* file = storage_file_alloc(storage);
 
-    const char* path = UNIT_TESTS_PATH("storage/md5.txt");
+    const char* path = UNIT_TESTS_RESOURCES_PATH("storage/md5.txt");
     const char* md5_cstr = "2a456fa43e75088fdde41c93159d62a2";
     const uint8_t md5[MD5_HASH_SIZE] = {
         0x2a,

diff --git a/applications/services/storage/storage.h b/applications/services/storage/storage.h
@@ -401,21 +401,26 @@ bool storage_common_exists(Storage* storage, const char* path);
  * - /int/Test and /int/test -> false (Case-sensitive storage),
  * - /ext/Test and /ext/test -> true (Case-insensitive storage).
  *
- * If the truncate parameter is set to true, the second path will be
- * truncated to be no longer than the first one. It is useful to determine
- * whether path2 is a subdirectory of path1.
- *
  * @param storage pointer to a storage API instance.
  * @param path1 pointer to a zero-terminated string containing the first path.
  * @param path2 pointer to a zero-terminated string containing the second path.
- * @param truncate whether to truncate path2 to be no longer than path1.
  * @return true if paths are equivalent, false otherwise.
  */
-bool storage_common_equivalent_path(
-    Storage* storage,
-    const char* path1,
-    const char* path2,
-    bool truncate);
+bool storage_common_equivalent_path(Storage* storage, const char* path1, const char* path2);
+
+/**
+ * @brief Check whether a path is a subpath of another path.
+ * 
+ * This function respects storage-defined equivalence rules
+ * (see `storage_common_equivalent_path`).
+ * 
+ * @param storage pointer to a storage API instance.
+ * @param parent pointer to a zero-terminated string containing the parent path.
+ * @param child pointer to a zero-terminated string containing the child path.
+ * @return true if `child` is a subpath of `parent`, or if `child` is equivalent
+ *         to `parent`; false otherwise.
+ */
+bool storage_common_is_subdir(Storage* storage, const char* parent, const char* child);
 
 /******************* Error Functions *******************/
 

diff --git a/applications/services/storage/storage_external_api.c b/applications/services/storage/storage_external_api.c
@@ -493,13 +493,13 @@ FS_Error storage_common_rename(Storage* storage, const char* old_path, const cha
             }
 
             // Cannot rename a directory to itself or to a nested directory
-            if(storage_common_equivalent_path(storage, old_path, new_path, true)) {
+            if(storage_common_is_subdir(storage, old_path, new_path)) {
                 error = FSE_INVALID_NAME;
                 break;
             }
 
             // Renaming a regular file to itself does nothing and always succeeds
-        } else if(storage_common_equivalent_path(storage, old_path, new_path, false)) {
+        } else if(storage_common_equivalent_path(storage, old_path, new_path)) {
             error = FSE_OK;
             break;
         }
@@ -816,11 +816,11 @@ bool storage_common_exists(Storage* storage, const char* path) {
     return storage_common_stat(storage, path, &file_info) == FSE_OK;
 }
 
-bool storage_common_equivalent_path(
+static bool storage_internal_equivalent_path(
     Storage* storage,
     const char* path1,
     const char* path2,
-    bool truncate) {
+    bool check_subdir) {
     furi_check(storage);
 
     S_API_PROLOGUE;
@@ -829,7 +829,7 @@ bool storage_common_equivalent_path(
         .cequivpath = {
             .path1 = path1,
             .path2 = path2,
-            .truncate = truncate,
+            .check_subdir = check_subdir,
             .thread_id = furi_thread_get_current_id(),
         }};
 
@@ -839,6 +839,14 @@ bool storage_common_equivalent_path(
     return S_RETURN_BOOL;
 }
 
+bool storage_common_equivalent_path(Storage* storage, const char* path1, const char* path2) {
+    return storage_internal_equivalent_path(storage, path1, path2, false);
+}
+
+bool storage_common_is_subdir(Storage* storage, const char* parent, const char* child) {
+    return storage_internal_equivalent_path(storage, parent, child, true);
+}
+
 /****************** ERROR ******************/
 
 const char* storage_error_get_desc(FS_Error error_id) {

diff --git a/applications/services/storage/storage_message.h b/applications/services/storage/storage_message.h
@@ -72,7 +72,7 @@ typedef struct {
 typedef struct {
     const char* path1;
     const char* path2;
-    bool truncate;
+    bool check_subdir;
     FuriThreadId thread_id;
 } SADataCEquivPath;
 

diff --git a/applications/services/storage/storage_processing.c b/applications/services/storage/storage_processing.c
@@ -694,7 +694,23 @@ void storage_process_message_internal(Storage* app, StorageMessage* message) {
         storage_path_trim_trailing_slashes(path2);
         storage_process_alias(app, path1, message->data->cequivpath.thread_id, false);
         storage_process_alias(app, path2, message->data->cequivpath.thread_id, false);
-        if(message->data->cequivpath.truncate) {
+        if(message->data->cequivpath.check_subdir) {
+            // by appending slashes at the end and then truncating the second path, we can
+            // effectively check for shared path components:
+            // example 1:
+            //   path1: "/ext/blah"      -> "/ext/blah/"      -> "/ext/blah/"
+            //   path2: "/ext/blah-blah" -> "/ect/blah-blah/" -> "/ext/blah-"
+            //   results unequal, conclusion: path2 is not a subpath of path1
+            // example 2:
+            //   path1: "/ext/blah"      -> "/ext/blah/"      -> "/ext/blah/"
+            //   path2: "/ext/blah/blah" -> "/ect/blah/blah/" -> "/ext/blah/"
+            //   results equal, conclusion: path2 is a subpath of path1
+            // example 3:
+            //   path1: "/ext/blah/blah" -> "/ect/blah/blah/" -> "/ext/blah/blah/"
+            //   path2: "/ext/blah"      -> "/ext/blah/"      -> "/ext/blah/"
+            //   results unequal, conclusion: path2 is not a subpath of path1
+            furi_string_push_back(path1, '/');
+            furi_string_push_back(path2, '/');
             furi_string_left(path2, furi_string_size(path1));
         }
         message->return_data->bool_value =

diff --git a/targets/f18/api_symbols.csv b/targets/f18/api_symbols.csv
@@ -1,5 +1,5 @@
 entry,status,name,type,params
-Version,+,73.0,,
+Version,+,74.0,,
 Header,+,applications/services/bt/bt_service/bt.h,,
 Header,+,applications/services/bt/bt_service/bt_keys_storage.h,,
 Header,+,applications/services/cli/cli.h,,
@@ -2491,9 +2491,10 @@ Function,+,st25r3916_write_pttsn_mem,void,"FuriHalSpiBusHandle*, uint8_t*, size_
 Function,+,st25r3916_write_reg,void,"FuriHalSpiBusHandle*, uint8_t, uint8_t"
 Function,+,st25r3916_write_test_reg,void,"FuriHalSpiBusHandle*, uint8_t, uint8_t"
 Function,+,storage_common_copy,FS_Error,"Storage*, const char*, const char*"
-Function,+,storage_common_equivalent_path,_Bool,"Storage*, const char*, const char*, _Bool"
+Function,+,storage_common_equivalent_path,_Bool,"Storage*, const char*, const char*"
 Function,+,storage_common_exists,_Bool,"Storage*, const char*"
 Function,+,storage_common_fs_info,FS_Error,"Storage*, const char*, uint64_t*, uint64_t*"
+Function,+,storage_common_is_subdir,_Bool,"Storage*, const char*, const char*"
 Function,+,storage_common_merge,FS_Error,"Storage*, const char*, const char*"
 Function,+,storage_common_migrate,FS_Error,"Storage*, const char*, const char*"
 Function,+,storage_common_mkdir,FS_Error,"Storage*, const char*"

diff --git a/targets/f7/api_symbols.csv b/targets/f7/api_symbols.csv
@@ -1,5 +1,5 @@
 entry,status,name,type,params
-Version,+,73.0,,
+Version,+,74.0,,
 Header,+,applications/drivers/subghz/cc1101_ext/cc1101_ext_interconnect.h,,
 Header,+,applications/services/bt/bt_service/bt.h,,
 Header,+,applications/services/bt/bt_service/bt_keys_storage.h,,
@@ -3168,9 +3168,10 @@ Function,+,st25tb_save,_Bool,"const St25tbData*, FlipperFormat*"
 Function,+,st25tb_set_uid,_Bool,"St25tbData*, const uint8_t*, size_t"
 Function,+,st25tb_verify,_Bool,"St25tbData*, const FuriString*"
 Function,+,storage_common_copy,FS_Error,"Storage*, const char*, const char*"
-Function,+,storage_common_equivalent_path,_Bool,"Storage*, const char*, const char*, _Bool"
+Function,+,storage_common_equivalent_path,_Bool,"Storage*, const char*, const char*"
 Function,+,storage_common_exists,_Bool,"Storage*, const char*"
 Function,+,storage_common_fs_info,FS_Error,"Storage*, const char*, uint64_t*, uint64_t*"
+Function,+,storage_common_is_subdir,_Bool,"Storage*, const char*, const char*"
 Function,+,storage_common_merge,FS_Error,"Storage*, const char*, const char*"
 Function,+,storage_common_migrate,FS_Error,"Storage*, const char*, const char*"
 Function,+,storage_common_mkdir,FS_Error,"Storage*, const char*"