Skip to content

Adopt wil::unique_* RAII for Win32 handles + BSTR/CoTaskMem#39

Open
asklar wants to merge 4 commits into
mainfrom
refactor/wil-resource-handles
Open

Adopt wil::unique_* RAII for Win32 handles + BSTR/CoTaskMem#39
asklar wants to merge 4 commits into
mainfrom
refactor/wil-resource-handles

Conversation

@asklar

@asklar asklar commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Fixes #37

Summary

  • Replaced manual Win32 cleanup with wil::unique_* RAII across core providers, plugin loader, plugins, TAP DLLs, screenshot GDI, and integration fixtures.
  • Converted BSTR/CoTaskMem cleanup in src\tap\lvt_tap.cpp to wil::unique_bstr and wil::unique_cotaskmem.
  • Added WIL linkage for lvt_avalonia_plugin, lvt_avalonia_tap, lvt_chromium_plugin, and lvt_chromium_host.

Converted sites

  • Added wrappers: unique_handle 28, unique_event 16, unique_hfile 12, unique_hmodule 8, unique_hlocal 6, unique_hfind 4, unique_bstr 8, unique_cotaskmem 2, plus unique_hdc/GDI wrappers, unique_hkey, and unique_hwnd.
  • Removed 139 manual cleanup calls (CloseHandle, FreeLibrary, DeleteObject, DeleteDC, RegCloseKey, FindClose, SysFreeString, CoTaskMemFree, LocalFree, DestroyWindow).

Deliberately left raw

  • tests\integration_tests.cpp window-proc DestroyWindow(hwnd): the HWND is destroyed on its owning UI thread in response to a close message, not a scope-owned resource.
  • TAP FreeLibraryAndExitThread calls remain raw by design because they atomically unload the injected DLL and exit the worker thread.
  • GetStdHandle and GetModuleHandle* results remain raw/non-owning borrowed handles.

Validation

  • cmake --preset default passed.
  • dotnet build src\tap_wpf\LvtWpfTap.csproj -c Release passed.
  • dotnet build src\plugin_avalonia\LvtAvaloniaTreeWalker\LvtAvaloniaTreeWalker.csproj -c Release passed.
  • dotnet build src\tap_winforms\LvtWinFormsTap.csproj -c Release passed.
  • cmake --build build passed (existing Avalonia package vulnerability warnings; WinUI3 projection macro warning).
  • build\lvt_unit_tests.exe: 60/60 passed.
  • build\lvt_chromium_tests.exe: 18/18 passed.
  • build\lvt_integration_tests.exe: 31 passed, 1 skipped (NotepadFixture.XamlBoundsIfDetected, XAML/UWP not detected for this Notepad instance).
  • Targeted rerun after test-fixture HWND cleanup: KnownWindowFixture.*:ComCtlWindowFixture.* 5/5 passed.
  • Smokes: Notepad --frameworks returned win32 + winui3 2.2.0.0; screenshot smoke wrote a 47,748-byte PNG and was cleaned up; WinUI3 sample XML smoke wrote 36,560 bytes.

asklar and others added 4 commits July 9, 2026 02:30
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The three TAP CLR-hosting paths (WPF/WinForms/Avalonia TryNetCore) load
hostfxr.dll only as a fallback when it isn't already resident. The original
code loaded it and intentionally left it loaded for the process lifetime,
since it pins the hosted CoreCLR resolver in a foreign, injected process.

The WIL refactor wrapped that fallback load in a scope-owned wil::unique_hmodule,
which would FreeLibrary(hostfxr) on TryNetCore return -- a behavior change that
could unload a host library out from under the still-running runtime. (In the
common case hostfxr is already loaded, so the wrapper was empty and this only
bit the load-fallback path.)

Restore the exact original behavior: load hostfxr as a raw, intentionally
resident HMODULE with a comment explaining why it must not be scope-freed.
mscoree in the .NET Framework path is unaffected -- it is a shim over the app's
own already-loaded CLR and was already freed by the original, so unique_hmodule
there is a safe improvement.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@asklar

asklar commented Jul 9, 2026

Copy link
Copy Markdown
Owner Author

Review follow-up: kept hostfxr resident in the TAP CLR hosts (pushed f786a64)

Reviewed the full diff (screenshot GDI, all pipe/event handles, process-injection, plugin loader, tap natives). The handle mapping is correct throughout — notably unique_hfile (INVALID_HANDLE_VALUE sentinel) for pipes/files vs unique_event/unique_handle (nullptr sentinel) for events/threads/processes, unique_hlocal for the security descriptors, and remoteMem correctly left as manual VirtualFreeEx. GDI de-selects originals before RAII deletes, and the alpha fix is preserved.

One behavior change fixed (commit f786a64): the three TryNetCore CLR-hosting paths (WPF/WinForms/Avalonia) had wrapped the hostfxr.dll fallback load in a scope-owned wil::unique_hmodule. The original loaded hostfxr as a raw HMODULE and intentionally left it resident for the process lifetime — it pins the hosted CoreCLR resolver in a foreign, injected process. Scope-freeing it on TryNetCore return could unload a host library out from under the still-running runtime. (In the common case hostfxr is already loaded → the wrapper was empty, so this only affected the load-fallback path — but it's a real divergence for a "no behavior change" PR.) Restored the exact original behavior + added an explanatory comment so it isn't "re-cleaned" later. mscoree in the .NET Framework path is unaffected (shim over the app's own CLR; original already freed it).

Validation after the fix: cmake --build build green, lvt_unit_tests.exe 60/60.

@asklar

asklar commented Jul 10, 2026

Copy link
Copy Markdown
Owner Author

cdb validation of the full handle refactor (139 sites) — clean

Ran lvt.exe under cdb (which raises 0xC0000008 STATUS_INVALID_HANDLE on any double-close/invalid-handle, and 0xC0000005 on use-after-free) across all three managed-framework samples, with injection-stress (3 injections each) + break-on-invalid-handle (sxe ch) + break-on-AV (sxe av):

Framework Injections Access violations Invalid-handle (0xC0000008) Target survived Enriched
WPF (net10) 3 0 0
WinForms 3 0 0
Avalonia 3 0 0
WPF screenshot (GDI path) 1 0 0 valid PNG

This exercises both sides of the refactor: lvt.exe's inject/pipe/screenshot/plugin-loader handle code and the injected TAP DLLs' handle code (a target-side double-close would crash the target — none did). Enrichment succeeding proves the handle code ran the full inject→pipe→read→graft path, not just early-outs.

Combined with: build green, 60/60 unit, CI green, and the hostfxr fix being a proven revert to main's long-shipped behavior.

Caveat (unchanged): the rare hostfxr load-fallback branch still isn't force-exercised (a running .NET app has hostfxr resident), but everything around it is now debugger-verified and the fix itself is behavior-identical to shipped main.

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.

Adopt wil/resource.h (wil::unique_*) for Win32 handles across the codebase

1 participant