From a2d7f3819d994359cfe072299d03a2bebdf4012a Mon Sep 17 00:00:00 2001 From: "amelia@ivis.ai" Date: Thu, 17 Sep 2026 10:19:06 +0900 Subject: [PATCH 1/3] fix(launch_manager): Pimpl ControlProvider to make it movable Confirms and addresses eclipse-score/lifecycle#489 (discussion_r4024461903): ControlProvider registers three [this]-capturing callbacks (activate_run_target/get_active_run_target handlers, registerActiveRunTargetCallback) with no unregister path, so moving the object would leave them pointing at a dangling address -- a real use-after-free, not just a suspected one. Moves skeleton_, graph_, and the three RegisterHandler/ registerActiveRunTargetCallback calls into a private ControlProvider::Impl, with the callbacks capturing Impl* instead of ControlProvider's own this. ControlProvider now holds a single unique_ptr and is safely movable -- moving it only moves the pointer, and Impl's address (what the callbacks actually point at) never changes. Create() returns Result by value instead of Result, updating run.cpp's one call site accordingly. As a side effect, this also fixes a leak on Create()'s failure paths: the previous raw `new` was never deleted if a later setup*() call failed; the local unique_ptr now cleans up automatically via RAII. Also fixes a lifetime issue this same change would otherwise introduce: run.cpp originally scoped its Result to the `if (initialize())` block, relying on the raw pointer never actually being freed to outlive everything after it. Switching to value ownership without changing that scope would destroy ControlProvider (and its Impl) right when that block ends -- before ProcessGroupManager::deinitialize() runs. deinitialize()'s own comment says it drains in-flight worker completions before resetting graph_, and a drain that finishes a transition calls Graph::finalizeTransitionSuccess(), which invokes the very callback Impl registered. So control_provider_result is now declared in an outer scope and kept alive across the deinitialize() call, matching what the original leak provided by accident. Adds control_provider_UT, static-asserting ControlProvider is nothrow-movable and non-copyable -- locking in the contract this change establishes so a future regression (e.g. reinstating `= delete` on the move ops) fails to compile instead of silently reintroducing the dangling-callback bug. Verified: bazel build of control_provider + the full launch_manager binary succeeds; bazel test of the full launch_manager unit and integration suite passes. Also manually verified end-to-end against a real Launch Manager daemon spawning a client built against ILmControl: 2/2 real ActivateRunTarget round trips completed over shared-memory IPC. Signed-off-by: amelia@ivis.ai --- .../src/daemon/src/control/BUILD | 10 +++ .../daemon/src/control/control_provider.cpp | 80 +++++++++++++++---- .../daemon/src/control/control_provider.hpp | 47 ++++------- .../src/control/control_provider_UT.cpp | 44 ++++++++++ score/launch_manager/src/daemon/src/run.cpp | 22 +++-- 5 files changed, 149 insertions(+), 54 deletions(-) create mode 100644 score/launch_manager/src/daemon/src/control/control_provider_UT.cpp diff --git a/score/launch_manager/src/daemon/src/control/BUILD b/score/launch_manager/src/daemon/src/control/BUILD index da97a4efb..450e3562a 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,12 @@ cc_library( "//score/launch_manager/src/lm_control", ], ) + +lm_cc_test( + name = "control_provider_UT", + srcs = ["control_provider_UT.cpp"], + deps = [ + ":control_provider", + "@googletest//:gtest_main", + ], +) 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..a76e78384 100644 --- a/score/launch_manager/src/daemon/src/control/control_provider.cpp +++ b/score/launch_manager/src/daemon/src/control/control_provider.cpp @@ -18,7 +18,47 @@ namespace score::mw::lifecycle::internal { -Result ControlProvider::Create(IRunTargetControl* graph) noexcept +// Holds everything that self-references via a captured `this`: the three callbacks registered +// below all capture `Impl*`, and that address has to stay fixed for as long as they are +// registered (there is no unregister path). `ControlProvider` itself only owns a +// `unique_ptr`, so it stays freely movable while `Impl`'s address never moves. +class ControlProvider::Impl +{ + public: + Impl(LmControlSkeleton skeleton, IRunTargetControl* graph) noexcept : skeleton_(std::move(skeleton)), graph_(graph) + { + } + + /// @brief Register the handler for activate_run_target. + Result setupActivateRunTarget() noexcept; + + /// @brief Handle an activate_run_target request. + void handleActivateRunTarget(ActivateRunTargetResponse& response, const ActivateRunTargetRequest& request) noexcept; + + /// @brief Register the handler for get_active_run_target. + Result setupGetActiveRunTarget() noexcept; + + /// @brief Handle a get_active_run_target request. + void handleGetActiveRunTarget(GetActiveRunTargetResponse& response) noexcept; + + /// @brief Register the handler for activation_result. + Result setupActivationResult() noexcept; + + /// @brief Handle an activation_result event. + void handleActivationResult(IdentifierHash state, RunTargetActivationSource source) noexcept; + + /// @brief Make the service available to clients. + Result offerService() noexcept; + + private: + /// @brief The external `mw::com` interface. + LmControlSkeleton skeleton_; + + /// @brief The underlying graph implementation. + IRunTargetControl* graph_; +}; + +Result ControlProvider::Create(IRunTargetControl* graph) noexcept { const Result instance_specifier_result = com::InstanceSpecifier::Create(std::string{"LaunchManager/StateManager/Instance"}); @@ -36,41 +76,49 @@ Result ControlProvider::Create(IRunTargetControl* graph) noexc } LmControlSkeleton skeleton = std::move(skeleton_result).value(); - auto* control_provider = new ControlProvider{std::move(skeleton), graph}; + // `unique_ptr` rather than the previous raw `new`: on any of the failure paths below, this + // is freed automatically instead of leaking (the previous version returned + // `MakeUnexpected(...)` without ever deleting the raw pointer it had just allocated). + auto impl = std::make_unique(std::move(skeleton), graph); - const Result setup_activate_run_target_result = control_provider->setupActivateRunTarget(); + const Result setup_activate_run_target_result = impl->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 = impl->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 = impl->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 = impl->offerService(); if (!offer_service_result.has_value()) { return MakeUnexpected(static_cast(*offer_service_result.error())); } - return control_provider; + return ControlProvider{std::move(impl)}; } -ControlProvider::ControlProvider(LmControlSkeleton skeleton, IRunTargetControl* graph) noexcept - : skeleton_(std::move(skeleton)), graph_(graph) +ControlProvider::ControlProvider(std::unique_ptr impl) noexcept : impl_(std::move(impl)) { } -Result ControlProvider::setupActivateRunTarget() noexcept +// Defined here rather than defaulted in the header: `Impl` is only a complete type from this +// point in the file onward, and a deleter/mover for `unique_ptr` needs that completeness. +ControlProvider::ControlProvider(ControlProvider&&) noexcept = default; +ControlProvider& ControlProvider::operator=(ControlProvider&&) noexcept = default; +ControlProvider::~ControlProvider() = default; + +Result ControlProvider::Impl::setupActivateRunTarget() noexcept { const auto result = skeleton_.activate_run_target.RegisterHandler( [this](ActivateRunTargetResponse& response, const ActivateRunTargetRequest& request) { @@ -86,7 +134,7 @@ Result ControlProvider::setupActivateRunTarget() noexcept return {}; } -void ControlProvider::handleActivateRunTarget( +void ControlProvider::Impl::handleActivateRunTarget( ActivateRunTargetResponse& response, const ActivateRunTargetRequest& request) noexcept { @@ -127,7 +175,7 @@ void ControlProvider::handleActivateRunTarget( response = ActivateRunTargetResponse{status : RequestStatus::kAccepted}; } -Result ControlProvider::setupGetActiveRunTarget() noexcept +Result ControlProvider::Impl::setupGetActiveRunTarget() noexcept { const auto result = skeleton_.get_active_run_target.RegisterHandler([this](GetActiveRunTargetResponse& response) { this->handleGetActiveRunTarget(response); @@ -142,7 +190,7 @@ Result ControlProvider::setupGetActiveRunTarget() noexcept return {}; } -void ControlProvider::handleGetActiveRunTarget(GetActiveRunTargetResponse& response) noexcept +void ControlProvider::Impl::handleGetActiveRunTarget(GetActiveRunTargetResponse& response) noexcept { const score::Result result = graph_->getActiveRunTarget(); if (!result.has_value()) @@ -161,7 +209,7 @@ void ControlProvider::handleGetActiveRunTarget(GetActiveRunTargetResponse& respo response = GetActiveRunTargetResponse{status : QueryStatus::kAvailable, run_target : RunTargetName(name)}; } -Result ControlProvider::setupActivationResult() noexcept +Result ControlProvider::Impl::setupActivationResult() noexcept { graph_->registerActiveRunTargetCallback([this](IdentifierHash state, RunTargetActivationSource source) { this->handleActivationResult(state, source); @@ -170,7 +218,7 @@ Result ControlProvider::setupActivationResult() noexcept return {}; } -void ControlProvider::handleActivationResult(IdentifierHash state, RunTargetActivationSource source) noexcept +void ControlProvider::Impl::handleActivationResult(IdentifierHash state, RunTargetActivationSource source) noexcept { auto allocate_result = skeleton_.activation_result.Allocate(); if (!allocate_result.has_value()) @@ -201,7 +249,7 @@ void ControlProvider::handleActivationResult(IdentifierHash state, RunTargetActi LM_LOG_DEBUG() << "Sent the activation result to the state manager"; } -Result ControlProvider::offerService() noexcept +Result ControlProvider::Impl::offerService() noexcept { const auto result = skeleton_.OfferService(); if (!result.has_value()) diff --git a/score/launch_manager/src/daemon/src/control/control_provider.hpp b/score/launch_manager/src/daemon/src/control/control_provider.hpp index bb976e575..df98e7981 100644 --- a/score/launch_manager/src/daemon/src/control/control_provider.hpp +++ b/score/launch_manager/src/daemon/src/control/control_provider.hpp @@ -17,55 +17,36 @@ #include "score/mw/launch_manager/process_group_manager/irun_target_control.hpp" #include "score/mw/lifecycle/details/lm_control_service.h" +#include + namespace score::mw::lifecycle::internal { /// @brief Provides the mw::com service for state managers to connect to. -/// @details This cannot be moved, because the mw::com callbacks reference -// the ControlProvider at its original location. class ControlProvider { public: /// @brief Fallible constructor for ControllableGraph. - static Result Create(IRunTargetControl* graph) noexcept; + static Result Create(IRunTargetControl* graph) noexcept; - ~ControlProvider() = default; + // Movable: the mw::com/graph callbacks registered during `Create` capture a pointer to + // `Impl`, not to `ControlProvider` itself, so moving a `ControlProvider` only moves the + // `unique_ptr` — `Impl`'s address (the thing the callbacks actually point at) never + // changes. Declared out-of-line (not `= default` here) because `Impl` is still an + // incomplete type at this point; defined in the .cpp once `Impl` is complete. + ControlProvider(ControlProvider&&) noexcept; + ControlProvider& operator=(ControlProvider&&) noexcept; + ~ControlProvider(); - // Cannot be moved because callbacks capture the ControlProvider by reference. ControlProvider(const ControlProvider&) = delete; - ControlProvider(ControlProvider&&) = delete; ControlProvider& operator=(const ControlProvider&) = delete; - ControlProvider operator=(ControlProvider&&) = delete; private: - explicit ControlProvider(LmControlSkeleton skeleton, IRunTargetControl* graph) noexcept; - - /// @brief Register the handler for activate_run_target. - Result setupActivateRunTarget() noexcept; - - /// @brief Handle an activate_run_target request. - void handleActivateRunTarget(ActivateRunTargetResponse& response, const ActivateRunTargetRequest& request) noexcept; - - /// @brief Register the handler for get_active_run_target. - Result setupGetActiveRunTarget() noexcept; - - /// @brief Handle a get_active_run_target request. - void handleGetActiveRunTarget(GetActiveRunTargetResponse& response) noexcept; - - /// @brief Register the handler for activation_result. - Result setupActivationResult() noexcept; - - /// @brief Handle an activation_result event. - void handleActivationResult(IdentifierHash state, RunTargetActivationSource source) noexcept; - - /// @brief Make the service available to clients. - Result offerService() noexcept; + class Impl; - /// @brief The external `mw::com` interface. - LmControlSkeleton skeleton_; + explicit ControlProvider(std::unique_ptr impl) noexcept; - /// @brief The underlying graph implementation. - IRunTargetControl* graph_; + std::unique_ptr impl_; }; } // namespace score::mw::lifecycle::internal 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..b5d7b7680 --- /dev/null +++ b/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp @@ -0,0 +1,44 @@ +/******************************************************************************** + * 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 + +#include + +#include "score/mw/launch_manager/control/control_provider.hpp" + +namespace score::mw::lifecycle::internal +{ +namespace +{ + +// Locks in the exact contract this class was reworked to provide (see the class comment in +// control_provider.hpp): movable via a stable `Impl*` the registered callbacks capture, but +// never copyable, since copying would either alias or duplicate that `Impl`. A regression here +// (e.g. someone reinstating `= delete` on the move ops, or defaulting a copy op) would silently +// reintroduce the dangling-callback bug this Pimpl was introduced to fix, without necessarily +// failing any other test, since nothing else in this suite constructs a `ControlProvider`. +static_assert(std::is_nothrow_move_constructible_v); +static_assert(std::is_nothrow_move_assignable_v); +static_assert(!std::is_copy_constructible_v); +static_assert(!std::is_copy_assignable_v); + +TEST(ControlProviderUT, StaticAssertionsCompiled) +{ + // The interesting assertions above run at compile time; this keeps the test target from + // being empty and gives CI something to report. + SUCCEED(); +} + +} // namespace +} // namespace score::mw::lifecycle::internal diff --git a/score/launch_manager/src/daemon/src/run.cpp b/score/launch_manager/src/daemon/src/run.cpp index eb390ebcc..3ee76c533 100644 --- a/score/launch_manager/src/daemon/src/run.cpp +++ b/score/launch_manager/src/daemon/src/run.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include "score/mw/launch_manager/alive_monitor/details/daemon/AliveMonitorImpl.hpp" #include "score/mw/launch_manager/common/log.hpp" @@ -175,16 +176,24 @@ int run(int argc, const char* argv[]) std::move(watchdog), config_result.value().takeWatchdog()); + // Declared here, outside the `if` block below, and deliberately not destroyed until + // after `deinitialize()` returns: `deinitialize()`'s own comment notes that a worker may + // still be (de)activating a node when it runs, and drains those in flight before + // resetting `graph_`. A drain that completes a transition calls + // Graph::finalizeTransitionSuccess(), which invokes the very callback `ControlProvider` + // registered via `registerActiveRunTargetCallback`. If `ControlProvider` were destroyed + // before that drain finishes -- as it would be if scoped only to the `if` block below -- + // that callback would fire through a dangling pointer. + std::optional> control_provider_result; + if (process_group_manager->initialize()) { - // Remains active in the background until the ControlProvider is destroyed. - const score::Result control_provider_result = - ControlProvider::Create(process_group_manager.get()); + control_provider_result = ControlProvider::Create(process_group_manager.get()); - if (!control_provider_result.has_value()) + if (!control_provider_result->has_value()) { LM_LOG_FATAL() << "Failed to set up LmControl service provider:" - << control_provider_result.error().Message(); + << control_provider_result->error().Message(); exit_code = EXIT_FAILURE; } else if (runLCMDaemon(*process_group_manager)) @@ -198,6 +207,9 @@ int run(int argc, const char* argv[]) process_group_manager->deinitialize(); process_group_manager.reset(); } + + // Safe to let `control_provider_result` go out of scope now: `deinitialize()` above has + // already joined every worker and reset `graph_`, so no further callback can arrive. } catch (...) { From 06678be3dd6c36718c77cb171a2b2bb389e2d5d8 Mon Sep 17 00:00:00 2001 From: "amelia@ivis.ai" Date: Mon, 21 Sep 2026 12:29:42 +0900 Subject: [PATCH 2/3] test(launch_manager): verify ControlProvider callbacks survive a move Fulfills the test-coverage gap flagged on PR #654 (eclipse-score/lifecycle): the static_assert-only test added previously checks that ControlProvider's type is move-constructible, but that doesn't prove a move is actually safe. Adds a real end-to-end regression test: creates a ControlProvider via Create() over a real mw::com skeleton (using a dedicated test service config, mirroring the one from #653), moves it into an outer-scoped variable, destroys the moved-from original, then invokes the callback registered via registerActiveRunTargetCallback. That callback closure captures Impl*, not ControlProvider's own `this`, so it must still dispatch correctly through the moved-to instance's Impl -- if a future regression stored Impl by value instead of behind a unique_ptr, this would be a genuine use-after-free/scope, catchable under --config=asan_ubsan_lsan. Verified: bazel test --config=x86_64-linux and bazel test --config=asan_ubsan_lsan both pass, 2/2 tests. Signed-off-by: amelia@ivis.ai --- .../src/daemon/src/control/BUILD | 11 ++- .../src/control/control_provider_UT.cpp | 90 +++++++++++++++++-- .../control_provider_test_mw_com_config.json | 70 +++++++++++++++ 3 files changed, 165 insertions(+), 6 deletions(-) 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 450e3562a..18e684eaa 100644 --- a/score/launch_manager/src/daemon/src/control/BUILD +++ b/score/launch_manager/src/daemon/src/control/BUILD @@ -31,8 +31,17 @@ cc_library( 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", - "@googletest//:gtest_main", + "//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 index b5d7b7680..65028ad98 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 @@ -11,11 +11,17 @@ * 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 +#include #include - -#include "score/mw/launch_manager/control/control_provider.hpp" +#include namespace score::mw::lifecycle::internal { @@ -26,19 +32,93 @@ namespace // control_provider.hpp): movable via a stable `Impl*` the registered callbacks capture, but // never copyable, since copying would either alias or duplicate that `Impl`. A regression here // (e.g. someone reinstating `= delete` on the move ops, or defaulting a copy op) would silently -// reintroduce the dangling-callback bug this Pimpl was introduced to fix, without necessarily -// failing any other test, since nothing else in this suite constructs a `ControlProvider`. +// reintroduce the dangling-callback bug this Pimpl was introduced to fix. static_assert(std::is_nothrow_move_constructible_v); static_assert(std::is_nothrow_move_assignable_v); static_assert(!std::is_copy_constructible_v); static_assert(!std::is_copy_assignable_v); -TEST(ControlProviderUT, StaticAssertionsCompiled) +// `Create()` never calls back into `graph` before returning, so a no-op stub is sufficient for +// `getActiveRunTarget`/`setRequestedRunTarget`. `registerActiveRunTargetCallback` is captured so +// the test below can invoke it directly, as if a real transition had just completed. +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 + { + callback_ = std::move(callback); + } + + void TriggerActivation(IdentifierHash state, RunTargetActivationSource source) + { + ASSERT_TRUE(static_cast(callback_)) << "registerActiveRunTargetCallback was never called"; + callback_(state, source); + } + + private: + ActivationCallbackT callback_; +}; + +class ControlProviderUT : public ::testing::Test +{ + protected: + FakeRunTargetControl graph_; +}; + +TEST_F(ControlProviderUT, StaticAssertionsCompiled) { // The interesting assertions above run at compile time; this keeps the test target from // being empty and gives CI something to report. SUCCEED(); } +TEST_F(ControlProviderUT, CallbacksDispatchThroughMovedInstance) +{ + RecordProperty( + "Description", + "After a ControlProvider is moved, its registered callbacks still dispatch through the " + "moved-to instance's Impl, not through a stale, already-destroyed one."); + + const IdentifierHash run_target_id{"control_provider_ut_run_target"}; + + std::optional moved_to; + { + Result create_result = ControlProvider::Create(&graph_); + ASSERT_TRUE(create_result.has_value()); + + ControlProvider original = std::move(create_result).value(); + + // Move `original` into the outer-scoped `moved_to`, then let `original` (and this + // block) be destroyed. If the registered callback captured `original`'s own address + // instead of the stable, heap-allocated `Impl*` -- the exact bug the Pimpl in this PR + // fixes -- that address is gone once this block ends, and triggering the callback below + // would be a genuine use-after-scope, not just a theoretical one. + moved_to.emplace(std::move(original)); + } + ASSERT_TRUE(moved_to.has_value()); + + // Doesn't crash and doesn't trip ASan/UBSan iff the callback dispatches through the + // still-alive `Impl` that `moved_to` now owns. + graph_.TriggerActivation(run_target_id, RunTargetActivationSource::kStateManagerRequest); +} + } // 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 ab61cb68f0655a16b2228c6600eca86e2ef57517 Mon Sep 17 00:00:00 2001 From: "amelia@ivis.ai" Date: Wed, 23 Sep 2026 14:05:04 +0900 Subject: [PATCH 3/3] refactor(launch_manager): return ControlProvider via unique_ptr Follows the review suggestion on eclipse-score/lifecycle#654: instead of a Pimpl that makes ControlProvider movable, Create() now returns Result>. Nothing ever needs to move a ControlProvider; the registered callbacks only need its address to stay fixed, which heap ownership through a unique_ptr already guarantees. This drops ControlProvider::Impl and the out-of-line move/destructor definitions, and keeps the class non-movable and non-copyable as it was on main. The Create() failure-path leak stays fixed, since the local unique_ptr frees the object on every early return. run.cpp keeps the provider alive in an outer scope until after ProcessGroupManager::deinitialize(), so callbacks fired while in-flight transitions drain still reach a live object. control_provider_UT now static-asserts that the class is neither movable nor copyable, checks that callbacks still dispatch after ownership of the unique_ptr is transferred, and checks that destroying the provider releases the service so a second Create() succeeds. Signed-off-by: amelia@ivis.ai --- .../daemon/src/control/control_provider.cpp | 82 ++++--------------- .../daemon/src/control/control_provider.hpp | 46 ++++++++--- .../src/control/control_provider_UT.cpp | 55 +++++++------ score/launch_manager/src/daemon/src/run.cpp | 20 +++-- 4 files changed, 96 insertions(+), 107 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 ff44fff81..b84cfd7fc 100644 --- a/score/launch_manager/src/daemon/src/control/control_provider.cpp +++ b/score/launch_manager/src/daemon/src/control/control_provider.cpp @@ -18,47 +18,7 @@ namespace score::mw::lifecycle::internal { -// Holds everything that self-references via a captured `this`: the three callbacks registered -// below all capture `Impl*`, and that address has to stay fixed for as long as they are -// registered (there is no unregister path). `ControlProvider` itself only owns a -// `unique_ptr`, so it stays freely movable while `Impl`'s address never moves. -class ControlProvider::Impl -{ - public: - Impl(LmControlSkeleton skeleton, IRunTargetControl* graph) noexcept : skeleton_(std::move(skeleton)), graph_(graph) - { - } - - /// @brief Register the handler for activate_run_target. - Result setupActivateRunTarget() noexcept; - - /// @brief Handle an activate_run_target request. - void handleActivateRunTarget(ActivateRunTargetResponse& response, const ActivateRunTargetRequest& request) noexcept; - - /// @brief Register the handler for get_active_run_target. - Result setupGetActiveRunTarget() noexcept; - - /// @brief Handle a get_active_run_target request. - void handleGetActiveRunTarget(GetActiveRunTargetResponse& response) noexcept; - - /// @brief Register the handler for activation_result. - Result setupActivationResult() noexcept; - - /// @brief Handle an activation_result event. - void handleActivationResult(IdentifierHash state, RunTargetActivationSource source) noexcept; - - /// @brief Make the service available to clients. - Result offerService() noexcept; - - private: - /// @brief The external `mw::com` interface. - LmControlSkeleton skeleton_; - - /// @brief The underlying graph implementation. - IRunTargetControl* graph_; -}; - -Result ControlProvider::Create(IRunTargetControl* graph) noexcept +Result> ControlProvider::Create(IRunTargetControl* graph) noexcept { const Result instance_specifier_result = com::InstanceSpecifier::Create(std::string{"LaunchManager/StateManager/Instance"}); @@ -76,49 +36,43 @@ Result ControlProvider::Create(IRunTargetControl* graph) noexce } LmControlSkeleton skeleton = std::move(skeleton_result).value(); - // `unique_ptr` rather than the previous raw `new`: on any of the failure paths below, this - // is freed automatically instead of leaking (the previous version returned - // `MakeUnexpected(...)` without ever deleting the raw pointer it had just allocated). - auto impl = std::make_unique(std::move(skeleton), graph); + // Owned by a `unique_ptr` so that it is freed on any of the failure paths below. Not + // `std::make_unique`, because the constructor is private. + std::unique_ptr control_provider{new ControlProvider{std::move(skeleton), graph}}; - const Result setup_activate_run_target_result = impl->setupActivateRunTarget(); + const Result setup_activate_run_target_result = control_provider->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 = impl->setupGetActiveRunTarget(); + const Result setup_get_active_run_target_result = control_provider->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 = impl->setupActivationResult(); + const Result setup_activation_result_result = control_provider->setupActivationResult(); if (!setup_activation_result_result.has_value()) { return MakeUnexpected(static_cast(*setup_activation_result_result.error())); } - const Result offer_service_result = impl->offerService(); + const Result offer_service_result = control_provider->offerService(); if (!offer_service_result.has_value()) { return MakeUnexpected(static_cast(*offer_service_result.error())); } - return ControlProvider{std::move(impl)}; + return control_provider; } -ControlProvider::ControlProvider(std::unique_ptr impl) noexcept : impl_(std::move(impl)) +ControlProvider::ControlProvider(LmControlSkeleton skeleton, IRunTargetControl* graph) noexcept + : skeleton_(std::move(skeleton)), graph_(graph) { } -// Defined here rather than defaulted in the header: `Impl` is only a complete type from this -// point in the file onward, and a deleter/mover for `unique_ptr` needs that completeness. -ControlProvider::ControlProvider(ControlProvider&&) noexcept = default; -ControlProvider& ControlProvider::operator=(ControlProvider&&) noexcept = default; -ControlProvider::~ControlProvider() = default; - -Result ControlProvider::Impl::setupActivateRunTarget() noexcept +Result ControlProvider::setupActivateRunTarget() noexcept { const auto result = skeleton_.activate_run_target.RegisterHandler( [this](ActivateRunTargetResponse& response, const ActivateRunTargetRequest& request) { @@ -134,7 +88,7 @@ Result ControlProvider::Impl::setupActivateRunTarget() noexcept return {}; } -void ControlProvider::Impl::handleActivateRunTarget( +void ControlProvider::handleActivateRunTarget( ActivateRunTargetResponse& response, const ActivateRunTargetRequest& request) noexcept { @@ -175,7 +129,7 @@ void ControlProvider::Impl::handleActivateRunTarget( response = ActivateRunTargetResponse{status : RequestStatus::kAccepted}; } -Result ControlProvider::Impl::setupGetActiveRunTarget() noexcept +Result ControlProvider::setupGetActiveRunTarget() noexcept { const auto result = skeleton_.get_active_run_target.RegisterHandler([this](GetActiveRunTargetResponse& response) { this->handleGetActiveRunTarget(response); @@ -190,7 +144,7 @@ Result ControlProvider::Impl::setupGetActiveRunTarget() noexcept return {}; } -void ControlProvider::Impl::handleGetActiveRunTarget(GetActiveRunTargetResponse& response) noexcept +void ControlProvider::handleGetActiveRunTarget(GetActiveRunTargetResponse& response) noexcept { const score::Result result = graph_->getActiveRunTarget(); if (!result.has_value()) @@ -208,7 +162,7 @@ void ControlProvider::Impl::handleGetActiveRunTarget(GetActiveRunTargetResponse& response = GetActiveRunTargetResponse{status : QueryStatus::kAvailable, run_target}; } -Result ControlProvider::Impl::setupActivationResult() noexcept +Result ControlProvider::setupActivationResult() noexcept { graph_->registerActiveRunTargetCallback([this](IdentifierHash state, RunTargetActivationSource source) { this->handleActivationResult(state, source); @@ -217,7 +171,7 @@ Result ControlProvider::Impl::setupActivationResult() noexcept return {}; } -void ControlProvider::Impl::handleActivationResult(IdentifierHash state, RunTargetActivationSource source) noexcept +void ControlProvider::handleActivationResult(IdentifierHash state, RunTargetActivationSource source) noexcept { auto allocate_result = skeleton_.activation_result.Allocate(); if (!allocate_result.has_value()) @@ -248,7 +202,7 @@ void ControlProvider::Impl::handleActivationResult(IdentifierHash state, RunTarg LM_LOG_DEBUG() << "Sent the activation result to the state manager"; } -Result ControlProvider::Impl::offerService() noexcept +Result ControlProvider::offerService() noexcept { const auto result = skeleton_.OfferService(); if (!result.has_value()) diff --git a/score/launch_manager/src/daemon/src/control/control_provider.hpp b/score/launch_manager/src/daemon/src/control/control_provider.hpp index df98e7981..58664ae92 100644 --- a/score/launch_manager/src/daemon/src/control/control_provider.hpp +++ b/score/launch_manager/src/daemon/src/control/control_provider.hpp @@ -23,30 +23,52 @@ namespace score::mw::lifecycle::internal { /// @brief Provides the mw::com service for state managers to connect to. +/// @details This cannot be moved, because the mw::com callbacks reference +// the ControlProvider at its original location. It is therefore +// handed out as a `std::unique_ptr`, which keeps that address fixed. class ControlProvider { public: /// @brief Fallible constructor for ControllableGraph. - static Result Create(IRunTargetControl* graph) noexcept; + static Result> Create(IRunTargetControl* graph) noexcept; - // Movable: the mw::com/graph callbacks registered during `Create` capture a pointer to - // `Impl`, not to `ControlProvider` itself, so moving a `ControlProvider` only moves the - // `unique_ptr` — `Impl`'s address (the thing the callbacks actually point at) never - // changes. Declared out-of-line (not `= default` here) because `Impl` is still an - // incomplete type at this point; defined in the .cpp once `Impl` is complete. - ControlProvider(ControlProvider&&) noexcept; - ControlProvider& operator=(ControlProvider&&) noexcept; - ~ControlProvider(); + ~ControlProvider() = default; + // Cannot be moved because callbacks capture the ControlProvider by reference. ControlProvider(const ControlProvider&) = delete; + ControlProvider(ControlProvider&&) = delete; ControlProvider& operator=(const ControlProvider&) = delete; + ControlProvider operator=(ControlProvider&&) = delete; private: - class Impl; + explicit ControlProvider(LmControlSkeleton skeleton, IRunTargetControl* graph) noexcept; - explicit ControlProvider(std::unique_ptr impl) noexcept; + /// @brief Register the handler for activate_run_target. + Result setupActivateRunTarget() noexcept; - std::unique_ptr impl_; + /// @brief Handle an activate_run_target request. + void handleActivateRunTarget(ActivateRunTargetResponse& response, const ActivateRunTargetRequest& request) noexcept; + + /// @brief Register the handler for get_active_run_target. + Result setupGetActiveRunTarget() noexcept; + + /// @brief Handle a get_active_run_target request. + void handleGetActiveRunTarget(GetActiveRunTargetResponse& response) noexcept; + + /// @brief Register the handler for activation_result. + Result setupActivationResult() noexcept; + + /// @brief Handle an activation_result event. + void handleActivationResult(IdentifierHash state, RunTargetActivationSource source) noexcept; + + /// @brief Make the service available to clients. + Result offerService() noexcept; + + /// @brief The external `mw::com` interface. + LmControlSkeleton skeleton_; + + /// @brief The underlying graph implementation. + IRunTargetControl* graph_; }; } // namespace score::mw::lifecycle::internal 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 65028ad98..d256f748d 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 @@ -19,7 +19,7 @@ #include -#include +#include #include #include @@ -28,13 +28,12 @@ namespace score::mw::lifecycle::internal namespace { -// Locks in the exact contract this class was reworked to provide (see the class comment in -// control_provider.hpp): movable via a stable `Impl*` the registered callbacks capture, but -// never copyable, since copying would either alias or duplicate that `Impl`. A regression here -// (e.g. someone reinstating `= delete` on the move ops, or defaulting a copy op) would silently -// reintroduce the dangling-callback bug this Pimpl was introduced to fix. -static_assert(std::is_nothrow_move_constructible_v); -static_assert(std::is_nothrow_move_assignable_v); +// Locks in the contract from the class comment in control_provider.hpp: the registered callbacks +// capture the ControlProvider's own address, so it must stay neither movable nor copyable, and +// is handed out as a `std::unique_ptr` instead. A regression here (e.g. someone defaulting the +// move ops) would silently reintroduce a dangling-callback bug. +static_assert(!std::is_move_constructible_v); +static_assert(!std::is_move_assignable_v); static_assert(!std::is_copy_constructible_v); static_assert(!std::is_copy_assignable_v); @@ -82,36 +81,46 @@ TEST_F(ControlProviderUT, StaticAssertionsCompiled) SUCCEED(); } -TEST_F(ControlProviderUT, CallbacksDispatchThroughMovedInstance) +TEST_F(ControlProviderUT, CallbacksDispatchAfterOwnershipTransfer) { RecordProperty( "Description", - "After a ControlProvider is moved, its registered callbacks still dispatch through the " - "moved-to instance's Impl, not through a stale, already-destroyed one."); + "After the std::unique_ptr returned by Create() is moved to a different owner, the " + "registered callbacks still dispatch through the same, still-alive ControlProvider."); const IdentifierHash run_target_id{"control_provider_ut_run_target"}; - std::optional moved_to; + std::unique_ptr owner; { - Result create_result = ControlProvider::Create(&graph_); + Result> create_result = ControlProvider::Create(&graph_); ASSERT_TRUE(create_result.has_value()); - ControlProvider original = std::move(create_result).value(); - - // Move `original` into the outer-scoped `moved_to`, then let `original` (and this - // block) be destroyed. If the registered callback captured `original`'s own address - // instead of the stable, heap-allocated `Impl*` -- the exact bug the Pimpl in this PR - // fixes -- that address is gone once this block ends, and triggering the callback below - // would be a genuine use-after-scope, not just a theoretical one. - moved_to.emplace(std::move(original)); + // Only the pointer moves; the ControlProvider the callbacks captured stays where it is. + owner = std::move(create_result).value(); } - ASSERT_TRUE(moved_to.has_value()); + ASSERT_NE(owner, nullptr); // Doesn't crash and doesn't trip ASan/UBSan iff the callback dispatches through the - // still-alive `Impl` that `moved_to` now owns. + // still-alive ControlProvider that `owner` now holds. graph_.TriggerActivation(run_target_id, RunTargetActivationSource::kStateManagerRequest); } +TEST_F(ControlProviderUT, DestructionReleasesService) +{ + RecordProperty( + "Description", + "Destroying the ControlProvider returned by Create() releases the offered service, so " + "a later Create() for the same instance succeeds again."); + + Result> first = ControlProvider::Create(&graph_); + ASSERT_TRUE(first.has_value()); + first.value().reset(); + + FakeRunTargetControl second_graph; + Result> second = ControlProvider::Create(&second_graph); + EXPECT_TRUE(second.has_value()); +} + } // namespace } // namespace score::mw::lifecycle::internal diff --git a/score/launch_manager/src/daemon/src/run.cpp b/score/launch_manager/src/daemon/src/run.cpp index 3ee76c533..80720022d 100644 --- a/score/launch_manager/src/daemon/src/run.cpp +++ b/score/launch_manager/src/daemon/src/run.cpp @@ -18,7 +18,6 @@ #include #include #include -#include #include "score/mw/launch_manager/alive_monitor/details/daemon/AliveMonitorImpl.hpp" #include "score/mw/launch_manager/common/log.hpp" @@ -184,21 +183,26 @@ int run(int argc, const char* argv[]) // registered via `registerActiveRunTargetCallback`. If `ControlProvider` were destroyed // before that drain finishes -- as it would be if scoped only to the `if` block below -- // that callback would fire through a dangling pointer. - std::optional> control_provider_result; + std::unique_ptr control_provider; if (process_group_manager->initialize()) { - control_provider_result = ControlProvider::Create(process_group_manager.get()); + score::Result> control_provider_result = + ControlProvider::Create(process_group_manager.get()); - if (!control_provider_result->has_value()) + if (!control_provider_result.has_value()) { LM_LOG_FATAL() << "Failed to set up LmControl service provider:" - << control_provider_result->error().Message(); + << control_provider_result.error().Message(); exit_code = EXIT_FAILURE; } - else if (runLCMDaemon(*process_group_manager)) + else { - exit_code = EXIT_SUCCESS; + control_provider = std::move(control_provider_result).value(); + if (runLCMDaemon(*process_group_manager)) + { + exit_code = EXIT_SUCCESS; + } } } @@ -208,7 +212,7 @@ int run(int argc, const char* argv[]) process_group_manager.reset(); } - // Safe to let `control_provider_result` go out of scope now: `deinitialize()` above has + // Safe to let `control_provider` go out of scope now: `deinitialize()` above has // already joined every worker and reset `graph_`, so no further callback can arrive. } catch (...)