Harden arguments.h and add tests

Modules/app-module/include/app/execute.h

@@ -83,7 +83,7 @@ error_t app_execute_for_result_with_streams(
     struct AppStackConfig stack,
     int argc,
     const char* const argv[],
-    const struct AppStreamBinding* binding,
+    const struct AppStreamBinding* bindings,
     size_t binding_count,
     AppInstanceId parent_instance_id,
     AppInstanceId* out_app_instance_id

Modules/app-module/private/app/private/arguments.h

@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: Apache-2.0
+#pragma once
+
+#include <cstring>
+#include <new>
+
+/**
+ * Deep-copies @a argv (@a argc <= 0 => NULL, matching "no parameters"). Caller passes the result
+ * to app_scheduler_start(), which takes ownership regardless of outcome.
+ * @return NULL if @a argc <= 0 (no parameters), or if allocation failed. For @a argc > 0,
+ * these are the only cases that produce NULL, so a caller can tell them apart by its own
+ * already-known @a argc: NULL back from a positive @a argc always means allocation failed.
+ * All partial allocations are freed before returning NULL, so failure never leaks memory.
+ */
+inline char** app_arguments_copy(int argc, const char* const argv[]) {
+    if (argc <= 0) {
+        return nullptr;
+    }
+
+    auto* copy = new (std::nothrow) char*[argc + 1];
+    if (copy == nullptr) {
+        return nullptr;
+    }
+
+    int copied = 0;
+    for (; copied < argc; copied++) {
+        size_t length = strlen(argv[copied]);
+        copy[copied] = new (std::nothrow) char[length + 1];
+        if (copy[copied] == nullptr) {
+            break;
+        }
+        memcpy(copy[copied], argv[copied], length + 1);
+    }
+
+    if (copied < argc) {
+        for (int i = 0; i < copied; i++) {
+            delete[] copy[i];
+        }
+        delete[] copy;
+        return nullptr;
+    }
+
+    copy[argc] = nullptr;
+    return copy;
+}
+
+/**
+ * Frees a deep-copied argv previously built by app_arguments_copy(): each individually
+ * heap-allocated string, then the array itself. Safe to call with count == 0 / values == nullptr
+ * (no-op).
+ */
+inline void app_arguments_free(int count, char** values) {
+    if (values == nullptr) {
+        return;
+    }
+    for (int i = 0; i < count; i++) {
+        delete[] values[i];
+    }
+    delete[] values;
+}

Modules/app-module/private/app/private/ledger.h

@@ -71,17 +71,3 @@ inline AppLedger& app_ledger() {
     static AppLedger ledger;
     return ledger;
 }
-
-/**
- * Frees a deep-copied argv previously built by app_start()/app_start_for_result():
- * each individually heap-allocated string, then the array itself. Safe to call with count == 0  values == nullptr (no-op).
- */
-inline void app_ledger_free_arguments(int count, char** values) {
-    if (values == nullptr) {
-        return;
-    }
-    for (int i = 0; i < count; i++) {
-        delete[] values[i];
-    }
-    delete[] values;
-}

Modules/app-module/source/internal_loader.cpp

@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: Apache-2.0
 #include <app/loader.h>
-#include <app/manifest.h>
 
 #include <service/instance.h>
 #include <service/manager.h>

Modules/app-module/source/manager.cpp

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 #include <app/manager.h>
 #include <app/metadata.h>
+#include <app/private/arguments.h>
 #include <app/private/fd_table.h>
 #include <app/private/fs.h>
 #include <app/private/ledger.h>
@@ -71,33 +72,14 @@ void app_manager_for_each_manifest(AppManifestVisitorFn visitor, void* context)
     mutex_unlock(&ledger.mutex);
 }
 
