fix(launch_manager): fix ControlProvider leak via unique_ptr by hskang-amelia · Pull Request #654 · eclipse-score/lifecycle

hskang-amelia · 2026-09-18T04:34:52Z

No description provided.

hskang-amelia · 2026-09-18T07:31:53Z

Note on test coverage

One gap I want to flag explicitly: no existing unit test references
ControlProvider, so as it stands this change lands without regression
coverage. Since #649 is classified as Safety & Security Relevant, I don't
think that's good enough to ask for an approval on.

I'll add a test that creates a ControlProvider via Create(), moves it,
and then asserts the registered callbacks still dispatch through the correct
Impl — i.e. that the move leaves no handler pointing at the original
object. That pins down exactly the use-after-free this PR is meant to remove,
and it would fail against the pre-Pimpl code.

If reviewers would prefer a different shape for that test (e.g. also covering
the Create() failure path now that cleanup is RAII-based), say so and I'll
fold it in before pushing.

Confirms and addresses eclipse-score#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<Impl> 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<ControlProvider> by value instead of Result<ControlProvider*>, 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<ControlProvider*> 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 <amelia@ivis.ai>

github-actions · 2026-09-21T01:10:55Z

Documentation preview for this pull request is available at:
pr-654: https://eclipse-score.github.io/lifecycle/pr-654/

Fulfills the test-coverage gap flagged on PR eclipse-score#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 eclipse-score#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 <amelia@ivis.ai>

hskang-amelia · 2026-09-21T04:44:16Z

Verified this branch (fix/control-provider-pimpl-rebased) end-to-end via ghcr.io/eclipse-score/devcontainer:v1.11.0

