fix(launch_manager): avoid leaking ControlProvider on failure - #653
Closed
hskang-amelia wants to merge 3 commits into
Closed
hskang-amelia wants to merge 3 commits into
hskang-amelia wants to merge 3 commits into
Conversation
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<ControlProvider*> signature.
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.
hskang-amelia
requested review from
FScholPer,
MaciejKaszynski,
NicolasFussberger,
anmittag,
antonkri,
pawelrutkaq and
ramceb
as code owners
September 18, 2026 04:26
hskang-amelia
requested a deployment
to
workflow-approval
September 18, 2026 04:26 — with 
GitHub Actions
Waiting
GitHub Actions
Waiting
hskang-amelia
requested a deployment
to
workflow-approval
September 18, 2026 04:26 — with
GitHub Actions
Waiting
GitHub Actions
Waiting
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.
hskang-amelia
requested a deployment
to
workflow-approval
September 18, 2026 04:31 — with
GitHub Actions
Waiting
GitHub Actions
Waiting
hskang-amelia
requested a deployment
to
workflow-approval
September 18, 2026 04:31 — with
GitHub Actions
Waiting
GitHub Actions
Waiting
github-project-automation
Bot
moved this from Backlog
to Done
in LCM - Lifecycle & Health FT
Contributor
Author
|
Superseded by #654 — while working on this, #649 (already investigated and implemented on a separate branch) appears to offer a more complete solution to the same root cause. That solution makes ControlProvider properly movable via a Pimpl, which incidentally also fixes the memory leak from Create() failures, and also resolves a lifetime issue in main.cpp that the Pimpl redesign itself could introduce. To avoid having two PRs touching the same code, closing this leak-only fix and prioritizing #649's solution. |
hskang-amelia
added a commit
to hskang-amelia/lifecycle
that referenced
this pull request
Sep 21, 2026
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>
This branch is waiting to be deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ControlProvider::Create()heap-allocates aControlProvidervia a barenewand returnsMakeUnexpected(...)on any of the four setup-stepfailure paths without ever deleting it, leaking the object.
How
Wraps the allocation in a
unique_ptrguard that frees it automatically onany early return, and only
release()s the raw pointer once every setupstep has succeeded. No change to
control_provider.hppor theResult<ControlProvider*>signature.Testing
bazel build --lockfile_mode=error --config=x86_64-linux //score/launch_manager/src/daemon/src/control:control_provider //score/launch_manager/src/daemon:launch_managerbazel test --lockfile_mode=error --config=x86_64-linux //score/launch_manager/...(the subset that isn't blocked by this environment's unrelated Ferrocene/glibc issue)control_provider_UTwith two tests exercisingCreate()'s happypath and a second
Create()for an already-offered instance failingcleanly. Neither reaches the
unique_ptrguard itself (both fail earlier,at
LmControlSkeleton::Create()'s flock check) — no config-based way tomake
RegisterHandler/OfferServicefail after skeleton creationsucceeds was found without mocking, noted in the test file.
launch_managerdaemon offthis branch and ran it against a real client application over actual
shared-memory IPC in a private downstream integration harness — 2/2
ActivateRunTargetround trips succeeded.Related
Result<ControlProvider>at 1c243af (pre-final-review) tried to fixthis same non-movability concern via a Pimpl; superseded by this repo's
own
Result<ControlProvider*>pointer-return design in the final reviewpass. This PR keeps that design and only fixes the leak on top of it.