From 3f11b76aaedae17a71b29e6a8bf893f52216869f Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Fri, 4 Sep 2026 08:31:47 +0200 Subject: [PATCH] Harden arguments.h and add tests --- .../private/app/private/arguments.h | 33 ++++++++++++--- Modules/app-module/source/manager.cpp | 3 ++ Modules/app-module/tests/CMakeLists.txt | 2 +- .../tests/source/arguments_test.cpp | 40 +++++++++++++++++++ 4 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 Modules/app-module/tests/source/arguments_test.cpp diff --git a/Modules/app-module/private/app/private/arguments.h b/Modules/app-module/private/app/private/arguments.h index 4200f6a2e..34126cb1d 100644 --- a/Modules/app-module/private/app/private/arguments.h +++ b/Modules/app-module/private/app/private/arguments.h @@ -2,21 +2,44 @@ #pragma once #include +#include /** * 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; } diff --git a/Modules/app-module/source/manager.cpp b/Modules/app-module/source/manager.cpp index d905cf5f7..c76148996 100644 --- a/Modules/app-module/source/manager.cpp +++ b/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); diff --git a/Modules/app-module/tests/CMakeLists.txt b/Modules/app-module/tests/CMakeLists.txt index de1c477e0..d10cf759a 100644 --- a/Modules/app-module/tests/CMakeLists.txt +++ b/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) diff --git a/Modules/app-module/tests/source/arguments_test.cpp b/Modules/app-module/tests/source/arguments_test.cpp new file mode 100644 index 000000000..2f417fe85 --- /dev/null +++ b/Modules/app-module/tests/source/arguments_test.cpp @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: Apache-2.0 +#include "doctest.h" + +#include + +#include + +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(copy[0]), static_cast(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); +}