Skip to content

Avoid Debug TLS leaks and make Windows network detection reload-safe - #1544

Open
bmehta001 wants to merge 3 commits into
microsoft:mainfrom
bmehta001:fix/debug-listener-tls
Open

bmehta001 wants to merge 3 commits into
microsoft:mainfrom
bmehta001:fix/debug-listener-tls

Conversation

@bmehta001

@bmehta001 bmehta001 commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fix Debug Windows leaks when a DLL embedding the SDK is unloaded with attaching
threads still alive, and make Windows network detection survive repeated DLL
loads without requiring the host to keep a COM MTA alive.

Pending listeners

  • Replace the namespace-scope owning TLS vector with a trivial TLS pointer to
    PendingListenersScope-owned storage.
  • Preserve outer pending-list snapshots across nested dispatch, duplicate
    registrations, removal, exception unwinding, and reentrant release callbacks.
  • Return false without allocating when no dispatch scope is active.

Network detector

The SDK-only reproducer failed on the second load with the former WinRT
activation path. Keeping a host MTA alive isolated the dependency but is not
part of this fix.

  • Use dynamically resolved IP Helper connectivity hints and change notifications
    on Windows 10 version 2004/build 19041 and later, without WinRT or NLM.
  • Preserve legacy detector API coverage using native INetworkListManager
    and the original three event families on a private SDK-owned STA.
    Query INetworkCostManager only as an optional capability. Missing cost
    support or logged cost-query failures retain Unknown cost while monitoring
    continues, matching the pre-Fix Windows network detection leak, add periodic leak reports, and standardize dependencies #1536 behavior. Server does not require the
    unsupported cost interface. No cost-specific event subscriptions are required.
    Unsubscribe, disconnect, release all interfaces, and balance COM initialization
    on the owning thread before joining it.
  • Marshal cost refreshes onto the listener rather than sharing raw COM
    interfaces across apartments; recognize listener generations across restart.
  • Drain thread-pool callbacks through completion, including concurrent external
    stops after a reentrant stop. Restart also drains the previous dispatch.
    Balance each callback's temporary DLL reference with
    FreeLibraryWhenCallbackReturns; do not permanently retain the DLL.
  • Complete private STA rundown even if explicit sink disconnection fails:
    log the failure, release interfaces, and call CoUninitialize before joining.
    This closes its RPC connections without terminating the host. Native
    notification cancellation failure remains fatal because it has no apartment
    rundown safety mechanism.
  • Keep existing Dr. Memory leak baselines unchanged. The full unit run includes
    the forced legacy backend; a separate modern-backend run preserves the
    no-netprofm.dll gate alongside functional/sample production paths.

The modern backend reports aggregate connectivity hints rather than only the
WinRT Internet connection profile. These hints do not expose WinRT's separate
background-data restriction flag. Roaming and approaching/exceeded data limits
remain restrictive. The legacy fallback legitimately loads netprofm.dll.

Validation

The Windows hosts do not link the SDK. They embed the static SDK in a Debug DLL,
require the shared Debug CRT/default Debug STL iterator checking, verify actual
DLL unloading, and count outstanding normal/client CRT allocations.

Seven-live-thread unload case Original Microsoft main Fixed
Idle / SDK never called 7 blocks, 112 bytes 0 blocks, 0 bytes
Listener dispatch/removal 14 blocks, 168 bytes 0 blocks, 0 bytes
IP Helper start/read/stop Not applicable 0 blocks, 0 bytes
Forced native COM fallback start/read/stop Not applicable 0 blocks, 0 bytes
Legacy cost support absent Not applicable 0 blocks, 0 bytes
Legacy cost-query/subscription/disconnect failures Not applicable 0 blocks, 0 bytes
  • Windows x64 and Win32 Debug, Visual Studio 2026: all 59 selected listener/network
    tests pass, covering default, legacy and no-cost capabilities, failed subscription/retry, repeated
    start/read/stop, queued refresh races, concurrent/reentrant stop, callback drain,
    restart, and cost reads across listener generations.
  • All ten DLL regression cases pass. All four network modes survive five load/start/
    read/stop/unload cycles
    , with 0 blocks / 0 bytes per cycle, while the host
    COM apartment remains uninitialized. All seven worker threads are confirmed
    alive at the six live-thread unload checkpoints.
  • All ten DLL cases passed a five-repeat stress run. Earlier revisions also
    passed ten DLL stress rounds and three focused-unit rounds.
  • Linux/WSL Debug, GCC 13: the actual SDK branch builds and all 27
    DebugEventSourceTests.* pass.
  • No tests were skipped or negatively excluded in these selected enabled-SDK
    suites. The feature-disabled module reports explicit skip code 77 for network
    cases; its idle/dispatch cases still pass with zero blocks/bytes.
  • Repository-pinned misspell v0.3.4 and git diff --check pass.
  • Windows Server 2022 CI: all six Win32/x64 Debug/Release WinHTTP/WinInet
    jobs pass. The x64 Debug job runs 653 unit tests, including the forced
    legacy/no-cost and failure-path tests, without skipping those paths.
  • Current-head Windows and Linux Dr. Memory workflows complete successfully.
    The isolated modern network backend, Windows functional tests and basic sample
    each report zero actual and possible leaks; the modern production paths
    also pass the no-netprofm.dll check.
  • The full Windows unit scan is not leak-free: it reports 15 unique / 17
    total actual leaks (824 bytes), and 39 unique / 44 total possible leaks
    (225,601 bytes). The unchanged baseline comparison emits warnings, not a
    failing exit status. Its successful workflow status must not be interpreted
    as a clean leak baseline.
  • One actual-leak stack is a 528-byte allocation through NETPROFM.dll,
    CoInitializeEx and an OS WNF notification thread. Other actual-leak stacks
    involve offline-storage and HTTP tests. These full-suite results do not
    establish whether the legacy backend grows native allocations per lifecycle;
    no zero-native-heap claim is made for the legacy path.

