Skip to content

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

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

bmehta001 wants to merge 5 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 the native Network List Manager COM fallback when IP Helper
    connectivity hints are unavailable. Own its STA, interfaces and the original
    three event families entirely inside the SDK, without a host-owned MTA.
    Query cost support optionally; unavailable or failed cost queries retain
    unknown cost while base connectivity monitoring continues.
  • Unsubscribe and release interfaces before completing private STA rundown.
    Explicit COM disconnection failure logs an error but does not terminate the
    host.
  • Consumers can set CFG_BOOL_ENABLE_NET_DETECT = false before SDK
    initialization to avoid constructing either detector backend. Preserve
    existing disabled-detection cost behavior. Add unit and actual DLL
    unload/reload regressions for this configuration.
  • Perform cost refreshes on the listener and 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.
  • Keep native notification cancellation failure fatal because allowing
    callbacks into unloaded SDK code is unsafe.
  • Keep existing broad-suite Dr. Memory leak baselines unchanged. Require zero
    actual and possible leaks in an isolated modern/disabled detector scan.
    Enforce no netprofm.dll loads for those paths and functional/sample
    production scenarios. The full unit suite deliberately exercises COM.

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 user chose to retain older-Windows compatibility and disable detection
in unload-sensitive consumers instead. A prior forced-NLM scan reported a
528-byte COM allocation on an OS notification thread. Its precise root cause
and per-cycle growth remain unestablished; retaining COM is not a claim that
this native-heap finding has been fixed. The original Debug listener TLS
allocation fix remains intact.

Update the README Target Platforms CI column to distinguish native runtime
tests, cross-builds, API-floor checks and OS versions without GitHub runners.

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
Network detection disabled in runtime configuration Not applicable 0 blocks, 0 bytes
Forced 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/disconnection failures Not applicable 0 blocks, 0 bytes
  • Windows x64 and Win32 Debug, Visual Studio 2026: all 60 selected listener/network
    tests pass, covering runtime disablement, legacy/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 twelve DLL regression cases pass. All five 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 seven live-thread unload checkpoints.
  • 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.
  • The embedding DLL does not import the newer IP Helper query/notification
    APIs directly. The network detector no longer activates WinRT.
  • Local all-available-module Windows Debug: 1,320/1,320 unit tests pass.
    The full functional suite passes 101/105, with four sanitizer/privacy
    concern-count assertions reporting one extra event. All 13 sanitizer
    functional tests pass in isolation
    , indicating sequence-dependent test
    state; this does not turn the failed full-suite run into a pass.
  • WSL all-available-module Clang ASan/LSan: 766/766 unit assertions pass
    and 86/86 functional assertions pass, but both processes exit 1
    because leak checking fails. Units report 219,952 bytes in 174
    allocations
    ; functional tests report 17,298 bytes in 105 allocations.
    A positive control established working leak detection. No negative filters
    or leak suppressions were added.
  • Preliminary attribution separates test leaks from production paths:
    the unit suite leaves caller-owned HTTP requests unfreed, including a
    204,800-byte request body. Functional stacks include
    DefaultDataViewer::SendPacket and LiveEventInspector::InspectRecord.
    DataViewer does not release requests borrowed by the current SDK transports
    or delete the responses transferred to its callback by the public contract.
    Its mocks also have incorrect ownership, including wrapping the borrowed
    callback in an owning shared_ptr. These module/test defects are not fixed
    by the listener TLS or Windows network-lifecycle changes in this PR.
  • Native-memory CI
    completed successfully for restoration revision 2bfc8fb5. The strict
    modern/runtime-disabled detector scan has zero actual and possible leaks,
    and the no-NLM production-path gate passes. Its unit scans remain nonzero:
Current-head CI configuration Actual leaks (total allocations / bytes) Possible leaks (total allocations / bytes)
Windows unit tests, including forced legacy COM 30 / 1,096 40 / 225,025
Windows modern/runtime-disabled detector 0 / 0 0 / 0
Windows functional tests and sample, each 0 / 0 0 / 0
Linux unit tests 13 / 3,425 11 / 209,135
Linux functional tests and sample, each 0 / 0 0 / 0

These CI configurations do not exercise all the optional modules included in
the local all-module runs above. Their unchanged unit baseline checks emit
warnings rather than fail CI, so workflow success is not whole-SDK leak
freedom. The restored Windows unit report again contains the 528-byte
NETPROFM/COM finding on an OS WNF notification thread
. Its root cause and
per-cycle growth are still unestablished. No new leak baselines or timing
exclusions were introduced.

All six current-head Win32/x64 Debug/Release WinHTTP/WinInet test jobs and the
Windows public-header/API-floor gate pass.

Coverage: IP Helper connectivity hints are used where available (Windows
10 2004/build 19041 and corresponding Server releases). The COM fallback
preserves base NLM coverage without requiring client-only cost interfaces.
This does not change the whole SDK's OS support policy or compiler/runtime
requirements. Runtime disablement and custom-SKU exclusion remain available.

Limits: Forced legacy execution on the current Windows host is not actual
older-OS execution. 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.
Android native ASan detects a known memory-error control, but its runtime
forces leak detection off and does not report a known-leak control.
Standalone LSan fails to link with both installed NDK versions. The complete
Android native unit binary also has an existing Room/SQLite backend linkage
mismatch. The native functional run without a Java VM encounters configuration
failures and a subsequent sanitizer error; its isolated bad-network test
passes. None of these Android results establishes leak freedom.
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
bmehta001 and others added 3 commits October 3, 2026 01:41
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
Keep IP Helper detection where both runtime APIs are available. On older
Windows, retain unknown network cost without starting listener resources,
rather than activating Network List Manager and its internal COM threads.

Replace legacy coverage with missing-API and native failure/retry tests.
Require zero actual and possible leaks in the isolated detector CI scan
and extend the no-NLM module gate to the full unit suite.

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/debug-listener-unload-module.cpp
- tests/dll-unload/debug-listener-unload-test.cpp
- tests/dll-unload/network-detector-reload-test.cpp
- tests/dll-unload/CMakeLists.txt
- 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
Restore the optional-cost NLM backend so older Windows keeps its original
connectivity coverage. Unload-sensitive consumers can avoid that COM path
by disabling network detection before SDK initialization.

Exercise the actual disabled configuration through network information
creation, seven-live-thread DLL unload and five-cycle reload. Keep a strict
zero-leak CI gate for modern and disabled detection without pretending
the restored COM backend or broader SDK is leak-free.

Describe actual GitHub Actions coverage rather than treating cross-builds
as runtime testing on every supported target OS.

Files changed:
- README.md
- .github/workflows/memory-leak-analysis.yml
- docs/building-custom-SKU.md
- 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/README.md
- tests/dll-unload/debug-listener-unload-module.cpp
- tests/dll-unload/debug-listener-unload-test.cpp
- tests/dll-unload/network-detector-reload-test.cpp

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