From 6d4109e070829683d756300eecd3e1fd20e085f5 Mon Sep 17 00:00:00 2001 From: "amelia@ivis.ai" Date: Fri, 18 Sep 2026 13:12:22 +0900 Subject: [PATCH 1/3] fix(launch_manager): avoid leaking ControlProvider on Create failure Create() heap-allocated a ControlProvider via raw `new` and returned MakeUnexpected(...) on any of the four setup-step failure paths without ever deleting it, leaking the object. Wrap the allocation in a unique_ptr guard that frees it on any early return and only release()s the raw pointer once every setup step has succeeded, matching the existing Result signature. --- .../src/daemon/src/control/control_provider.cpp | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/score/launch_manager/src/daemon/src/control/control_provider.cpp b/score/launch_manager/src/daemon/src/control/control_provider.cpp index 51160b11b..038908c4d 100644 --- a/score/launch_manager/src/daemon/src/control/control_provider.cpp +++ b/score/launch_manager/src/daemon/src/control/control_provider.cpp @@ -15,6 +15,8 @@ #include "score/mw/launch_manager/common/log.hpp" #include "score/mw/launch_manager/osal/ipc_comms.hpp" +#include + namespace score::mw::lifecycle::internal { @@ -36,33 +38,35 @@ Result ControlProvider::Create(IRunTargetControl* graph) noexc } LmControlSkeleton skeleton = std::move(skeleton_result).value(); - auto* control_provider = new ControlProvider{std::move(skeleton), graph}; + // Freed automatically via `guard` on any of the failure paths below; released to the + // caller only once every setup step below has succeeded. + std::unique_ptr guard{new ControlProvider{std::move(skeleton), graph}}; - const Result setup_activate_run_target_result = control_provider->setupActivateRunTarget(); + const Result setup_activate_run_target_result = guard->setupActivateRunTarget(); if (!setup_activate_run_target_result.has_value()) { return MakeUnexpected(static_cast(*setup_activate_run_target_result.error())); } - const Result setup_get_active_run_target_result = control_provider->setupGetActiveRunTarget(); + const Result setup_get_active_run_target_result = guard->setupGetActiveRunTarget(); if (!setup_get_active_run_target_result.has_value()) { return MakeUnexpected(static_cast(*setup_get_active_run_target_result.error())); } - const Result setup_activation_result_result = control_provider->setupActivationResult(); + const Result setup_activation_result_result = guard->setupActivationResult(); if (!setup_activation_result_result.has_value()) { return MakeUnexpected(static_cast(*setup_activation_result_result.error())); } - const Result offer_service_result = control_provider->offerService(); + const Result offer_service_result = guard->offerService(); if (!offer_service_result.has_value()) { return MakeUnexpected(static_cast(*offer_service_result.error())); } - return control_provider; + return guard.release(); } ControlProvider::ControlProvider(LmControlSkeleton skeleton, IRunTargetControl* graph) noexcept From 10c9fbae8d9eee5cfd529c060a8d72ce2c97ad6c Mon Sep 17 00:00:00 2001 From: "amelia@ivis.ai" Date: Fri, 18 Sep 2026 13:21:03 +0900 Subject: [PATCH 2/3] test(launch_manager): add unit tests for ControlProvider::Create There was previously no unit test coverage for ControlProvider::Create at all. Adds two: the happy path, and a second Create() for an already-offered instance failing cleanly rather than crashing. Neither exercises the unique_ptr guard added in the previous commit (both fail earlier, at LmControlSkeleton::Create's flock check) -- no config-based way to make RegisterHandler or OfferService fail after skeleton creation succeeds was found. Noted in the test comments. --- .../src/daemon/src/control/BUILD | 19 ++++ .../src/control/control_provider_UT.cpp | 98 +++++++++++++++++++ .../control_provider_test_mw_com_config.json | 70 +++++++++++++ 3 files changed, 187 insertions(+) create mode 100644 score/launch_manager/src/daemon/src/control/control_provider_UT.cpp create mode 100644 score/launch_manager/src/daemon/src/control/control_provider_test_mw_com_config.json diff --git a/score/launch_manager/src/daemon/src/control/BUILD b/score/launch_manager/src/daemon/src/control/BUILD index da97a4efb..18e684eaa 100644 --- a/score/launch_manager/src/daemon/src/control/BUILD +++ b/score/launch_manager/src/daemon/src/control/BUILD @@ -11,6 +11,7 @@ # SPDX-License-Identifier: Apache-2.0 # ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") +load("//tests/utils/bazel:unit_test.bzl", "lm_cc_test") cc_library( name = "control_provider", @@ -26,3 +27,21 @@ cc_library( "//score/launch_manager/src/lm_control", ], ) + +lm_cc_test( + name = "control_provider_UT", + srcs = ["control_provider_UT.cpp"], + args = [ + "--service_instance_manifest", + "$(rootpath control_provider_test_mw_com_config.json)", + ], + data = ["control_provider_test_mw_com_config.json"], + deps = [ + ":control_provider", + "//score/launch_manager:error", + "//score/launch_manager/src/daemon/src/process_group_manager:irun_target_control", + "@googletest//:gtest", + "@score_baselibs//score/string_manipulation/arguments", + "@score_communication//score/mw/com", + ], +) diff --git a/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp b/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp new file mode 100644 index 000000000..4ab1e9857 --- /dev/null +++ b/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp @@ -0,0 +1,98 @@ +/******************************************************************************** + * Copyright (c) 2026 Contributors to the Eclipse Foundation + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Apache License Version 2.0 which is available at + * https://www.apache.org/licenses/LICENSE-2.0 + * + * SPDX-License-Identifier: Apache-2.0 + ********************************************************************************/ + +#include "score/mw/launch_manager/control/control_provider.hpp" +#include "score/mw/launch_manager/process_group_manager/irun_target_control.hpp" + +#include "score/mw/com/runtime.h" +#include "score/string_manipulation/arguments/arguments.h" + +#include + +namespace score::mw::lifecycle::internal +{ +namespace +{ + +// `Create()` never calls back into `graph` before returning, so a no-op stub is sufficient for +// exercising `Create()` itself: none of these bodies run in the tests below. +class FakeRunTargetControl : public IRunTargetControl +{ + public: + [[nodiscard]] score::Result getActiveRunTarget() const noexcept override + { + return MakeUnexpected(ExecErrc::kCommunicationError); + } + + [[nodiscard]] score::Result setRequestedRunTarget(IdentifierHash /*run_target*/) noexcept override + { + return {}; + } + + void registerActiveRunTargetCallback(ActivationCallbackT /*callback*/) noexcept override {} +}; + +class ControlProviderUT : public ::testing::Test +{ + protected: + FakeRunTargetControl graph_; +}; + +TEST_F(ControlProviderUT, CreateSucceeds) +{ + RecordProperty("Description", "ControlProvider::Create returns a valid instance when the instance is free."); + + const Result result = ControlProvider::Create(&graph_); + + ASSERT_TRUE(result.has_value()); + EXPECT_NE(result.value(), nullptr); + + // `ControlProvider` is intentionally never destroyed in production (see the class-level + // comment: the mw::com callbacks it registers reference it for the daemon's whole lifetime). + // Free it explicitly here so this test doesn't report that deliberate, process-lifetime + // "leak" as a `--config=asan_ubsan_lsan` finding of its own, and so the instance below is + // free to reuse the same instance specifier. + delete result.value(); +} + +TEST_F(ControlProviderUT, SecondCreateForSameInstanceFailsCleanly) +{ + RecordProperty("Description", + "A second ControlProvider::Create for an already-offered instance fails " + "cleanly instead of crashing or hanging."); + + // NOTE: this fails at LmControlSkeleton::Create() itself (an flock on a marker file), before + // Create() ever reaches the `new ControlProvider{...}` this fix wraps in a unique_ptr guard. + // It does not exercise that guard's cleanup path -- no config-based way to make one of the + // later setup steps (setupActivateRunTarget/setupGetActiveRunTarget/offerService) fail was + // found; RegisterHandler succeeds even when a method is missing from the deployment config. + // That path's correctness rests on unique_ptr's RAII guarantee rather than on this test. + const Result first_result = ControlProvider::Create(&graph_); + ASSERT_TRUE(first_result.has_value()); + + const Result second_result = ControlProvider::Create(&graph_); + EXPECT_FALSE(second_result.has_value()); + + delete first_result.value(); +} + +} // namespace +} // namespace score::mw::lifecycle::internal + +int main(int argc, char** argv) +{ + ::testing::InitGoogleTest(&argc, argv); + score::mw::com::runtime::InitializeRuntime( + score::string_manipulation::GetArguments(argc, const_cast(argv))); + return RUN_ALL_TESTS(); +} diff --git a/score/launch_manager/src/daemon/src/control/control_provider_test_mw_com_config.json b/score/launch_manager/src/daemon/src/control/control_provider_test_mw_com_config.json new file mode 100644 index 000000000..861d110c6 --- /dev/null +++ b/score/launch_manager/src/daemon/src/control/control_provider_test_mw_com_config.json @@ -0,0 +1,70 @@ +{ + "serviceTypes": [ + { + "serviceTypeName": "/score/mw/lifecycle/LmControlService", + "version": { + "major": 1, + "minor": 0 + }, + "bindings": [ + { + "binding": "SHM", + "serviceId": 7101, + "events": [ + { + "eventName": "ActivationResult", + "eventId": 1 + } + ], + "methods": [ + { + "methodName": "ActivateRunTarget", + "methodId": 2 + }, + { + "methodName": "GetActiveRunTarget", + "methodId": 3 + } + ] + } + ] + } + ], + "serviceInstances": [ + { + "instanceSpecifier": "LaunchManager/StateManager/Instance", + "serviceTypeName": "/score/mw/lifecycle/LmControlService", + "version": { + "major": 1, + "minor": 0 + }, + "instances": [ + { + "instanceId": 1, + "asil-level": "QM", + "binding": "SHM", + "events": [ + { + "eventName": "ActivationResult", + "numberOfSampleSlots": 8, + "maxSubscribers": 1 + } + ], + "methods": [ + { + "methodName": "ActivateRunTarget", + "queueSize": 1 + }, + { + "methodName": "GetActiveRunTarget", + "queueSize": 1 + } + ] + } + ] + } + ], + "global": { + "asil-level": "QM" + } +} From 50666558a2683411503098b39198021db296407e Mon Sep 17 00:00:00 2001 From: "amelia@ivis.ai" Date: Fri, 18 Sep 2026 13:31:28 +0900 Subject: [PATCH 3/3] test(launch_manager): document why guard path is untestable here Traced RegisterHandler down to the lola binding's concrete implementation (unconditional success) to prove three of the four setup-step failure branches are unreachable with this binding, rather than just asserting no trigger was found. --- .../src/daemon/src/control/control_provider_UT.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp b/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp index 4ab1e9857..1561bd389 100644 --- a/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp +++ b/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp @@ -73,10 +73,16 @@ TEST_F(ControlProviderUT, SecondCreateForSameInstanceFailsCleanly) // NOTE: this fails at LmControlSkeleton::Create() itself (an flock on a marker file), before // Create() ever reaches the `new ControlProvider{...}` this fix wraps in a unique_ptr guard. - // It does not exercise that guard's cleanup path -- no config-based way to make one of the - // later setup steps (setupActivateRunTarget/setupGetActiveRunTarget/offerService) fail was - // found; RegisterHandler succeeds even when a method is missing from the deployment config. - // That path's correctness rests on unique_ptr's RAII guarantee rather than on this test. + // It does not exercise that guard's cleanup path. Traced why no config-based trigger exists + // for the other three setup steps: SkeletonMethod::RegisterHandler (communication's + // score/mw/com/impl/bindings/lola/skeleton_method.cpp) unconditionally does + // `type_erased_callback_ = std::move(...); return {};` -- it cannot fail on this binding, so + // setupActivateRunTarget/setupGetActiveRunTarget can't either, and setupActivationResult's + // own body is an unconditional `return {};`. That leaves offerService(), whose only reachable + // failure mode here is genuine OS resource exhaustion (e.g. an artificially lowered FD + // rlimit) during SHM event-slot allocation -- deliberately not done here, since it'd depend on + // the binding's internal FD-consumption pattern and risk CI flakiness for little benefit. This + // path's correctness rests on unique_ptr's RAII guarantee rather than on an executable test. const Result first_result = ControlProvider::Create(&graph_); ASSERT_TRUE(first_result.has_value());