Coverage: The base NLM APIs are documented for Vista/Server 2008 onward;
optional cost APIs are Windows 8-client APIs with no supported Server versions.
The detector no longer requires WinRT or Windows 8 cost support on the
Windows 7 SP1/Server 2008 R2 range referenced by the old source. This does not
change the whole SDK's support policy or compiler/runtime requirements.

Limits: Forced legacy execution on the current Windows host is not actual
Server 2016/2019 validation. These are SDK-only embedding tests, not an ORT
Windows consumer build: the inspected ORT checkout uses ETW on Windows and
excludes 1DS from its standard Windows build. The Debug CRT checks are not a
claim of zero allocations in every OS/native heap. The CI-pinned Dr. Memory
tool could not launch even an SDK-independent target locally (0xc06d007f);
native-heap results above come from the supported Windows CI host.
The workflow's pre-existing timing-sensitive exclusions are unchanged, and
no new exclusions or relaxed leak baselines have been added.

Based on Microsoft main at 3886eda702c3dd986bf47160a6e91b91f19114ae.

Keep TLS trivial and let each dispatch scope own the pending-list snapshot
so unused attaching threads allocate nothing and completed dispatches retain
no thread-owned storage. Preserve nested listener and release semantics.

Add listener coverage and an SDK-only unload regression that checks zero
outstanding blocks and bytes with seven worker threads still alive.

Files changed:
- lib/callbacks/DebugSource.cpp
- lib/callbacks/DebugSourceInternal.hpp
- tests/CMakeLists.txt
- tests/unittests/DebugEventSourceTests.cpp
- tests/dll-unload/CMakeLists.txt
- tests/dll-unload/debug-listener-unload-module.cpp
- tests/dll-unload/debug-listener-unload-test.cpp
- tests/dll-unload/README.md

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e3876793-eab3-449a-b32b-5a983d24a6c3
@bmehta001
bmehta001 requested a review from a team as a code owner October 3, 2026 02:19
Replace the WinRT activation path that can fail or hang after final apartment
teardown. Resolve modern IP Helper APIs dynamically and balance native NLM
subscriptions in an SDK-owned STA on older supported Windows.

Drain dispatched callbacks through completion, retain the embedding DLL only
until callback return, and preserve drain state for reentrant/concurrent stop
and restart. Keep cost refreshes on the backend's owning thread.

Files changed:
- lib/pal/desktop/NetworkDetector.cpp
- lib/pal/desktop/NetworkDetector.hpp
- tests/common/network-detector-test-access.hpp
- tests/unittests/NetworkDetectorTests.cpp
- tests/dll-unload/CMakeLists.txt
- tests/dll-unload/debug-listener-unload-module.cpp
- tests/dll-unload/debug-listener-unload-test.cpp
- tests/dll-unload/network-detector-reload-test.cpp
- tests/dll-unload/README.md
- docs/building-custom-SKU.md
- .github/workflows/memory-leak-analysis.yml

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e3876793-eab3-449a-b32b-5a983d24a6c3
@bmehta001 bmehta001 changed the title Avoid Debug STL TLS leaks when SDK DLLs unload with live threads Avoid Debug TLS leaks and make Windows network detection reload-safe Oct 3, 2026
Before the WinRT-only detector change, NLM connectivity monitoring could run
when its optional cost interface was unavailable. Activate INetworkListManager
first and keep Unknown cost after logged query failures, including on Server.

Preserve the original three NLM event families instead of requiring the newly
added cost-specific events. Let the private non-agile sink's STA complete COM
rundown after an explicit disconnect failure instead of terminating the host.

Cover no-cost operation, cost-query errors, partial subscriptions and apartment
rundown in unit tests and the zero-allocation DLL unload/reload harness.

Files changed:
- lib/pal/desktop/NetworkDetector.cpp
- lib/pal/desktop/NetworkDetector.hpp
- tests/common/network-detector-test-access.hpp
- tests/unittests/NetworkDetectorTests.cpp
- tests/dll-unload/CMakeLists.txt
- tests/dll-unload/debug-listener-unload-module.cpp
- tests/dll-unload/debug-listener-unload-test.cpp
- tests/dll-unload/network-detector-reload-test.cpp
- tests/dll-unload/README.md
- docs/building-custom-SKU.md

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e3876793-eab3-449a-b32b-5a983d24a6c3

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant