diff --git a/Modules/app-module/private/app/private/app_fs.h b/Modules/app-module/private/app/private/app_fs.h index 9438976b3..b76445baa 100644 --- a/Modules/app-module/private/app/private/app_fs.h +++ b/Modules/app-module/private/app/private/app_fs.h @@ -46,39 +46,76 @@ inline bool app_fs_delete_recursively(const std::string& path) { return true; } - if (app_fs_is_directory(path)) { - FileMutex file_mutex; - file_mutex_get(&file_mutex, path.c_str()); - file_mutex_lock(&file_mutex); + // Use lstat() so symbolic links are not followed: a symlink that points at + // an external directory must be removed as a leaf entry (unlink), not + // recursed into. app_fs_is_directory() uses stat() and would follow the + // link, potentially deleting files outside the target tree. + // ESP-IDF newlib has no lstat(); ESP32 filesystems (FAT/SPIFFS) don't + // support symlinks, so stat() is equivalent there. + struct stat st {}; + FileMutex file_mutex; + file_mutex_get(&file_mutex, path.c_str()); + file_mutex_lock(&file_mutex); +#ifdef ESP_PLATFORM + int rc = stat(path.c_str(), &st); +#else + int rc = lstat(path.c_str(), &st); +#endif + file_mutex_unlock(&file_mutex); + if (rc != 0) { + return false; + } + +#ifndef ESP_PLATFORM + if (S_ISLNK(st.st_mode)) { + // Symlink — remove as a leaf regardless of its target. + file_mutex_lock(&file_mutex); + bool result = unlink(path.c_str()) == 0; + file_mutex_unlock(&file_mutex); + return result; + } +#endif + + if (S_ISDIR(st.st_mode)) { + // Collect child names while locked, then release before recursing — + // child paths can resolve to the same mount mutex (see + // app_fs_list_direct_subdirectories comment), so holding the parent + // lock across the recursive call would self-deadlock. + std::vector children; + + file_mutex_lock(&file_mutex); DIR* dir = opendir(path.c_str()); if (dir == nullptr) { file_mutex_unlock(&file_mutex); return false; } - bool success = true; struct dirent* entry; - while (success && (entry = readdir(dir)) != nullptr) { + while ((entry = readdir(dir)) != nullptr) { if (std::strcmp(entry->d_name, ".") == 0 || std::strcmp(entry->d_name, "..") == 0) { continue; } - success = app_fs_delete_recursively(path + "/" + entry->d_name); + children.push_back(path + "/" + entry->d_name); } closedir(dir); + file_mutex_unlock(&file_mutex); - if (!success) { - file_mutex_unlock(&file_mutex); - return false; + bool success = true; + for (const auto& child : children) { + success = app_fs_delete_recursively(child); + if (!success) { + return false; + } } + file_mutex_lock(&file_mutex); bool result = rmdir(path.c_str()) == 0; file_mutex_unlock(&file_mutex); return result; } - FileMutex file_mutex; - file_mutex_get(&file_mutex, path.c_str()); + // Regular file or other — unlink. file_mutex_lock(&file_mutex); bool result = unlink(path.c_str()) == 0; file_mutex_unlock(&file_mutex); diff --git a/Modules/app-module/source/manager.cpp b/Modules/app-module/source/manager.cpp index 3786f7d21..96b976943 100644 --- a/Modules/app-module/source/manager.cpp +++ b/Modules/app-module/source/manager.cpp @@ -1,13 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 #include - #include - #include #include #include #include +#include #include #include @@ -350,19 +349,44 @@ error_t app_manager_install_path_uninstall(const char* app_id) { return ERROR_NOT_FOUND; } + const AppManifest* manifest = &iterator->second->manifest; auto path = iterator->second->path; mutex_unlock(®istry.mutex); - // app_manager_remove takes ledger.mutex internally - call outside registry.mutex - // to match the lock ordering in app_manager_install_path_scan(). + // Stop every running instance that retains this manifest pointer, mirroring + // stop_all_instances_of() in app_install.cpp. Collect under ledger.mutex, + // then call app_manager_stop() outside it (that call bound-joins the + // instance's thread, which itself takes ledger.mutex in its thread_main). + std::vector instance_ids; + auto& ledger = app_ledger(); + mutex_lock(&ledger.mutex); + for (const auto& [id, record] : ledger.instances) { + if (record.manifest == manifest) { + instance_ids.push_back(id); + } + } + mutex_unlock(&ledger.mutex); + + for (uint32_t id : instance_ids) { + app_manager_stop(id); + } + + // app_manager_remove takes ledger.mutex internally - call outside both + // registry.mutex and ledger.mutex to match the lock ordering in + // app_manager_install_path_scan(). app_manager_remove(app_id); + // Every instance has stopped and the manifest is unregistered — safe to + // delete the on-disk directory. Delete before erasing the scan record so + // that a failed deletion leaves the entry discoverable for a retry. + if (!app_fs_delete_recursively(path)) { + return ERROR_RESOURCE; + } + mutex_lock(®istry.mutex); registry.scanned.erase(app_id); mutex_unlock(®istry.mutex); - app_fs_delete_recursively(path); - return ERROR_NONE; } diff --git a/Modules/service-module/tests/source/service_paths_test.cpp b/Modules/service-module/tests/source/service_paths_test.cpp index ba19e006d..4730b7a8e 100644 --- a/Modules/service-module/tests/source/service_paths_test.cpp +++ b/Modules/service-module/tests/source/service_paths_test.cpp @@ -9,7 +9,7 @@ TEST_CASE("paths_get_data_path returns a non-empty path") { char buffer[192]; - CHECK_EQ(paths_get_data_path(buffer, sizeof(buffer)), ERROR_NONE); + REQUIRE_EQ(paths_get_data_path(buffer, sizeof(buffer)), ERROR_NONE); CHECK_GT(std::strlen(buffer), 0); }