-namespace {
-
-// Deep-copies argv (argc <= 0 => NULL, matching "no parameters"). Caller passes the result to
-// app_scheduler_start(), which takes ownership regardless of outcome.
-char** copy_arguments(int argc, const char* const argv[]) {
-    if (argc <= 0) {
-        return nullptr;
-    }
-    auto* copy = new char*[argc + 1];
-    for (int i = 0; i < argc; i++) {
-        size_t length = strlen(argv[i]);
-        copy[i] = new char[length + 1];
-        memcpy(copy[i], argv[i], length + 1);
-    }
-    copy[argc] = nullptr;
-    return copy;
-}
-
-} // namespace
-
 error_t app_manager_start_internal(const AppManifest* manifest, AppLocation location, AppStackConfig stack, AppInstanceId parent_instance_id, int argc, const char* const argv_in[], const AppStreamBinding* bindings, size_t binding_count, AppInstanceId* out_app_instance_id) {
-    // Owns its own deep copy from here on: app_scheduler_start() takes ownership of it regardless
+    char** argv = app_arguments_copy(argc, argv_in);
-    // of outcome, and the spawned task frees it once its run() returns.
+    if (argc > 0 && argv == nullptr) {
-    char** argv = copy_arguments(argc, argv_in);
+        return ERROR_OUT_OF_MEMORY;
+    }
 
     if (binding_count != 0 && bindings == nullptr) {
-        app_ledger_free_arguments(argc, argv);
+        app_arguments_free(argc, argv);
         return ERROR_INVALID_ARGUMENT;
     }
 
@@ -108,9 +90,8 @@ error_t app_manager_start_internal(const AppManifest* manifest, AppLocation loca
     AppInstanceRecord record { .id = target_id, .manifest = manifest, .state = APP_INSTANCE_STATE_STARTING, .task = nullptr };
     record.parent_id = parent_instance_id;
     ledger.instances[target_id] = record;
-    // Constructed on the map-resident copy, not the local `record` about to go out of scope.
+    // Construct on the map-resident copy, not `record`: fds[] point into slots[] by address
-    // AppFdTable::fds[] entries point into AppFdTable::slots[] by address (see fd_table.h), so
+    // (fd_table.h), so constructing on the stack-local record would leave them dangling.
-    // constructing before the copy above would leave them pointing at stack storage.
     app_fd_table_construct(&ledger.instances[target_id].fd_table);
     mutex_unlock(&ledger.mutex);
 
@@ -120,9 +101,8 @@ error_t app_manager_start_internal(const AppManifest* manifest, AppLocation loca
         error_t bind_result = app_stream_subscribe(bindings[i].stream, bindings[i].buffer, bindings[i].buffer_capacity, bindings[i].event_group, target_id, bindings[i].producer_fd);
         if (bind_result != ERROR_NONE) {
             LOG_E(TAG, "[instance %d] Failed to bind stream at fd %d: %s", target_id, bindings[i].producer_fd, error_to_string(bind_result));
-            // Undo bindings[0..i): app_fd_table_teardown() below only closes each stream. It
+            // Undo bindings[0..i): teardown() below only closes the fd, not the event bits
-            // doesn't release the event bits app_stream_subscribe() claimed or destruct
+            // or mutex app_stream_subscribe() claimed; only app_stream_unsubscribe() does.
-            // stream->internal.mutex; only app_stream_unsubscribe() does that.
             for (size_t j = 0; j < i; j++) {
                 app_stream_unsubscribe(bindings[j].stream);
             }
@@ -130,15 +110,13 @@ error_t app_manager_start_internal(const AppManifest* manifest, AppLocation loca
             app_fd_table_teardown(&ledger.instances[target_id].fd_table);
             ledger.instances.erase(target_id);
             mutex_unlock(&ledger.mutex);
-            app_ledger_free_arguments(argc, argv);
+            app_arguments_free(argc, argv);
             return bind_result;
         }
     }
 
     error_t error = app_scheduler_start(target_id, location, stack, argc, argv);
     if (error != ERROR_NONE) {
-        // Every binding succeeded before app_scheduler_start() failed. Unsubscribe all of them,
-        // same reasoning as the bind-failure path above.
         for (size_t j = 0; j < binding_count; j++) {
             app_stream_unsubscribe(bindings[j].stream);
         }
@@ -172,8 +150,7 @@ error_t app_manager_get_topmost_instance_id(AppInstanceId* out_app_instance_id)
     mutex_lock(&ledger.mutex);
     AppInstanceId topmost_id = 0;
     for (auto& [instance_id, record] : ledger.instances) {
-        // Instance ids are handed out in increasing order (AppLedger::next_instance_id), so
+        // Ids increase monotonically, so the highest Active id is the most recent.
-        // the highest Active id is also the most recently started one.
         if (record.state == APP_INSTANCE_STATE_ACTIVE && instance_id > topmost_id) {
             topmost_id = instance_id;
         }
@@ -224,10 +201,8 @@ error_t app_manager_get_topmost_app_id(char* buffer, size_t buffer_size) {
 namespace {
 
 // Owns the AppManifest (and its id/name/path strings) that app_manager_add() only keeps a
-// non-owning pointer to (see app_manager_add()'s contract), for manifests registered by
+// non-owning pointer to. Separate from app_install.cpp's registry: scanning only
-// app_manager_install_path_scan() specifically - separate from app_install.cpp's own registry,
+// adds/removes registrations, never touches disk or running instances.
-// since scanning only ever adds/removes manifest registrations and never touches files on disk
-// or running instances (unlike app_install()/app_uninstall()).
 struct ScannedAppManifest {
     std::string id;
     std::string name;
@@ -274,15 +249,15 @@ void app_manager_install_path_scan(void) {
         app_fs_list_direct_subdirectories(root, found_app_dirs);
     }
 
-    // Snapshot of what's already registered, taken once so the rest of this scan can run without holding registry.mutex
+    // Snapshot once so the rest of the scan doesn't hold registry.mutex.
     mutex_lock(&registry.mutex);
-    std::unordered_map<std::string, std::string> known_paths; // id -> path
+    std::unordered_map<std::string, std::string> known_paths;
     for (const auto& [id, record] : registry.scanned) {
         known_paths.emplace(id, record->path);
     }
     mutex_unlock(&registry.mutex);
 
-    // Stat each manifest and parse it entirely without registry.mutex held (due to filesystem IO being slow)
+    // Parses without registry.mutex held; filesystem IO is slow.
     std::vector<std::unique_ptr<ScannedAppManifest>> new_records;
     for (const auto& app_dir : found_app_dirs) {
         auto manifest_path = app_dir + "/manifest.properties";
@@ -297,7 +272,7 @@ void app_manager_install_path_scan(void) {
         }
 
         if (known_paths.contains(metadata.app_id)) {
-            continue; // already registered by an earlier scan
+            continue;
         }
 
         auto record = std::make_unique<ScannedAppManifest>();
@@ -315,7 +290,6 @@ void app_manager_install_path_scan(void) {
         new_records.push_back(std::move(record));
     }
 
-    // Anything a previous scan registered whose directory has since disappeared  gets unregistered below.
     std::vector<std::string> missing_ids;
     for (const auto& [id, path] : known_paths) {
         if (!app_fs_is_directory(path)) {
@@ -323,11 +297,9 @@ void app_manager_install_path_scan(void) {
         }
     }
 
-    // app_manager_add()/app_manager_remove() take app-module's own ledger mutex internally -
+    // app_manager_add()/remove() take the ledger mutex internally, so calling them under
-    // calling them while holding registry.mutex would establish a registry.mutex -> ledger-
+    // registry.mutex would fix a lock order an opposite-order caller could deadlock against.
-    // mutex lock order that any future opposite-order path would deadlock against, so these
+    // registry.mutex is retaken afterward only to publish the in-memory results.
-    // also run with registry.mutex released. registry.mutex is taken only afterward, briefly,
-    // to publish the results (plain in-memory map updates, no I/O or other locks involved).
     for (const auto& id : missing_ids) {
         app_manager_remove(id.c_str());
     }
@@ -364,10 +336,8 @@ error_t app_manager_install_path_uninstall(const char* app_id) {
     auto path = iterator->second->path;
     mutex_unlock(&registry.mutex);
 
-    // Stop every running instance that retains this manifest pointer, mirroring
+    // Mirrors stop_all_instances_of() in app_install.cpp. Collect under ledger.mutex, stop
-    // stop_all_instances_of() in app_install.cpp.  Collect under ledger.mutex,
+    // outside it: app_manager_stop() bound-joins the thread, which itself takes 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<uint32_t> instance_ids;
     auto& ledger = app_ledger();
     mutex_lock(&ledger.mutex);
@@ -382,14 +352,12 @@ error_t app_manager_install_path_uninstall(const char* app_id) {
         app_manager_stop(id);
     }
 
-    // app_manager_remove takes ledger.mutex internally - call outside both
+    // app_manager_remove() takes ledger.mutex; call outside registry.mutex too, matching
-    // registry.mutex and ledger.mutex to match the lock ordering in
+    // the lock order in app_manager_install_path_scan().
-    // app_manager_install_path_scan().
     app_manager_remove(app_id);
 
-    // Every instance has stopped and the manifest is unregistered — safe to
+    // Delete before erasing the scan record, so a failed deletion still leaves the
-    // delete the on-disk directory.  Delete before erasing the scan record so
+    // entry discoverable for a retry.
-    // that a failed deletion leaves the entry discoverable for a retry.
     if (!app_fs_delete_recursively(path)) {
         return ERROR_RESOURCE;
     }

Modules/app-module/source/scheduler.cpp

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 #include <app/instance.h>
 #include <app/loader.h>
+#include <app/private/arguments.h>
 #include <app/private/event.h>
 #include <app/private/fd_table.h>
 #include <app/private/ledger.h>
@@ -223,7 +224,7 @@ void app_task_main(void* context) {
     // response to APP_EVENT_CLOSE.
     set_state(ctx->app_instance_id, APP_INSTANCE_STATE_STOPPED);
 
-    app_ledger_free_arguments(ctx->argc, ctx->argv);
+    app_arguments_free(ctx->argc, ctx->argv);
 
     AppInstanceId app_instance_id = ctx->app_instance_id;
     AppCompletionSignal* completion = ctx->completion;
@@ -274,7 +275,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
     const AppLoaderApi* loader = find_loader_api(location.type);
     if (loader == nullptr) {
         LOG_E(TAG, "[instance %lu] No app loader is registered (service '%s' not found)", app_instance_id, loader_service_id_for(location.type));
-        app_ledger_free_arguments(argc, argv);
+        app_arguments_free(argc, argv);
         return ERROR_NOT_FOUND;
     }
 
@@ -282,7 +283,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
     error_t load_result = loader->load(location, &runtime);
     if (load_result != ERROR_NONE) {
         LOG_E(TAG, "[instance %lu] Failed to load app: %s", app_instance_id, error_to_string(load_result));
-        app_ledger_free_arguments(argc, argv);
+        app_arguments_free(argc, argv);
         return load_result;
     }
 
@@ -290,7 +291,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
     if (completion == nullptr) {
         LOG_E(TAG, "[instance %lu] Failed to allocate app", app_instance_id);
         loader->unload(runtime);
-        app_ledger_free_arguments(argc, argv);
+        app_arguments_free(argc, argv);
         return ERROR_OUT_OF_MEMORY;
     }
     completion->semaphore = xSemaphoreCreateBinary();
@@ -298,7 +299,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
         LOG_E(TAG, "[instance %lu] Failed to allocate app", app_instance_id);
         delete completion;
         loader->unload(runtime);
-        app_ledger_free_arguments(argc, argv);
+        app_arguments_free(argc, argv);
         return ERROR_OUT_OF_MEMORY;
     }
 
@@ -309,7 +310,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
         vSemaphoreDelete(completion->semaphore);
         delete completion;
         loader->unload(runtime);
-        app_ledger_free_arguments(argc, argv);
+        app_arguments_free(argc, argv);
         return ERROR_INVALID_ARGUMENT;
     }
 
@@ -335,7 +336,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
         vSemaphoreDelete(completion->semaphore);
         delete completion;
         loader->unload(runtime);
-        app_ledger_free_arguments(argc, argv);
+        app_arguments_free(argc, argv);
         return ERROR_OUT_OF_MEMORY;
     }
 
@@ -346,7 +347,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
         vSemaphoreDelete(completion->semaphore);
         delete completion;
         loader->unload(runtime);
-        app_ledger_free_arguments(argc, argv);
+        app_arguments_free(argc, argv);
         return ERROR_OUT_OF_MEMORY;
     }
 #else
@@ -374,7 +375,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
         vSemaphoreDelete(completion->semaphore);
         delete completion;
         loader->unload(runtime);
-        app_ledger_free_arguments(argc, argv);
+        app_arguments_free(argc, argv);
         return ERROR_OUT_OF_MEMORY;
     }
 
@@ -401,7 +402,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
         vSemaphoreDelete(completion->semaphore);
         delete completion;
         loader->unload(runtime);
-        app_ledger_free_arguments(argc, argv);
+        app_arguments_free(argc, argv);
         return ERROR_OUT_OF_MEMORY;
     }
     vTaskSuspend(task_handle);

Modules/app-module/tests/CMakeLists.txt

@@ -12,7 +12,7 @@ if (NOT APPLE)
     target_sources(AppModuleTests PRIVATE ${CMAKE_CURRENT_LIST_DIR}/../../../Tactility/Source/AppStdioWrap.cpp)
 endif ()
 
-target_include_directories(AppModuleTests PRIVATE ${DOCTESTINC})
+target_include_directories(AppModuleTests PRIVATE ${DOCTESTINC} ${CMAKE_CURRENT_LIST_DIR}/../private)
 
 add_test(NAME AppModuleTests COMMAND AppModuleTests)
 

Modules/app-module/tests/source/arguments_test.cpp

@@ -0,0 +1,40 @@
+// SPDX-License-Identifier: Apache-2.0
+#include "doctest.h"
+
+#include <app/private/arguments.h>
+
+#include <cstring>
+
+TEST_CASE("app_arguments_copy returns NULL for argc <= 0") {
+    const char* const argv[] = { "a" };
+    CHECK_EQ(app_arguments_copy(0, argv), nullptr);
+    CHECK_EQ(app_arguments_copy(-1, argv), nullptr);
+    CHECK_EQ(app_arguments_copy(0, nullptr), nullptr);
+}
+
+TEST_CASE("app_arguments_copy deep-copies argv") {
+    const char* const argv[] = { "one", "two", "three" };
+    char** copy = app_arguments_copy(3, argv);
+    REQUIRE_NE(copy, nullptr);
+
+    CHECK_NE(static_cast<const void*>(copy[0]), static_cast<const void*>(argv[0]));
+    CHECK_EQ(std::strcmp(copy[0], "one"), 0);
+    CHECK_EQ(std::strcmp(copy[1], "two"), 0);
+    CHECK_EQ(std::strcmp(copy[2], "three"), 0);
+    CHECK_EQ(copy[3], nullptr);
+
+    app_arguments_free(3, copy);
+}
+
+TEST_CASE("app_arguments_copy handles an empty string argument") {
+    const char* const argv[] = { "" };
+    char** copy = app_arguments_copy(1, argv);
+    REQUIRE_NE(copy, nullptr);
+    CHECK_EQ(std::strcmp(copy[0], ""), 0);
+    CHECK_EQ(copy[1], nullptr);
+    app_arguments_free(1, copy);
+}
+
+TEST_CASE("app_arguments_free is a no-op for count 0 / null values") {
+    app_arguments_free(0, nullptr);
+}