Harden arguments.h and add tests

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

@@ -2,21 +2,44 @@
 #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 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);
+
+    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;
 }

Modules/app-module/source/manager.cpp

@@ -74,6 +74,9 @@ void app_manager_for_each_manifest(AppManifestVisitorFn visitor, void* context)
 
 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) {
     char** argv = app_arguments_copy(argc, argv_in);
+    if (argc > 0 && argv == nullptr) {
+        return ERROR_OUT_OF_MEMORY;
+    }
 
     if (binding_count != 0 && bindings == nullptr) {
         app_arguments_free(argc, argv);

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);
+}