Build + unit tests: control_provider + launch_manager build cleanly; //score/launch_manager/... 32/32 unit tests pass, including control_provider_UT.
switch_run_target integration test (real IPC, real launch_manager + real control_client_test_driver): exercises ActivateRunTarget/GetActiveRunTarget — exactly the callback path this PR changes — round trips, run target switching, and unrecognized-run-target rejection all pass.
Full tests/integration/* suite, run individually: all pass except sandbox_options, which fails only in --//config:integration_mode=host (used here since this sandbox has no nested docker) — that mode wraps the binary in fakeroot, which no-ops setuid/setgid without erroring. Confirmed via strace that the real syscalls succeed cleanly outside fakeroot, and that integration_mode=docker (what CI actually uses) never wraps in fakeroot at all. Not a code issue, not related to this PR.
No regressions from this change.

Follows the review suggestion on eclipse-score#654: instead of a Pimpl that makes ControlProvider movable, Create() now returns Result<std::unique_ptr<ControlProvider>>. 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 <amelia@ivis.ai> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Follows the review suggestion on eclipse-score#654: instead of a Pimpl that makes ControlProvider movable, Create() now returns Result<std::unique_ptr<ControlProvider>>. 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 <amelia@ivis.ai>

MaciejKaszynski · 2026-09-23T10:15:46Z

+    ActivationCallbackT callback_;
+};
+
+class ControlProviderUT : public ::testing::Test


Can you add a setup to add a test type property for test linkage.

e.g.

lifecycle/score/launch_manager/src/exec_error_domain_UT.cpp

Lines 50 to 54 in bb79e12

void SetUp() override

{

RecordProperty("TestType", "interface-test");

RecordProperty("DerivationTechnique", "explorative-testing");

}

MaciejKaszynski · 2026-09-23T10:21:42Z

+int main(int argc, char** argv)
+{
+    ::testing::InitGoogleTest(&argc, argv);
+    score::mw::com::runtime::InitializeRuntime(


I noticed that we are not mocking mw::com. We should mock it however all the other unit tests also don't mock. I will make a Issue for this. #688

MaciejKaszynski · 2026-09-23T10:24:01Z

+        // 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.


Too verbose don't need it

Suggested change

// 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.

MaciejKaszynski · 2026-09-23T10:25:32Z

+        // 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.


Too verbose, don't think we need to explain anything here, people should know about lifetimes

Suggested change

// 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.

hskang-amelia requested review from FScholPer, MaciejKaszynski, NicolasFussberger, anmittag, antonkri, pawelrutkaq and ramceb as code owners September 18, 2026 04:34

github-project-automation Bot added this to LCM - Lifecycle & Health FT Sep 18, 2026

github-project-automation Bot moved this to Backlog in LCM - Lifecycle & Health FT Sep 18, 2026

hskang-amelia requested a deployment to workflow-approval September 18, 2026 04:34 — with GitHub Actions Waiting

hskang-amelia mentioned this pull request Sep 18, 2026

fix(launch_manager): avoid leaking ControlProvider on failure #653

Closed

hskang-amelia force-pushed the fix/control-provider-pimpl-rebased branch from a84e566 to a2d7f38 Compare September 21, 2026 01:00

hskang-amelia requested a deployment to workflow-approval September 21, 2026 01:00 — with GitHub Actions Waiting

hskang-amelia requested a deployment to workflow-approval September 21, 2026 03:30 — with GitHub Actions Waiting

hskang-amelia mentioned this pull request Sep 21, 2026

Bump score_lifecycle pin and fix simple_lifecycle mw::com config eclipse-score/reference_integration#350

Open

MaciejKaszynski reviewed Sep 22, 2026

View reviewed changes

Comment thread score/launch_manager/src/daemon/src/control/control_provider.hpp Outdated

Merge branch 'main' into fix/control-provider-pimpl-rebased

28b0c01

hskang-amelia requested a deployment to workflow-approval September 23, 2026 04:31 — with GitHub Actions Waiting

hskang-amelia changed the title ~~fix(launch_manager): Pimpl ControlProvider to make it movable~~ fix(launch_manager): fix ControlProvider leak via unique_ptr Sep 23, 2026

hskang-amelia requested a deployment to workflow-approval September 23, 2026 08:05 — with GitHub Actions Waiting

hskang-amelia force-pushed the fix/control-provider-pimpl-rebased branch from 1f220e6 to ab61cb6 Compare September 23, 2026 08:28

hskang-amelia requested a deployment to workflow-approval September 23, 2026 08:28 — with GitHub Actions Waiting

danth mentioned this pull request Sep 23, 2026

Close UT gap in component daemon/src/control #685

Open

1 task

MaciejKaszynski requested changes Sep 23, 2026

View reviewed changes

Navigation Menu

fix(launch_manager): fix ControlProvider leak via unique_ptr - #654

fix(launch_manager): fix ControlProvider leak via unique_ptr#654
hskang-amelia wants to merge 4 commits into
eclipse-score:mainfrom
hskang-amelia:fix/control-provider-pimpl-rebased

hskang-amelia commented Sep 18, 2026 •

edited

Loading

Uh oh!

hskang-amelia commented Sep 18, 2026

Uh oh!

github-actions Bot commented Sep 21, 2026

Uh oh!

hskang-amelia commented Sep 21, 2026

Uh oh!

Uh oh!

MaciejKaszynski Sep 23, 2026

Uh oh!

MaciejKaszynski Sep 23, 2026

Uh oh!

MaciejKaszynski Sep 23, 2026

Uh oh!

MaciejKaszynski Sep 23, 2026

Uh oh!

This branch is waiting to be deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	void SetUp() override
	{
	RecordProperty("TestType", "interface-test");
	RecordProperty("DerivationTechnique", "explorative-testing");
	}

		// 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.

Conversation

hskang-amelia commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hskang-amelia commented Sep 18, 2026

Uh oh!

github-actions Bot commented Sep 21, 2026

Uh oh!

hskang-amelia commented Sep 21, 2026

Uh oh!

Uh oh!

MaciejKaszynski Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

MaciejKaszynski Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

MaciejKaszynski Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

MaciejKaszynski Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

This branch is waiting to be deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hskang-amelia commented Sep 18, 2026 •

edited

Loading