Fix DVC plugin class factory lifetime after session removal - #192
Merged
Marc-André Moreau (mamoreau-devolutions) merged 5 commits intoSep 24, 2026
Merged
Marc-André Moreau (mamoreau-devolutions) merged 5 commits into
Marc-André Moreau (mamoreau-devolutions) merged 5 commits into
Conversation
Capture the session ID rather than borrowing an instance pointer, and acquire the registered plugin while synchronized with removal and replacement. Exercise stale factories and replacement during plugin QueryInterface without opening an RDP session. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Marc-André Moreau (mamoreau-devolutions)
September 24, 2026 10:56
View session
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The lifetime and COM ownership changes are internally consistent and covered by focused regression tests.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes DVC class-factory lifetime and COM ownership when sessions or plugins are removed or replaced.
Changes:
- Resolves plugins by session GUID with synchronized temporary ownership.
- Corrects class-factory reference counting and error handling.
- Adds coverage for removal, replacement, and built-in plugin fallback.
| File | Description |
|---|---|
dll/MsRdpEx.cpp |
Uses the session-based factory API. |
dll/RdpDvcClient.cpp |
Implements safe factory and plugin lifetimes. |
dll/RdpDvcClient.h |
Updates the factory signature. |
dll/RdpInstance.cpp |
Adds synchronized plugin acquisition. |
dll/RdpInstanceInternal.h |
Declares the acquisition helper. |
tests/logging/DvcFactoryLifetimeTest.cpp |
Tests factory lifetime scenarios. |
tests/logging/PluginReferenceFixture.cpp |
Exposes fixture lifecycle operations. |
tests/logging/GatewayShutdownFixture.def |
Exports new fixture functions. |
tests/logging/CMakeLists.txt |
Registers the new tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Marc-André Moreau (mamoreau-devolutions)
marked this pull request as draft
September 24, 2026 12:23
Pin plugins with an internal nothrow reference holder, so COM AddRef and Release run outside locks. Coordinate session lookups with manager teardown, exercise the production factory route, and cover reentrant callbacks plus in-flight shutdown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Marc-André Moreau (mamoreau-devolutions)
marked this pull request as ready for review
September 24, 2026 12:29
Marc-André Moreau (mamoreau-devolutions)
marked this pull request as draft
September 24, 2026 12:31
Do not release the transferred reference on holder allocation failure: managed callers release it on failure. Document success-only transfer and test forced OOM, same-pointer retry, and plugin QueryInterface failure. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Marc-André Moreau (mamoreau-devolutions)
marked this pull request as ready for review
September 24, 2026 12:34
Marc-André Moreau (mamoreau-devolutions)
marked this pull request as draft
September 24, 2026 13:04
Close the plugin slot before releasing the final holder so reentrant COM callbacks cannot access a freed holder or register a replacement on an instance being destroyed. Cover reentrant getters and setters during teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Marc-André Moreau (mamoreau-devolutions)
marked this pull request as ready for review
September 24, 2026 13:07
Marc-André Moreau (mamoreau-devolutions)
marked this pull request as draft
September 24, 2026 13:07
Combine the fixture exports and native tests from detached-instance lifetime work with the DVC class factory tests, preserving both changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Marc-André Moreau (mamoreau-devolutions)
marked this pull request as ready for review
September 24, 2026 13:09
Marc-André Moreau (mamoreau-devolutions)
added a commit
that referenced
this pull request
Sep 24, 2026
## Summary
This closes the **primary root cause** of RDMW-24263 / the forum report
("Embedded RDP sessions leak the ActiveX control after the tab is
closed"): the wrapped `mstscax` control instance itself was never
released, so every closed embedded session permanently leaked 2 native
threads (`mstscax!CSND::SND_Main`, `mstscax!CRCV::RCVMain`) and ~100
handles.
It also fixes a refcount-imbalance bug that is a strong suspect for the
intermittent native heap-corruption crash (`ntdll.dll 0xC0000374`)
observed during teardown/forced-GC testing on the RDM side.
## Bugs fixed
1. **`CClassFactory::CreateInstance` leaked the real control's reference
(primary leak).**
`IClassFactory::CreateInstance` returns the real `mstscax` control
object with refcount 1, owned by the caller. `CMsRdpClient`'s
constructor takes its *own* `AddRef()` on that pointer, but the factory
never released its copy after wrapping — so the real control's refcount
never reached 0 when the `CMsRdpClient` wrapper was later destroyed.
Nothing ever drove the control's core shutdown, matching the reported
symptom exactly (send/receive threads parked forever in
`CTSThread::ThreadMsgLoop`).
2. **`CMsRdpClient` destructor released its wrapper objects after the
control interfaces.**
`m_pMsRdpExtendedSettings` and `m_pMsRdpExInstance` hold references into
control-internal objects whose lifetime is owned by the control.
Reordered so wrappers are released first, before the control interfaces.
3. **`CMsRdpPropertySet` released a reference it never took.**
It stored the internal `ITSPropertySet` pointer (for core/base/transport
TS property sets) without an `AddRef`, but its destructor
unconditionally called `Release()` on it anyway — decrementing a
refcount it never incremented, which could prematurely free a
control-internal object still referenced elsewhere. Added the missing
`AddRef()` in the constructor to balance the destructor's `Release()`.
## Verification
- Full x64 build (Ninja/MSVC).
- `ctest` — **31/31 passing**, including all existing DVC factory /
plugin-reference / detached-instance regression tests from #189–#192.
## How to verify the primary leak fix manually
Open and close N embedded RDP sessions, take a full-memory dump, count
threads whose stack contains `mstscax!CSND::SND_Main`. Before this fix:
N threads remain. After: 0.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
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.
Summary
REGDB_E_CLASSNOTREG.AddRef/ReleaseandQueryInterfacerun outside the instance and registry locks. The borrowed getter and success-only ownership-transfer setter ABI—including same-pointer replacement—remain unchanged. If holder allocation fails, the caller retains its incoming reference, matching Release managed WTS plugin reference when registration fails #190.DllGetClassObjectsession lookup and factory plugin acquisition with manager teardown. Keep manager destruction and plugin releases outside the global lock.E_UNEXPECTEDand preserve caller ownership. This closes the previously identified destructor reentrancy use-after-free/double-release path.QueryInterface, reentrantAddRef/Release, destructor reentrancy, manager shutdown during an in-flight factory call, deterministic allocation failure (including same-pointer retry), and failed pluginQueryInterfacewithout connecting RDP.Validation
masterafter integrating Release managed WTS plugin reference when registration fails #190 and Fix detached RDP instance reference leak #191.ctest --test-dir build-dvc-x64 --output-on-failure: 31/31 passed, includinglogging.dvc-factory.destructor-reentrantand the detached-instance regression from Fix detached RDP instance reference leak #191.A plugin callback using a raw instance pointer after the instance's final COM
Releaseis outside the normal COM lifetime contract; this regression specifically guards synchronous reentry during native teardown. Local build used an ignored, untrackedbuild-dvc-x64/stubs/atlbase.hcompatibility shim because this VS2026 installation lacks ATL. Nothing from the shim, package versions, or workflows is included. Validation with the full ATL toolchain or a live RDP connection remains outstanding. No change to the original mstscaxpUnknownrelease behavior.