Skip to content

Windows Update Control Panel Restorer - #5052

Open
babamohammed2022 wants to merge 16 commits into
ramensoftware:mainfrom
babamohammed2022:patch-24
Open

Windows Update Control Panel Restorer#5052
babamohammed2022 wants to merge 16 commits into
ramensoftware:mainfrom
babamohammed2022:patch-24

Conversation

@babamohammed2022

@babamohammed2022 babamohammed2022 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

This is a Windhawk mod that restores the classic Windows 7/8.1 Windows Update Control Panel interface on Windows 10 and 11 systems by downloading the Windows 8.1 file from the Microsoft servers to make it work properly. The mod recreates the familiar colored status banner with real-time update information (up to date, updates available, pending restart) using the modern WUA (Windows Update Agent) backend. It includes multi-language support, classic task links, and friendly error messages when the Windows Update service is unavailable. To make the mod better, please point out any possible improvements for accuracy and functionality to make it more robust and functional.To make the mod better, please point out any possible improvements for accuracy and functionality to make it more robust and functional.

Changelog

If this pull request updates an existing mod, describe the changes below:

  • Changelog item 1...
  • Changelog item 2...

Mod authorship

If this pull request introduces a new mod, please complete the section below.

This mod was created by:

    • The submitter, without AI assistance
    • The submitter, with AI assistance
    • Claude
    • ChatGPT
    • Gemini
    • Another AI (please specify):
    • Other (please specify):

Please select the options that best apply. Your selection does not affect the acceptance criteria, but it helps reviewers understand the context of the code and provide relevant feedback.

@windhawk-reviewer windhawk-reviewer Bot added the waiting-for-author The author's turn: request an AI review, or respond to one that was posted. label Aug 9, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Thanks for the pull request! This repository uses a two-stage review: an AI review that you run yourself, followed by a human review.

To get started, comment /ai-review. Once you're happy with the result, comment /ready-for-reviewer to hand it over to a human reviewer.

See the pull request review process for the full details.

@babamohammed2022

Copy link
Copy Markdown
Contributor Author

/ai-review
The mod generates a new sidebar instead of properly hooking into the existing Control Panel navigation pane (the code has been commented out because it is a proof of concept at the moment). I need help figuring out how to correctly integrate with the native panel instead of creating an additional one.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 9, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


There are a couple of blocking issues here — one architectural (the runtime download), one that looks like unfinished work — plus a few concrete stability bugs.

1. The mod downloads and executes a DLL from a Microsoft server at runtime — this breaks Windhawk's self-contained rule. SetupWorkerEnsurePayloadDownloadWithTimeout fetches wucltux.dll from msdl.microsoft.com on first run, and SetupWorker then loads that downloaded binary as executable code into explorer.exe (LoadLibraryExW(path, 0), line 5047). A Windhawk mod must be entirely self-contained: it may not depend on, download, or contact external servers, and it must work offline. The README even lists "Internet access on first run" as a requirement, which confirms the dependency. SHA-256 verification does not change this — running a downloaded PE from a remote host is exactly what the policy forbids. This is the core design problem and there's no small fix: the mod needs to work without ever fetching a remote file. (Separately, embedding Microsoft's localized wucltux.dll.mui string table verbatim and redistributing/downloading Microsoft's DLL raises a redistribution/copyright question worth thinking about too.)

2. The submission looks unfinished — there's a "HELP NEEDED" block addressed to the reviewer, and the advertised task-links feature is actually disabled. PatchWuMainPageNavPane (lines 4013-4107) contains a large // REVIEWER COMMENT: … // REQUEST FOR HELP: … // NEED ASSISTANCE WITH: comment and unconditionally hides the entire navigation pane (visible="false" width="0rp"). Meanwhile the README advertises "Classic left task links" and there's a ShowClassicTaskLinks setting (default on) — but g_showClassicTaskLinks is only ever stored (line 3548), never read in the render path, so the setting does nothing and the feature it describes is off. Please finish or remove this: either implement the working sidebar or drop the feature, the ShowClassicTaskLinks setting, the commented-out BuildWuNavPaneLinksXml/SidebarExtraString, and the README section — and remove the reviewer-directed comments. A PR is not the place to ask the reviewer to write the feature; if you need help, the Windhawk Discord / GitHub Discussions is the right venue.

3. The Ctrl+P hotkey is a system-wide hotkey and will hijack Print in every application. RegisterHotKey(hwnd, kToggleAvailHotkeyId, MOD_CONTROL, 'P') (line 671) registers a global hotkey: while the listener is active (i.e. whenever ShowAvailableUpdates is enabled), the system routes Ctrl+P to the mod's hidden window and does not deliver it to the focused app, so Ctrl+P (Print) stops working across the whole session. A global hotkey for a niche preview-banner toggle is hard to justify, and Ctrl+P specifically is one of the most common shortcuts. Recommend dropping the hotkey entirely and driving the banner from the setting alone; if you keep a runtime toggle, don't bind a ubiquitous shortcut.

4. The hotkey window class is registered but never unregistered — it crashes on the next mod load. StartHotkeyListener calls RegisterClassW (line 660) but nothing ever calls UnregisterClass, and Wh_ModUninit doesn't either. A class registered by a DLL is not auto-removed when the DLL unloads; its lpfnWndProc (HotkeyWndProc) keeps pointing into the now-unmapped mod image. On the next enable/update, RegisterClassW fails with ERROR_CLASS_ALREADY_EXISTS — which the code deliberately treats as success (&& GetLastError() != ERROR_CLASS_ALREADY_EXISTS) — and then CreateWindowExW builds a window on the stale class, dispatching messages to a dangling WndProc → crash / arbitrary-code execution. Fix: register with the mod's own module handle (not GetModuleHandleW(nullptr), which is the host .exe) and UnregisterClass(kHotkeyClassName, hInstance) in Wh_ModUninit; don't swallow ERROR_CLASS_ALREADY_EXISTS.

5. The private wucltux.dll module is leaked into explorer.exe on every reload. SetupWorker loads the DLL into the process on each init, and Wh_ModUninit intentionally never FreeLibrarys it (just g_module.store(nullptr), line 5209). Because a disable/enable leaves the process running, every cycle leaks another mapping of the module (plus the GDI+/MUI datafile handles) into the live explorer.exe. I understand the stated reason (a cached DirectUI page may still hold a reference), so there may be no perfectly clean fix within this design — but as written it accumulates across reloads and is worth calling out, because it's a direct consequence of the download-and-load-a-module architecture in point 1.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • Dead code. Remove the commented-out BuildWuNavPaneLinksXml (lines 3959-4004) and SidebarExtraString (3922-3939), and the unused helpers SearchAvailableUpdates (279), FormatFileSize (486), and FindRootElementEnd (3474) — all currently kept alive only with [[maybe_unused]]. If you keep the update-history feature, note ShowUpdateHistory pops a plain MessageBox, which doesn't match the restored classic look.
  • Unused @compilerOptions libraries. -lcrypt32 is not needed — the hashing uses CryptAcquireContextW/CryptCreateHash/CryptHashData (the CryptoAPI base provider, which lives in advapi32.dll), not crypt32.dll. -lgdi32 also appears unused — there are no direct GDI calls, and GDI+ is resolved dynamically via LoadLibraryExW/GetProcAddress. Drop both.
  • WindhawkUtils::StringSetting. LoadLanguageSetting does a raw Wh_GetStringSetting + manual Wh_FreeStringSetting (lines 3535-3537); the RAII WindhawkUtils::StringSetting is the idiomatic form.

Functionality notes

Non-critical observations about the feature behavior itself.

  • @architecture x86-64 vs. an amd64-only payload. The download is verified specifically as x64 (IMAGE_FILE_MACHINE_AMD64, line 1123) and the pinned SHA is for the x64 binary. With x86-64, on ARM64 devices the mod runs in the native ARM64 explorer.exe, where loading an x64 wucltux.dll will fail — so the feature silently won't appear there. If ARM64 is out of scope anyway, amd64 communicates that more honestly. (This is moot if point 1 is resolved.)
  • Registry hooks on a hot path. RegOpenKeyExWHook/RegQueryValueExWHook/RegEnumKeyExWHook/RegGetValueWHook run for every registry call in explorer.exe, each doing a lock + string-build + ToLower/EndsWith classification. This is the same in-memory CPL-registration technique your win7-legacy-applet-restorer uses, so it's an accepted pattern, but it's a real per-call cost worth being aware of given how registry-heavy Explorer is.
  • "Most recent check" is the mod's query time, not Windows' own scan timestamp — already documented in the README's known-limitations, just noting it's a visible inaccuracy.
  • Overlap with your own mods. This shares the CPL-injection machinery (KeyTracker, Reg* virtualization, CoCreateInstance element-provider routing) with your win7-legacy-applet-restorer. Not a duplicate — different applet — but if the two could share one code path or one mod with options, that'd be easier to maintain than two near-identical registration engines.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 9, 2026
@babamohammed2022 babamohammed2022 changed the title Publish the first version of the mod Windows Update Control Panel Restorer Aug 9, 2026
@babamohammed2022

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 9, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


Thanks for the update — the hotkey, the unregistered window class and the unfinished nav-pane placeholder from the previous round are all resolved, and the sidebar is now implemented. The runtime download is still the blocking issue, and the new code introduced a few more problems.

1. The mod still downloads and executes a DLL from a Microsoft server at runtime. kDownloadUrl (line 536) points at msdl.microsoft.com, DownloadWithTimeout (line 1054) fetches it, and SetupWorker (line 5683) loads that downloaded binary as executable code into explorer.exe (LoadLibraryExW(path, nullptr, 0)). This is unchanged from the previous review. A Windhawk mod must be entirely self-contained: it may not depend on, download, or contact external servers or resources, and it must work offline. SHA-256 verification doesn't change that — running a PE fetched from a remote host is exactly what the rule forbids. The README now states it as a feature ("The Windows 8.1 UI DLL is downloaded from Microsoft Symbol Server and verified before use"), which confirms the dependency is still there.

There's no small fix; the mod needs an architecture that never fetches a remote file. The rest of the machinery (in-memory CPL registration, embedded MUI table, DirectUI XML patching) doesn't depend on the download — what does is the wucltux.dll DirectUI page implementation and the WUAppElementProvider COM class it hosts. Options worth considering: build the page from XML/DirectUI the mod supplies itself (you already synthesize most of the visible content in PatchModernWuPageXml), or drop the legacy provider and render the applet with a mod-owned window. Note also that embedding Microsoft's wucltux.dll.mui string table verbatim (kWucltuxMuiStrings, lines 645-975) and redistributing/fetching Microsoft's DLL is a separate redistribution question that needs a clear answer regardless of how the download is resolved.

2. Page rendering does blocking service + COM work on the Control Panel UI thread. DUISetXMLHook (line 4750) runs on the window's own thread, and PatchModernWuPageXml calls, synchronously:

  • IsWindowsUpdateServiceAvailable() (line 4486) — OpenSCManagerW + OpenServiceW + QueryServiceConfigW, i.e. RPC to the SCM.
  • LastInstallTimeText() (line 4558) → GetUpdateHistory(200) (line 371) — CoCreateInstance(UpdateSession) plus IUpdateSearcher::GetTotalHistoryCount / QueryHistory, which reads the Windows Update datastore through wuauserv.

QueryHistory routinely takes seconds when the service is cold or the datastore is large, and it can block much longer on a machine where Windows Update is unhealthy — which is precisely the audience this mod targets. During that time the Control Panel window is frozen with no repaint. Move the status gathering off the UI thread: collect it on a worker (or on the existing setup thread), cache it in globals, and have the render path read only the cached values — then refresh the page asynchronously when new data arrives. The 5-second cache on IsWindowsUpdateServiceAvailable doesn't help here, because the first call on a fresh page render always pays the full cost.

3. Debug leftovers are shipped and active. DumpSettingsXmlIfNeeded (line 4734) — whose own comment says "Remove in production" — writes the raw settings-page XML to a file in the mod storage folder every time the settings page is rendered, and it's called unconditionally from both DUISetXMLHook (line 4758) and DUISetXMLFromResourceHook (line 4789). Please remove the function and both call sites. In the same vein, DebugForcePendingUpdate is a debug/preview switch exposed as a normal user setting; if you want to keep it for your own testing, gate it behind Wh_Log/a code constant rather than shipping it in the settings UI.

4. The LoadImageW hook doesn't check which module the icon is being loaded from. LoadImageWHookForLegacyWarningIcon (line 3688) ignores its instance parameter entirely and substitutes the mod's icons for any IMAGE_ICON request whose resource ID is 61002-61005, from any module, anywhere in explorer.exe. Your own XML asks for library(shell32.dll) (lines 4553, 4686), so the intent is clearly "only when the request came from our own XML" — but as written any other component that happens to use one of those four IDs gets the wrong icon. Restrict it, e.g.:

if (type == IMAGE_ICON && IS_INTRESOURCE(name) &&
    instance == GetModuleHandleW(L"shell32.dll")) {

(and pick IDs that shell32 genuinely doesn't define, or better, use a distinctive range).

5. The ShellExecuteW hook and everything behind it is unreachable dead code. ShellExecuteWHook (line 4891) exists only to intercept wuamodern:<action> parameter strings, but nothing in the mod emits such a string any more — all the generated NavigateButtons use shell:::{CLSID} or ms-settings: (lines 3950, 4028, 4410, 4426, 4600, 4628). Grepping the file, wuamodern: appears only in IsModernWuAction itself (line 4864). So the mod installs a process-wide hook on ShellExecuteW in explorer.exe — with a std::wstring copy and towlower pass on every call — that can never do anything. Along with it, HandleModernWuAction, ShowUpdateHistory, OpenInstalledUpdates, DoCheckForUpdatesInPage/ReRenderWuPage, and the g_wuParser/g_wuResModule/g_wuHInstance/g_wuBaseXml caching in DUISetXMLHook (which copies the entire page XML on every render) are all dead. Either wire the actions back up or delete the whole path including InstallModernWuHooks.

6. Wh_ModUninit can block for ~20 seconds. Wh_ModUninit (line 5833) joins g_setupThread, which may be sitting inside InternetOpenUrlW (line 1062). The read loop checks g_stopping, but the InternetOpenUrlW call itself isn't cancellable — it's bounded only by the 20 s connect/send/receive timeouts, and there's no InternetCloseHandle from the stop path. So disabling the mod on a machine with a slow/blocked connection stalls for the full timeout. This goes away with item 1; if any network-free long operation remains on that thread, make sure every blocking call in it is either cancellable or short.

7. Leaks that accumulate across enable/disable cycles.

  • g_legacyWarningShield (line 3532) and g_wuDisabledShieldIcon (line 3679) are never destroyed — Wh_ModUninit only cleans up g_updatesInstalledIcon and g_windows81UpdateStatusIcon (lines 5848-5849). Add matching DestroyIcon calls.
  • The private wucltux.dll (g_module) and every generated .mres resource module are deliberately never FreeLibrary'd (lines 5701-5710, 5840-5841), so each enable cycle adds another mapping to the live explorer.exe, and each language change adds one more .mres mapping on top. I understand the stated reason (a cached DirectUI page can still hold a reference), so there may be no fully clean fix inside this design — but it's a direct consequence of the load-a-foreign-module architecture and worth reconsidering together with item 1.

8. EnsureControlPanelTasksXmlFile writes a file from inside a registry hook, unsynchronized. ProvideValue (line 5399) calls it on every RegQueryValueExW/RegGetValueW for System.Software.TasksFileUrl — that is, a CreateFileW(CREATE_ALWAYS) + WriteFile on an arbitrary caller's thread inside a hooked registry read. Two Explorer threads (each Explorer window has its own) hitting it concurrently will collide: the file is opened with FILE_SHARE_READ only, so the second CreateFileW fails and ProvideValue returns false, silently dropping the task links for that query. Write the file once (from the setup thread, or lazily under a mutex like EnsureAppletLogoIconFile does with g_appletLogoIconMutex, line 5255) and just return the cached path from the hook.

9. ShowAvailableUpdates defaults to true but its description says it's off — and the underlying check is nearly always true. The settings block declares ShowAvailableUpdates: true (line 40) while its own $description says "Disabled by default" and the code comment at g_showAvailableUpdates (line 595) says "default off". Worse, IsUpdatesAvailable() (line 4245) reports "updates available" whenever HKLM\...\WindowsUpdate\Auto Update\Results\Download or ...\Results\Install merely exists — those keys are present on essentially every Windows installation regardless of whether anything is actually staged, so the amber "There are updates available" banner will be shown permanently on most machines out of the box. Please fix the default to match the documented intent, and base the check on real state (e.g. a LastSuccessTime/LastError value inside those keys, or a WUA search result) rather than key existence.

10. The README has no screenshot. This mod recreates a visible Control Panel page with two selectable skins and a colored status banner — a screenshot (or two, one per skin) makes a big difference in the mod catalog. Only i.imgur.com and raw.githubusercontent.com are allowed image hosts; your win7-legacy-applet-restorer README already does this correctly.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • Data race on g_language across a settings change. g_language (line 611) is a plain std::wstring written by LoadLanguageSetting() (line 3821) on the settings-changed thread, while EmbeddedMuiString() (line 1393) reads it from LoadStringWHook and SHLoadIndirectStringHook on arbitrary threads. Concurrent read/write of a std::wstring is UB (the buffer can be reallocated mid-read). Since it only ever holds a short code, a std::atomic<int> language index — or a small enum — would sidestep it entirely and simplify the if (g_language == L"it") … chains at the same time.
  • Dead code still marked [[maybe_unused]]. SearchAvailableUpdates (line 254), FormatFileSize (line 461) and FindRootElementEnd (line 3761) are unreferenced. SearchAvailableUpdates also drags in the IsUpdateImportant helper (line 200), whose category match on L"Update" would classify basically every update as important.
  • Unused @compilerOptions libraries. -lcrypt32 isn't needed — the hashing uses CryptAcquireContextW/CryptCreateHash/CryptHashData, which are CryptoAPI base-provider functions in advapi32.dll, not crypt32.dll. -lgdi32 also looks unused: there are no direct GDI calls and GDI+ is resolved dynamically. The #pragma comment(lib, ...) lines (125-127) are MSVC-only and are no-ops under Clang/mingw — they can go too.
  • WindhawkUtils::StringSetting. LoadLanguageSetting (lines 3822-3835) does a raw Wh_GetStringSetting + manual Wh_FreeStringSetting; the RAII WindhawkUtils::StringSetting is the idiomatic form. Note Wh_GetStringSetting never returns NULL (it returns L""), so the if (lang) checks are redundant.
  • ReadWuaResultString buffer sizing. Line 4275 does out.resize(size / sizeof(wchar_t)) and then passes written = size bytes. If a malformed REG_SZ has an odd byte count, the integer division rounds down and the buffer is one byte short of what's passed to RegQueryValueExW. Round up: out.resize((size + sizeof(wchar_t) - 1) / sizeof(wchar_t)).
  • IsWindowsUpdateServiceAvailable cold start. g_wuCheckedTick starts at 0, so for the first 5 seconds of uptime now - last < kWuCheckIntervalMs short-circuits and returns the uninitialized false without ever querying the SCM (line 4185). Initialize the tick to a sentinel, or use a separate "checked at least once" flag.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • @architecture x86-64 vs. an amd64-only payload. IsValidPayload requires IMAGE_FILE_MACHINE_AMD64 (line 1029) and the pinned SHA is the x64 binary. With x86-64, on ARM64 devices Windhawk runs the mod inside the native ARM64 explorer.exe, where loading an x64 wucltux.dll cannot succeed — the applet silently won't appear. If ARM64 is out of scope, amd64 states that honestly. (Moot if item 1 is resolved by dropping the foreign module.)
  • Shell hot-path hooks. Beyond the registry virtualization (which your win7-legacy-applet-restorer already established as an accepted pattern), this mod adds process-wide hooks on SHLoadIndirectString, ExtractIconExW, PrivateExtractIconsW, SHDefExtractIconW and LoadImageW — all of which run for essentially every string and icon Explorer resolves, and each does a std::wstring copy plus a towlower pass in IsWucltuxPathString (line 4912). A cheap pre-check before building the string (e.g. bail out immediately when the path has no L'\\'-free wucltux prefix, or compare with StrStrIW on the raw pointer) would keep the common case allocation-free.
  • "Most recent check" is the mod's own query time, not Windows' recorded scan timestamp (LastCheckForUpdatesText, line 4333) — documented in the README, just noting it's a visible inaccuracy on the restored page, and one that resets whenever the mod reloads.
  • IsWindows10() build gate. Line 4175 treats builds >= 10000 && < 22000 as Windows 10, which is used to decide whether to show the "View update history" link. Windows 11 Insider builds and any future build numbering will fall on the Windows 11 side, which is probably what you want — just be aware the >= 10000 lower bound also excludes the Windows 10 pre-release builds, if that matters to you.
  • Overlap with your own mods. This shares the whole CPL-injection engine (KeyTracker, Reg* virtualization, CoCreateInstance element-provider routing) with win7-legacy-applet-restorer. Not a duplicate — different applet — but two near-identical registration engines will be painful to keep in sync; folding this in as an option of the existing mod, or factoring the shared part, is worth weighing.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 9, 2026
@babamohammed2022

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 9, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


The blocking architectural issue from the previous two rounds is unchanged, and the new background-worker layer added since the last review brought its own problems (unload hang, races on shared caches, a registry-tracking regression).

1. The mod still downloads and executes a DLL from a Microsoft server at runtime. kDownloadUrl (line 549) points at msdl.microsoft.com, DownloadWithTimeout (line 1073) fetches it, and SetupWorker loads that downloaded binary as executable code into explorer.exe (LoadLibraryExW(path.c_str(), nullptr, 0), line 5859). This is the third review in a row raising it. A Windhawk mod must be entirely self-contained: it may not depend on, download, or contact external servers or resources, and it must work offline. SHA-256 pinning does not change that — running a PE fetched from a remote host is exactly what the rule forbids, and the README now argues the point explicitly rather than removing the dependency ("Why the mod downloads a DLL", lines 84-93).

There is no small fix; the mod needs an architecture that never fetches a remote file. What actually depends on the payload is the wucltux.dll DirectUI page implementation and the WUAppElementProvider COM class it hosts — the rest (in-memory CPL registration, embedded MUI table, DirectUI XML patching) does not. Options worth considering: build the page from XML/DirectUI the mod supplies itself (PatchModernWuPageXml already synthesizes most of the visible content), or drop the legacy provider and render the applet with a mod-owned window.

Two things that follow from the same design and need an answer regardless:

  • Redistribution/copyright: embedding Microsoft's wucltux.dll.mui string table verbatim (kWucltuxMuiStrings, lines 662-995) and fetching/loading Microsoft's DLL.
  • The verified copy lives in the mod storage directory, and IsValidPayload runs at EnsurePayload time while LoadLibraryExW happens afterwards — anything that can write to that directory can swap the file in the gap and get code execution in explorer.exe.

2. The README is unfinished and has no screenshots. Lines 65-68 ship a note to yourself rather than content:

**Screenshots**

Add one screenshot per skin here before submitting (only `i.imgur.com` and
`raw.githubusercontent.com` hosts are allowed).

This mod is entirely visual, so please add a real screenshot per skin (and remove the placeholder). Your merged win7-legacy-applet-restorer does this correctly.

3. Wh_ModUninit can block for over a minute, hanging the disable/update. Wh_ModUninit joins g_setupThread (line 6012), but SetupWorker has two long stretches it can be sitting in:

  • EnsurePayload → up to 3 attempts × a 20 s kDownloadTimeoutMs plus 2 × 3 s retry waits. The g_stopping check in DownloadWithTimeout only runs between transfers — a blocked InternetQueryDataAvailable/InternetReadFile still runs to its 20 s timeout.
  • GatherBackgroundStatusComputeLastInstallTimeGetUpdateHistory(200), which has no stop check at all and is documented in your own comment (line 4572) as being able to "block for seconds on a cold/unhealthy Windows Update datastore".

So toggling the mod off while it is starting up freezes Windhawk. Please make the wait bounded: close the WinINet handles from Wh_ModUninit to abort the transfer immediately (keep HINTERNET internet/url in globals and InternetCloseHandle them on stop), and check g_stopping before/around the history query so it is skipped once teardown starts.

4. Page rendering can still block the Control Panel UI thread for seconds. The caching added this round doesn't cover the cold path. LastInstallTimeText() (line 4596) is called from PatchModernWuPageXml on the DirectUI thread, and when the setup thread hasn't finished it runs the full ComputeLastInstallTime() inline — while holding g_statusMutex. Worse, if the setup thread is already inside GatherBackgroundStatus (line 4618 takes the same mutex around the same call), the UI thread simply blocks on the mutex for the whole duration of that WUA query. Same shape for IsWindowsUpdateServiceAvailable() (line 4409): when g_cachedWuServiceProbed is still false it does the SCM RPC inline on the UI thread.

The render path should never do either. Return an empty string / "up to date"-neutral state when the value isn't cached yet, and let the next page render pick it up:

static std::wstring LastInstallTimeText() {
    std::lock_guard<std::mutex> lock(g_statusMutex);
    return g_lastInstallComputed ? g_cachedLastInstall : std::wstring();
}

5. Shared caches are read/written from several Explorer threads with no synchronization. Each Explorer/Control Panel window runs on its own thread, so two windows rendering the page (or two icon requests) genuinely race:

  • g_lastQueryTimeText (line 636) — LastCheckForUpdatesText() does an unguarded if (empty) assign on a global std::wstring. Concurrent read + reallocating write on a std::wstring is a use-after-free in explorer.exe.
  • InitGdiPlusRendering() (line 3793) — g_hGdiPlus / g_gdiplusToken / the ten function pointers are set without a lock, and LoadImageWHookForLegacyWarningIcon is a process-wide hook that can enter it from any thread. Two threads racing here means two GdiplusStartup calls, one leaked token and one leaked module reference that ShutdownGdiPlusRendering never releases.
  • g_legacyWarningShield, g_updatesInstalledIcon, g_windows81UpdateStatusIcon, g_wuDisabledShieldIcon — same check-then-store pattern, so two threads can each create an icon and one is leaked.

A std::mutex (or std::call_once for the GDI+ init) around each of these is enough.

6. KeyTracker::Track now records every registry key opened in explorer.exe. OpenVirtual (line 5648) calls g_keys.Track(*out, full) for every successful RegOpenKeyExW/RegOpenKeyW, so every registry open in the process takes the tracker's exclusive lock and stores a full path string, and every subsequent RegQueryValueExW/RegGetValueW on it copies that string out and runs Classify() (a ToLower allocation plus nine EndsWith comparisons). That is a process-wide cost on one of the hottest APIs in the shell. Your own merged mod filters this out at exactly that point — win7-legacy-applet-restorer.wh.cpp#L223:

void Track(HKEY hKey, const std::wstring& path) {
    if (!hKey || IsSpecialRoot(hKey)) return;
    if (!ContainsRelevantKeywordInsensitive(path)) return;   // <-- missing here
    ...
}

Two related regressions against that same mod, both inside code that runs on foreign shell threads:

  • CreateFake uses std::make_unique<int>(1) (line 5279), which throws on OOM, and OpenVirtual/RegOpenKeyExWHook have no try/catch — a std::bad_alloc from there (or from the map insert in Track) unwinds out of RegOpenKeyExW straight into shell32. The merged mod uses new (std::nothrow) (#L239) and wraps Track in try/catch (#L1157).
  • IsFake() and Path() take the shared_mutex separately, so callers that need both can observe two different snapshots; the merged mod added IsFakeAndGetPath specifically to avoid that.

7. The DebugForcePendingUpdate setting has no effect. It's declared in the settings block (line 46) and shown to users, but it is only read under if constexpr (kWuDebugForcePendingEnabled) (line 4068) and that constant is hard-coded false (line 622). Shipping a user-visible setting that provably does nothing is confusing — please remove the setting from the settings block and keep only the compile-time constant (or drop the debug switch entirely).

8. @architecture x86-64 doesn't match an AMD64-only payload. IsValidPayload requires IMAGE_FILE_MACHINE_AMD64 (line 1048), and the pinned SHA-256 is the x64 build. On an ARM64 device x86-64 still applies to the native ARM64 explorer.exe, where LoadLibraryExW of an AMD64 DLL fails with ERROR_BAD_EXE_FORMAT — so the mod loads, hooks the registry APIs process-wide, and then silently does nothing. Use @architecture amd64 so it is not applied on ARM64 at all.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • Dead code. The #if 0 block at lines 4641-4730 (~90 lines) and the five [[maybe_unused]] functions — SearchAvailableUpdates (267), OpenInstalledUpdates (461), FormatFileSize (474), ShowUpdateHistory (497, a debug MessageBox), FindRootElementEnd (3982). Removing SearchAvailableUpdates also retires GetKbList, GetMsrcSeverity, IsUpdateImportant, WuaUpdateInfo and WuaSearchResult. "Kept for future use" is what git history is for; the reference implementation is preserved in the PR either way.
  • Unused dependencies. -lgdi32 (no GDI call in the file; GDI+ is resolved with GetProcAddress) and -lcrypt32 (the Crypt* hash functions used are advapi32 exports). #include <windowsx.h> is there for GET_Y_LPARAM (line 105) which is never used; <comdef.h> is unused; <shellapi.h>/<shlobj.h> become unused once the dead code above goes.
  • WindhawkUtils::StringSetting. LoadLanguageSetting (lines 4043-4056) does raw Wh_GetStringSetting + manual Wh_FreeStringSetting; the RAII wrapper is simpler. Also, Wh_GetStringSetting never returns NULL (it returns L""), so the lang && / skin && checks can be dropped.
  • g_language is written on a settings change while other threads read it. LoadLanguageSetting() assigns the global std::wstring from Wh_ModSettingsChanged (arbitrary thread) while EmbeddedMuiString, InfoTipForLanguage (called from the registry hooks) and the rebuild thread read it — a reallocating write concurrent with a read is a crash, not just a stale value. Only reachable via a settings change, hence optional, but an std::shared_mutex or a small enum/atomic language id would close it.
  • g_builtLanguage (line 592) is assigned twice and never read. Dead state.
  • StoreDir() puts a 64 KB buffer on the stack (wchar_t path[32768], line 1063) and is called from the registry hooks on arbitrary shell threads. A heap buffer or a std::wstring sized from the return value would be safer, and the result could be computed once and cached.
  • ComGuard's comment contradicts its code. The comment (lines 249-251) says CoUninitialize must not be called on S_FALSE; S_FALSE does require a balancing CoUninitialize, and the code correctly does so via SUCCEEDED(hr). Just the comment is wrong.
  • The comment at line 607 says ShowAvailableUpdates defaults to off; the settings block defaults it to true.
  • Each enable/disable cycle leaks a mapped .mres image. EmbeddedMuiResourceModule() never calls FreeLibrary, and Wh_ModUninit doesn't either — small and bounded per cycle, but it accumulates across mod updates within one explorer.exe session.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • IsUpdatesAvailable() will report "updates available" on almost every machine. It returns true when Results\Download or Results\Install has a non-empty LastSuccessTime/Success/Result (lines 4482-4493). Results\Install\LastSuccessTime is written after every successful install, so it is present on essentially any machine that has ever updated — it means "an install succeeded at some point", not "updates are staged". With ShowAvailableUpdates on (the default), the amber banner will therefore be shown permanently whenever no reboot is pending. A real check needs a WUA search (IsInstalled=0 and IsHidden=0) on the background thread, or at minimum Auto Update\Results\Detect combined with a downloaded-updates indicator.
  • "Most recent check for updates" is fabricated. LastCheckForUpdatesText() (line 4566) returns the moment the page was first rendered, cached for the life of the mod — it is presented in the UI as Windows' own last scan time. The real value is available from HKLM\SOFTWARE\Microsoft\Windows\CurrentVersion\WindowsUpdate\Auto Update\Results\Detect\LastSuccessTime, or from IAutomaticUpdatesResults::get_LastSearchSuccessDate. Showing a wrong timestamp is worse than showing none — consider omitting the line when the real value can't be read.
  • All the classic links do the same thing. Every sidebar row (BuildWuSidebarLinkRow, lines 4256-4274) and every Control Panel task (AppendControlPanelTaskXml, line 5484) is wired to explorer.exe shell:::{36eef7db-…}, i.e. reopening this same page — while being labelled "Check for updates", "Change settings", "View update history" and "Restore hidden updates". Users will read those labels as working actions. If they can't be implemented, it'd be better to show only the ones that do something (e.g. route "View update history" to the same target you already use for the Windows 10 link, and "Change settings" to pageSettings) and drop the rest.
  • WUA history dates are shown in UTC. FormatWuaDate (line 4539) passes the DATE from IUpdateHistoryEntry::get_Date — which is UTC — straight to VariantTimeToSystemTime with no local-time conversion, so "Updates were installed" is off by the timezone offset when it comes from the WUA fallback path (the registry path at line 4523 does convert). SystemTimeToTzSpecificLocalTime after the conversion would fix it.
  • DisableMuiConfigInPrivateCopy depends on undocumented layout of one specific Microsoft binary (your comment at lines 1247-1252 acknowledges this). Worth keeping in mind as a fragility point if the payload approach survives at all.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 9, 2026
@babamohammed2022

babamohammed2022 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

The embedded MUI strings are necessary for UI localization as they are text translations, not executable code. The DLL itself is downloaded directly from Microsoft's official symbol server (msdl.microsoft.com) and verified by SHA-256. This approach is identical to the already-approved Windhawk mods like "Win10 taskbar on Win11 24H2." The mod does NOT redistribute the DLL but it uses it as a runtime dependency.

Updated README and settings for Windows Update Control Panel Restorer mod. Removed debug option and improved comments.
@babamohammed2022

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 10, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


First, a correction to the earlier rounds of this review: the runtime download from the Microsoft Symbol Server is no longer being treated as blocking. Your Performance Information and Tools Restorer was merged on 2026-08-10 with the same pinned-SHA-256 symbol-server payload design, so that architecture has been accepted for this family of mods and it would be inconsistent to keep blocking on it here. The remaining findings below are about this mod's own code. Note also that the fixed RVAs into the payload (kCElementWithSiteSetSiteRva, kControlPanelNavLinksVtableRva) are genuinely well-guarded by the SHA-256 pin — if Microsoft ever serves something different, verification fails and the mod degrades to the fallback notice instead of hooking arbitrary bytes. Item 3 is about the one path where that guarantee has a gap.

1. "Check for updates" can freeze an Explorer UI thread, and can stall mod unload for minutes. RunRealWindowsUpdateSearch uses the synchronous IUpdateSearcher::Search with put_Online(VARIANT_TRUE) (line 7460-7464), which is not cancellable and routinely runs for minutes on exactly the machines this mod targets (broken/slow Windows Update). Two consequences:

  • UI freeze. The check window gives up after kWuCheckMaxDurationMs (45 s, line 6618) and sets g_checkingForUpdates = false (line 7180) — and WM_DESTROY does the same if the user just closes the window (line 7204) — while the search thread is still running. The next click on "Check for updates" enters StartWuUpdateCheck on the Explorer UI thread (it is called from ShellExecuteExWHook/ShellExecuteWHook, which run on the thread of the Control Panel window that was clicked) and hits:

    std::lock_guard<std::mutex> lock(g_wuSearchThreadMutex);
    ...
    if (g_wuSearchThread && g_wuSearchThread->joinable()) g_wuSearchThread->join();   // line 7554

    That blocks the UI thread until the previous online search returns — an unbounded, no-repaint hang of that Control Panel window.

  • Unload stall. Wh_ModUninit joins the same thread (line 10399). The comment says it is "bounded by IUpdateSearcher's own timeouts", but those timeouts are minutes-scale, so disabling or updating the mod during a check hangs Windhawk for that long.

    Fix: switch to the asynchronous API — IUpdateSearcher::BeginSearch returns an ISearchJob with RequestAbort(), so teardown can actually cancel and the join becomes bounded. Regardless of that, never join() on the UI thread: if a search is already in flight, return early (an extra std::atomic<bool> g_searchInFlight set by the worker and cleared at its end) instead of blocking, and let the previous thread be joined by teardown or by the worker itself.

2. Two Control Panel windows can open two "Change settings" dialogs, and the second one paints with a deleted GDI brush. ShowWuSettingsDialog guards against a second instance with FindLiveWuSettingsDialog() (line 5920), but the dialog only registers itself later, in WM_INITDIALOG (line 5815) — so the check and the registration aren't atomic. Each Explorer window runs on its own thread, so two windows invoking the link near-simultaneously both pass the guard. The code then relies on the "only one at a time" assumption for plain, unsynchronized globals:

  • g_wuDlgDarkBrush (line 5786) is shared. The first dialog to close runs TeardownWuSettingsDialogDarkMode() on WM_DESTROYDeleteObject(g_wuDlgDarkBrush) (line 5808), while the second dialog keeps returning that same handle from WM_CTLCOLORDLG/WM_CTLCOLORLISTBOX/WM_CTLCOLORBTN/WM_CTLCOLORSTATIC (lines 5852-5895) — i.e. painting with a destroyed GDI object.
  • g_wuDlgAuOptions / g_wuDlgRecommended / g_wuDlgMsProducts / g_wuDlgAllUsers (lines 5636-5639) are written just before the dialog is created (lines 5925-5926), so the second thread can clobber them while the first dialog is still in WM_INITDIALOG reading them.

g_wuFaqParent (line 6177, set at line 6557 and read in WM_INITDIALOG) has the same shape.

Fix: make the "already open" check and the registration one atomic step under g_wuSettingsDlgMutex (e.g. reserve a slot before CreateDialogIndirectParamW and drop it if creation fails), and move the per-dialog state (AUOptions snapshot, dark-mode brush, parent HWND) into the dialog instance via CreateDialogIndirectParamW's dwInitParam + GWLP_USERDATA — you already do exactly this correctly for the header icons/fonts with WuHeaderDialogResources.

3. The payload is verified and then loaded as two separate steps, leaving a TOCTOU window in front of fixed-RVA hooking. EnsurePayload validates the cached file and returns its path (lines 1167-1172), and SetupWorkerImpl calls LoadLibraryExW(path.c_str(), nullptr, 0) afterwards (line 9989); EnsurePrivateModuleLoaded (line 9920) loads g_dllPath with no verification at all. The file lives in mod storage, which is user-writable, so between the SHA-256 check and the load the file can be replaced. That matters more here than in the Performance mod, because this mod then hooks a hardcoded offset inside the module (Wh_SetFunctionHook at module + 0x26960, line 5589-5592) and hands the shell a fabricated COM object whose vtable is module + 0x2350 (line 5368) — against a substituted binary that is arbitrary code execution in explorer.exe, not a graceful failure.

Fix: hold the file open across verify-and-load. Open it once with CreateFileW(..., FILE_SHARE_READ, ...) (no write/delete sharing), hash that handle, keep it open, and load the module while the handle is held — the perf mod's load-time file-identity confirmation is the same idea. At minimum, re-verify in EnsurePrivateModuleLoaded rather than trusting g_dllPath blindly.

4. Two resource accumulations that are never reclaimed.

  • Per page navigation: CreateNativeControlPanelNavLinks (line 5360) allocates a fresh object plus seven links (0x70 bytes each) and their strings on every navigation, with referenceCount = kPinnedNavLinksReferenceCount (0x10000000, line 5279) so the shell's Release never destroys it, and nothing frees it at unload either — roughly 2 KB per visit to the page, permanently, for the life of explorer.exe. I understand why the refcount is pinned (the object is CoTaskMemAlloc'd by the mod but its vtable/destructor belong to wucltux, so letting it self-destruct would cross allocators) and why the previous caching attempt was reverted — but as written the growth is unbounded. Note this only fires on the path where wucltux doesn't publish its own list (the Windows 10 21H2 case); on Windows 11 PublishNativeNavigationLinks returns S_FALSE early. A tracked list of the objects the mod allocated, freed in Wh_ModUninit, would at least bound it — and by then g_registrationReady is already false.
  • Per mod reload / language change: the private wucltux.dll and every generated .mres module are deliberately never FreeLibrary'd (lines 1802-1808, 10405-10406), so each enable/disable cycle and each language change adds another permanent mapping to the live explorer.exe, and the on-disk .mres can't be deleted while mapped (CleanupGeneratedResourceModuleFiles counts it as locked). This was raised in the earlier rounds and is unchanged. The stale-file sweep for dead PIDs does bound the disk side across restarts, so this is mostly about address space in a long-lived Explorer.

If you conclude there's genuinely no safe way to reclaim either of these inside this design, say so in the PR and it can be recorded as a known limitation instead — but it shouldn't stay silent.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • The result window recomputes work that's already cached. WuCheckResultDlgProc's WM_INITDIALOG calls ComputeLastInstallDateFromUninstall() and ComputeLastInstallTime() directly (lines 7241-7242), which enumerates the whole HKLM\...\Uninstall tree on the Explorer UI thread while the dialog is being created. GatherBackgroundStatus already computes exactly this on the setup thread and caches it (line 7866-7873) — just call LastInstallTimeText() and keep the registry walk off the UI thread.
  • LoadLibraryW(L"riched20.dll") uses the default search order (line 6453). riched20.dll isn't a KnownDLL, so this is the classic hijack shape; the mod is scoped to explorer.exe/control.exe, which only run from protected system directories, so the practical exposure is negligible — but LoadLibraryExW(L"riched20.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32) is free to write and matches what the rest of the file already does everywhere else (gdiplus.dll, dui70.dll, uxtheme.dll, …).
  • LogCurrentSettings() isn't just logging. It calls CleanupControlPanelTasksXmlFile() (line 4239), so a function named "log" deletes a file as a side effect. Move that call to LoadLanguageSetting (or Wh_ModInit) so the name matches the behaviour.
  • EnsureProgressClassRegistered's static bool done (line 6803) is a plain non-atomic flag touched from several Explorer UI threads. InitCommonControlsEx is idempotent so nothing breaks, but a function-local static initializer (static bool done = [] { ...; return true; }();) is both thread-safe and shorter.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • DisableMuiConfigInPrivateCopy depends on undocumented PE layout details (line 1324) — it locates the named MUI resource-directory entry and rewrites one character to CUI so UpdateResource will accept the file. You already call this out in the comment, and it's confined to a private copy with a graceful failure path, so it's fine — just noting it's the piece most likely to need attention if the pinned payload is ever re-pinned to a different build.
  • The private ABI structs are hand-reverse-engineered (NativeControlPanelNavLink, sizeof == 0x70, line 5117-5139). The static_asserts are a good guard against accidental drift, and the SHA-256 pin means the layout can't change under you — but RedirectNativeNavLink (line 5194-5197) also assumes the previous command union fields are notification data rather than allocated strings. If that assumption is wrong for some link kind, those two pointers leak per redirect. Worth a comment saying how it was verified, if you have it.
  • The combobox on the settings page is interactive but inert. Both SettingsDirectUiSubclassProc (line 4857-4861) and WuSettingsDlgProc (line 5842-5845) deliberately leave the dropdown enabled while never writing the selection back. That's a defensible choice given modern Windows owns the policy, and the read-only note explains it — but a user who changes the selection and sees nothing happen may read it as a bug. Consider disabling the control and letting the note carry the explanation, or adding the selection change to the note text.
  • IsUpdatesAvailable() is now a real heuristic (line 7758-7774) — comparing Results\Download\LastSuccessTime against Results\Install\LastSuccessTime is a solid improvement over the earlier key-existence check. Note that on Windows 11 these legacy Auto Update\Results values are often not maintained at all, so the amber banner will simply never appear there; that's the safe direction to fail, just be aware the feature is effectively Windows 10-only.
  • @architecture x86-64 with an AMD64-only payload means the mod injects into native ARM64 explorer.exe and then refuses to start in IsRunningAsAmd64 (line 465). That's the same trade-off documented and accepted in your merged Performance mod, so it's consistent — just noting the feature is silently unavailable on ARM64 rather than covered.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 12, 2026
Refactor settings dialog state management to use a struct instead of static globals. This change allows for multiple instances of the dialog to maintain their own state without conflicts.
@babamohammed2022

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 12, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


This is a very ambitious mod (10.5k lines, a downloaded payload, a registry virtualization layer, private DirectUI/ABI patching, and four hand-built dialogs) running inside explorer.exe. The mechanism generally works and the defensive engineering is visible throughout, but there are several concrete defects — a verify→load TOCTOU that is a code-execution vector, an unbounded join that hangs unload and can freeze the Explorer UI, a dead deduplication guard that leaks a COM object per page navigation, and a per-call GetModuleHandleW in a LoadImageW hook. Those should be fixed before this is ready for a human reviewer.

1. Payload verification is TOCTOU: the file is verified by path, then loaded by path.

EnsurePayload verifies finalPath with IsValidPayload (its own CreateFileW + SHA-256), returns the path, and SetupWorkerImpl then does an independent LoadLibraryExW(path.c_str(), nullptr, 0). Anything that can write to the mod storage folder — i.e. any process running as the same user — can replace the file between those two opens and get arbitrary code executed inside explorer.exe. EnsurePrivateModuleLoaded (the lazy CoCreateInstance fallback) has the same shape and doesn't verify at all.

You already solved this in your own Performance Information and Tools Restorer, which pins the file handle across the verify→load window and then proves the loaded module maps the same file object:

Please port that pattern here (verify through the pinned handle, confirm after LoadLibraryExW), and apply it to EnsurePrivateModuleLoaded too.

Related: ReuseExistingEmbeddedMuiResourceModule trusts a leftover file with a predictable name (wucltux.embedded-mui-<pid>-<lang>-*.mres), validated only by "LoadStringW(module, 1) returns the expected text". It's loaded as LOAD_LIBRARY_AS_DATAFILE | LOAD_LIBRARY_AS_IMAGE_RESOURCE, so it can't execute code, but the perf mod deliberately rebuilds instead of trusting such a file — worth doing the same, or at least hashing the file against the build output.

2. Unload can hang for minutes, and the Explorer UI thread can freeze, joining the WUA search thread.

RunRealWindowsUpdateSearch calls the synchronous IUpdateSearcher::Search with put_Online(VARIANT_TRUE). That is unbounded — an online WU scan routinely takes minutes and there is no way to cancel it once it's running. Two consequences:

  • Wh_ModUninit does g_wuSearchThread->join() (line ~10452). Disabling/updating the mod while a check is in flight blocks Windhawk's unload for the whole scan. The comment says it's "bounded by IUpdateSearcher's own timeouts" — it isn't.
  • Worse, StartWuUpdateCheck also joins it, on the Explorer UI thread (line ~7607), while holding g_wuSearchThreadMutex. The check dialog gives up after kWuCheckMaxDurationMs (45 s) and destroys itself, which clears g_checkingForUpdates and unregisters the dialog — but the search thread keeps running. The user then clicks "Check for updates" again, both guards pass, and Explorer's UI thread blocks in join() until the scan finishes. That's a full shell freeze reachable by clicking a link twice.

Use the asynchronous API so the search is actually cancellable: IUpdateSearcher::BeginSearchISearchJob::RequestAbort() from Wh_ModUninit (and when the dialog times out), then CleanUp(). At minimum, never join() on a UI thread — if a search is still running, just refuse to start a new check.

WaitForPayloadNoticeThread ends with WaitForSingleObject(thread, INFINITE) after the 5 s posting loop. It's a much smaller risk (the message box does handle WM_CLOSE), but it's still an unbounded wait in the teardown path; a bounded wait plus giving up would be safer.

3. The Explorer UI thread is blocked for up to 3 seconds inside the COM activation hooks.

WaitForPayloadReadinessWindow runs a Sleep(10) spin for kInitialSetupGraceMs (250 ms) and then up to kFinalizationGraceMs (3000 ms), and it's called from HandleCoCreateInstance / HandleCoGetClassObject — which run on whatever thread the shell is binding the namespace folder on, i.e. the Explorer UI thread. Navigating to Control Panel on a first run (or on a slow disk) freezes the window for up to three seconds with no feedback.

Since the item is deliberately registered before the payload is ready, it would be better to fail the activation immediately and show the fallback, or to make the shell re-navigate once g_verified flips, rather than to block the UI thread.

4. The wucltuxWroteToBag deduplication guard is dead code — it's two different variables.

PSPropertyBag_WriteUnknownHook declares static thread_local IPropertyBag* wucltuxWroteToBag at line ~5441, and PublishNativeNavigationLinks declares another function-local static with the same name at line ~5511. They're separate objects, so the check in PublishNativeNavigationLinks:

static thread_local IPropertyBag* wucltuxWroteToBag = nullptr;
if (wucltuxWroteToBag == bag) {   // never true - the hook wrote to its own copy
    bag->Release();
    return S_FALSE;
}

never fires. The comment ("On builds where wucltux DOES publish its own list for this bag (Windows 11) … publishing a second list afterwards would only allocate and leak") describes exactly what now happens: on those builds CreateNativeControlPanelNavLinks() allocates a replacement list on every SetSite, and because those objects carry kPinnedNavLinksReferenceCount = 0x10000000 they are never destroyed — a permanent leak per page navigation, plus it overwrites the correctly-redirected native list.

Hoist the marker to a single file-scope thread_local that both functions read/write.

While you're there: both wucltuxWroteToBag and lastPublishedBag AddRef the bag and are never released — including in Wh_ModUninit, so they keep COM objects alive after the mod is gone. They're thread_locals on Explorer UI threads, so there's no easy join point; consider storing a non-owning key (e.g. the bag pointer plus a generation counter) instead of an owning reference, or clearing them from the subclass/teardown path.

5. TryReserveWuSettingsDialogSlot cannot distinguish "you hold the reservation" from "someone else is creating one".

if (g_wuSettingsDlgPending) return nullptr;  // another thread is creating one
g_wuSettingsDlgPending = true;
return nullptr;

Both paths return nullptr, and ShowWuSettingsDialog treats nullptr as "proceed to create". So the flag has no effect and the race it was added to close is still open: two Explorer window threads can both create a settings dialog, and the second RegisterWuSettingsDialog clears g_wuSettingsDlgPending for both. Return a tri-state (e.g. enum class SlotResult { Reserved, AlreadyOpen, PendingElsewhere } with the existing HWND out-param) so the caller can bail on PendingElsewhere.

6. LoadImageW hook calls GetModuleHandleW on every single call.

static HANDLE WINAPI LoadImageWHookForLegacyWarningIcon(HINSTANCE instance, ...) {
    if (instance != GetModuleHandleW(L"shell32.dll")) return LoadImageWOriginalFor...(...);

LoadImageW is called constantly in the shell, and GetModuleHandleW takes the loader lock each time. Resolve shell32's HMODULE once in Wh_ModInit into a global and compare against that.

7. RegEnumKeyExWHook resolves the native key path on every exhausted enumeration, with no negative cache.

RegQueryInfoKeyWHook got a negative cache (g_nonNamespaceKeys) precisely because "RegQueryInfoKeyW runs constantly in explorer.exe" and QueryNativeRegistryPath costs two NtQueryKey round trips plus an up-to-64 KB std::vector. RegEnumKeyExWHook has exactly the same problem — every enumeration loop in the process that runs to ERROR_NO_MORE_ITEMS pays that cost — but no cache at all. Please share the same cache (and evict it in RegCloseKeyHook, as you already do).

Two related notes on that cache: it's capped at kNonNamespaceCacheMax = 512 and then clear()ed wholesale, so on a busy Explorer it will thrash and degrade back to the uncached cost; and CountOriginalNamespaceEntries re-enumerates the whole ControlPanel\NameSpace key on every hit.

8. The DirectUI hooks do full work for every DirectUI document in the shell.

DUISetXMLFromResourceHook calls LoadDirectUiResourceXml(...) — a full std::wstring copy of the UIFILE — and then IsWindowsUpdatePageXml on it, for every DirectUI resource dui70 loads anywhere in explorer.exe. DUISetXMLHook similarly builds PatchModernWuPageXml(xml) (another full copy) before comparing it to the input.

Both hooks receive the resource module. The WU page's XML always comes from the pinned payload, so an early if (resourceModule != g_module.load(...)) return original(...) would skip essentially all of this. That's a cheap, exact gate.

9. Private DirectUI internals are probed on unrelated windows.

IsSettingsDirectUiWindow runs for every top-level and child window on the thread (FindSettingsDirectUiHwnd), and for anything whose class is DirectUIHWND it does:

void* root = reinterpret_cast<void*>(GetWindowLongPtrW(hwnd, 0));
ATOM atom = pStrToID(L"auOptionSelectorCombobox");
return root && atom && pFindDescendent(root, atom);

That takes whatever is in the window's first extra-bytes slot and passes it to dui70's undocumented Element::FindDescendent. Explorer hosts many DirectUIHWND windows that this mod has nothing to do with; if any of them doesn't store an Element* there (or stores a different type), that's a wild pointer dereference in the shell. Please scope the search to the window subtree of the Control Panel frame you actually navigated, rather than every DirectUIHWND on the thread.

10. A 200 ms polling timer with a hardcoded ID is installed on a window the mod doesn't own.

InitializeNativeSettingsCombobox does SetTimer(hwndParent, 889, 200, nullptr) on a Windows-owned DirectUIHWND. Timer IDs are per-window, so 889 can collide with a timer that window (or another mod) already uses, silently replacing it. Use SetTimer(hwnd, 0, ...) on a window you own, or derive a unique ID.

Separately, the timer keeps firing every 200 ms for as long as the settings page is open, and each tick calls pStrToID + pFindDescendent + pSetEnabled even after the combobox has been populated. Kill the timer once g_nativeComboPopulated is set, or drive this from a one-shot timer.

11. Modules and files are leaked on every load/unload cycle.

  • Wh_ModUninit does g_module.store(nullptr) with no FreeLibrary, so the payload's module reference is leaked on every disable. (The comment says "do not unload the datafile" — but it's loaded with LoadLibraryExW(path, nullptr, 0), a full executable load, not a datafile.)
  • RebuildEmbeddedMuiForLanguage explicitly never frees the previous .mres module, so each language change leaves another mapped module and an undeletable file in the mod storage folder (CleanupGeneratedResourceModuleFiles then reports it as locked=).
  • KeyTracker::ClearWithoutFreeing drops every synthetic HKEY without closing it.
  • LoadLibraryW(L"riched20.dll") in WuFaqDlgProc's WM_INITDIALOG bumps riched20's refcount every time the FAQ window opens and never releases it.

Individually these are small, but they accumulate across the enable/disable/settings-change cycles that mod development and updates involve, and none of them is reclaimed until Explorer restarts. It would be good to at least free the payload and the current .mres module in Wh_ModUninit once the dialogs and subclasses are torn down, and to close the tracked handles.

12. The custom dialogs have no DPI scaling.

WuFaqDlgProc, WuCheckDlgProc and WuCheckResultDlgProc size the dialog itself in dialog units (DS_SETFONT + Segoe UI, which does scale) but then create every child control and paint the header at hardcoded pixels — 14, bodyTop=62, cw - 28, btnW = 68, btnH = 23, WuPaintDialogHeader(hdc, rc, 40, ..., 28, ...), 16, 44, w - 32, 18. At 125% / 150% / 200%, and on a secondary monitor with a different DPI, the controls will be misplaced or clipped relative to the scaled client area.

Scale the pixel constants by GetDpiForWindow(hwnd) (falling back to GetDeviceCaps(LOGPIXELSX)), and handle WM_DPICHANGED for the multi-monitor case.

13. FindWindowW(L"ControlPanelWindowClass", nullptr) is not filtered by process.

PostControlPanelRefresh and StartWuUpdateCheck both fall back to a desktop-wide FindWindowW, which can return a window belonging to a different process — and PostControlPanelRefresh then posts WM_COMMAND, MAKEWPARAM(0xA220, 0) to it, and StartWuUpdateCheck uses it as a dialog owner (cross-process owner relationships are a bad idea). Enumerate with EnumWindows and filter on GetWindowThreadProcessId(...) == GetCurrentProcessId().

14. The classic settings dialog silently discards the user's selection.

The combobox is deliberately left enabled ("so it feels interactive") while WuSettingsDlgProc never writes anything back, and IDOK just closes the window. A user who picks "Never check for updates" and presses OK will reasonably believe they changed a system setting. Either disable the combobox like the three checkboxes next to it, or replace OK/Cancel with a single Close and make the read-only nature obvious in the dialog itself (the grey note at the bottom isn't enough when the control accepts input and the button says OK).

15. Downloading wucltux.dll from the Microsoft Symbol Server.

Windhawk's guidance is that a mod must be self-contained and must not depend on, download, or contact external servers. This mod requires a ~one-time network fetch before the page can render at all, and the pinned msdl.microsoft.com URL is a hard external dependency (as the README acknowledges: "if Microsoft removes that file, first-time setup will no longer work unless the verified payload was cached").

I'm flagging it rather than blocking on it, because your Performance Information and Tools Restorer does the same thing and was merged — so there's precedent and this is ultimately the maintainer's call. Worth being explicit about it in the PR description so it gets decided deliberately rather than by inheritance.

Two things that make the situation here worse than in that mod, and are worth mentioning either way: the hooking depends on hardcoded RVAs into the pinned build (kCElementWithSiteSetSiteRva = 0x26960, kControlPanelNavLinksVtableRva = 0x2350) plus reverse-engineered struct layouts (NativeControlPanelNavLink with BYTE privateState[0x33]), so the mod is welded to one exact binary; and DisableMuiConfigInPrivateCopy rewrites that binary's resource-config directory based on undocumented layout details. If the pin ever has to change, all of that has to be re-derived. Consider adding a cheap sanity check (e.g. verifying the expected first bytes of the function at kCElementWithSiteSetSiteRva) before calling Wh_SetFunctionHook on a raw offset.

Optional improvements

Minor polish — none of this affects users, so it's your call.

  • LoadLibraryW(L"riched20.dll") (WuFaqDlgProc) is a bare-name load, so it uses the default search order including the executable's directory. riched20.dll isn't a KnownDLL. explorer.exe and control.exe only ever run from System32, so the hijack surface is negligible here — but for consistency with the rest of the file (which correctly uses LOAD_LIBRARY_SEARCH_SYSTEM32 everywhere else) it should be LoadLibraryExW(L"riched20.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32).
  • EnsureProgressClassRegistered uses a plain static bool done and is called from multiple Explorer UI threads — the write isn't synchronized. The consequence is at worst a redundant InitCommonControlsEx, but static std::atomic<bool> or a function-local static initializer (which is thread-safe in C++) would be cleaner.
  • g_wuFaqParent is a plain global HWND written by ShowWuFaqDialog and read in WM_INITDIALOG. Each Explorer window runs on its own thread, so two windows opening the FAQ can cross-assign it and center the dialog over the wrong parent. Pass the parent through CreateDialogIndirectParamW's dwInitParam, like WuSettingsDlgState already does.
  • LoadImageWHookForLegacyWarningIcon returns CopyIcon(icon). Callers that passed LR_SHARED don't destroy what LoadImageW returns, so those calls will leak an icon handle. Worth checking whether the DirectUI icon(..., library(shell32.dll)) path uses LR_SHARED; if so, cache one non-shared icon per (id, size) and return it directly instead of copying.
  • DownloadWithTimeout has a no-op branch:
    if (g_stopping.load()) {
        g_downloadUrl = url;
    } else {
        g_downloadUrl = url;
    }
    Both arms are identical — collapse to a single assignment (the comment above it explains the intent, which the code already satisfies unconditionally).
  • UniqueWinHandle::Release() sets handle_ = INVALID_HANDLE_VALUE rather than nullptr, and Reset()'s default argument is INVALID_HANDLE_VALUE. It works because IsValid() checks both, but the two sentinels make the class harder to reason about than it needs to be.
  • PayloadNoticeCaption() has a dead branch — if (LanguageIs(L"it")) return L"Windows Update"; followed by return L"Windows Update";.
  • CurrentLanguage() returns a fresh std::wstring and is called on render paths (e.g. SelectWuCheckResultTexts, SelectRecommendedUpdatesLabel, and the various unordered_map<std::wstring, ...> lookups). Since the language is already an enum class Language index, these tables could be indexed directly instead of allocating and hashing a string per lookup.
  • Several #ifdef _WIN64 / #endif pairs in InstallModernWuXmlPatchHook are empty — leftovers from a 32-bit variant that no longer exists (the mod is x64-only, as the static_assert(sizeof(void*) == 8, ...) states).
  • The README says "Nothing is written to the real registry or to system files", which is very slightly overstated — EnsureVirtualKeyRoot does create a real (volatile) HKCU\Software\WindhawkWindowsUpdateControlPanelRestorer\VirtualKeys-<pid> key and RegSetValueExWs an owner marker into it. The rest of the paragraph explains this accurately; just the leading sentence reads stronger than what the code does.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • The four custom dialogs are modeless and won't get keyboard navigation. CreateDialogIndirectParamW creates a modeless dialog, which needs IsDialogMessage in the owning thread's message loop for Tab / Esc / Enter to work. Explorer's loop won't do that for a window it doesn't know about, so Tab won't move between the combobox and the checkboxes, Esc won't close, and Enter won't hit the default button. If you want proper keyboard behaviour, either use DialogBoxIndirectParamW (modal — but then it blocks the Explorer UI thread, so probably not) or run the dialog on its own thread with its own IsDialogMessage loop.
  • The "Check for updates" flow triggers a full online WU scan from explorer.exe. That's an expensive operation (network + wuauserv + potentially a TrustedInstaller scan) launched from a shell process, and the progress bar is a pure animation ((elapsed * 90) / 45000) that has no relationship to the actual search progress. IUpdateSearcher's async form exposes ISearchJob and a callback, so the bar could reflect something real; failing that, an indeterminate (PBS_MARQUEE) bar would be more honest than a fake percentage.
  • g_wuCheckedTick is initialized to (ULONGLONG)-1 as a "never probed" sentinel, but GetTickCount64() legitimately can't reach that value, so it works — it's just fragile. A separate std::atomic<bool> g_wuProbed would be clearer (you already have g_cachedWuServiceProbed for the render side).
  • The status/skin state is only re-read at page render. Wh_ModSettingsChanged only acts when the language changed; toggling UpdatePageSkin, ShowAvailableUpdates, LinkSystemSettingsText or RemoveLegacyBrokenOption updates the atomics but the already-rendered page won't change until the user navigates away and back. Worth mentioning in the settings descriptions if you don't want to force a refresh.
  • Running this alongside Performance Information and Tools Restorer doubles the process-wide hook surface. Both mods install their own RegOpenKeyExW/RegQueryValueExW/RegEnumKeyExW/CoCreateInstance/CoGetClassObject virtualization layers in the same explorer.exe. They should compose (each ignores the other's CLSIDs), but every registry call in the shell then pays both keyword gates. Since the two mods already share most of this infrastructure, it may be worth thinking about whether the Control Panel virtualization layer could eventually live in one place.
  • ComputeLastInstallDateFromUninstall / ReadWuaResultString read WU's recorded registry timestamps, which are frequently stale or absent on Windows 11 (the modern update stack doesn't maintain the legacy Auto Update\Results keys consistently). Showing "N/A" is handled, but you may find the IUpdateHistoryEntry collection from IUpdateSearcher::QueryHistory gives a more reliable "last installed" date — it's a local query, so it's cheap compared to the online search.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 12, 2026
Updated the Windows Update Control Panel Restorer to improve the handling of the native combobox and payload verification process.
@babamohammed2022

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 13, 2026
@windhawk-reviewer

Copy link
Copy Markdown

This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time.

Comment /ai-review again after 2026-08-13 09:32 UTC (in 2 hours) to get another one.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 13, 2026
@babamohammed2022

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 13, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

Note: This review was done by Claude. Due to the amount of submissions, doing a fully manual review for each pull request is no longer feasible. Thank you for understanding.

Remember: The AI reviewer can be wrong - it may misread code, flag correct code as broken, or suggest changes that make things worse. Treat its findings as suggestions to verify, not instructions to follow blindly. You're responsible for the code you submit, so if a finding doesn't hold up, say so instead of changing working code to satisfy it.

Please address the following issues. The items in the collapsed sections are optional, so it's your call whether to address them.


The overall structure follows the pattern you already established in performance-info-tools-restorer.wh.cpp, and the payload handling in particular is careful (pin-then-verify-then-load with ConfirmLoadedModuleMatchesPin genuinely closes the TOCTOU window, and re-verifying the SHA-256 through a pinned handle on every load means a tampered cached copy can't be mapped). The findings below are mostly around mod unload and the mod's own dialogs.

1. The update-search thread is detached at unload, which will crash explorer.exe.

Wh_ModUninit waits 5s for g_wuSearchThread and then detaches it:

} else {
    Wh_Log(L"...online update search still running at unload; detaching instead of blocking Explorer");
    g_wuSearchThread->detach();
}

Windhawk unloads the mod with a single FreeLibrary the moment Wh_ModUninit returns, so the detached thread is left executing code that is about to be unmapped. This isn't a "touches freed state" hazard that can be dodged - the thread's instruction pointer and its return address are inside the mod image. After IUpdateSearcher::Search returns it still runs IsPendingWindowsUpdate(), the g_realCheckHr/UpdateCount/PendingReboot/Done stores and two Wh_Log calls, all of which are mod code. And because Search with put_Online(TRUE) routinely takes minutes, the 5s wait will frequently expire - so any disable / settings-reload / mod update that lands while a check is running takes the shell down.

The comment acknowledges this and points at the right fix - please do it rather than deferring: use the asynchronous IUpdateSearcher::BeginSearch and cancel with ISearchJob::RequestAbort in Wh_ModUninit, then join() unconditionally. That makes the wait bounded by the abort rather than by the search, so no thread can survive Wh_ModUninit. Trading a hang for a crash is not the right trade here; if RequestAbort turns out not to be reliable enough on its own, the fallback should still be an unconditional join, not detach().

Background on the unloadability contract and the worker-thread case specifically: https://github.com/ramensoftware/windhawk/wiki/Global-objects-and-process-shutdown (see #1-worker-thread-stdthread). The [[clang::no_destroy]] std::optional<std::thread> wrapper you used for all three threads is exactly right - it's only the detach() escape hatch that breaks the contract.

2. The payload-notice thread has the same escape hatch.

WaitForPayloadNoticeThread posts WM_CLOSE for 5s, then posts IDCANCEL and waits 3 more seconds, and then:

if (WaitForSingleObject(thread, kFinalWaitMs) == WAIT_TIMEOUT) {
    Wh_Log(L"...payload-notice thread did not exit in time; giving up rather than blocking unload");
}

Wh_ModUninit then returns and the image is unmapped while PayloadNoticeThreadProc (and its MessageBoxW frame, whose return address is in the mod) is still live - same crash as above.

The wait here is safe to make unconditional: the notice runs on its own dedicated thread that holds no locks, and MB_YESNOCANCEL means the posted WM_CLOSE/IDCANCEL will dismiss it. Keep the posting loop, but replace the final bounded wait with WaitForSingleObject(thread, INFINITE). If you'd rather not rely on MessageBoxW responding, replace it with a modeless window of your own that Wh_ModUninit can DestroyWindow deterministically - you already do exactly that for the check/FAQ/settings dialogs.

3. The mod's own dialogs have no DPI scaling.

There is no GetDpiForWindow / MulDiv / WM_DPICHANGED handling anywhere in the file, but the FAQ, check and result windows lay their controls out in raw pixels inside WM_INITDIALOG, and the shared header is painted at fixed pixel sizes:

// WuFaqDlgProc
const int bodyTop = 62; ... const int btnW = 68; const int btnH = 23;
CreateWindowExW(WS_EX_CLIENTEDGE, L"RichEdit20W", ..., 14, bodyTop, cw - 28, bodyHeight, ...);

// WuCheckDlgProc
CreateWindowExW(0, L"STATIC", ..., 16, 44, w - 32, 18, ...);
CreateWindowExW(0, L"msctls_progress32", ..., 16, 64, w - 32, 14, ...);

// WuPaintDialogHeader / WuCreateHeaderFont
RECT header = { client.left, client.top, client.right, headerHeight };  // 40 or 56
DrawIconEx(hdc, 16, y, icon, iconSize, iconSize, ...);
return CreateFontW(-16, 0, 0, 0, FW_SEMIBOLD, ...);

The dialog shells are built from DLGTEMPLATE with DS_SETFONT, so their client area scales with DPI - but none of the above does. At 150%/200% (i.e. most laptops, and any secondary monitor at a different scale) the header strip and its 16px title font stay small against a much larger dialog, and the label/progress bar/Close button sit at the wrong offsets. Explorer is per-monitor-v2 aware on Win10/11, so this also changes when the window is dragged between monitors.

Scale every hard-coded pixel value through the window's DPI, e.g.:

const UINT dpi = GetDpiForWindow(hwnd);
const auto S = [dpi](int px) { return MulDiv(px, dpi, 96); };
... CreateWindowExW(0, L"STATIC", ..., S(16), S(44), w - S(32), S(18), ...);

and use -MulDiv(16, dpi, 96) for the header font. classic-taskbar-properties.wh.cpp#L2388 does this for the same kind of hand-built Win32 dialog. (The "Change settings" dialog itself is fine - it uses dialog units via addCtrl, so only the three pixel-laid-out windows and the shared header need this.)

Optional improvements

Minor polish - none of this affects users, so it's your call.

  • LoadLibraryW(L"riched20.dll") in WuFaqDlgProc is the one bare-name load left in the file; everything else correctly uses LOAD_LIBRARY_SEARCH_SYSTEM32. riched20.dll is not a KnownDLL, so this uses the default search order. With @include explorer.exe / control.exe (both in a protected system directory) the hijack surface is negligible, so this is a hardening nit rather than a real vector - but for consistency use LoadLibraryExW(L"riched20.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32). It's also called on every FAQ open and never freed, so hoisting it to a static one-shot would be tidier.

  • The LoadImageW hook ignores LR_SHARED. It always returns CopyIcon(icon). LoadImageW with LR_SHARED is documented to return a cached handle the caller must not destroy - a caller taking that path leaks one icon per call. Return the cached g_statusIconCache handle directly when flags & LR_SHARED, and CopyIcon only otherwise.

  • DUISetXMLHook copies every DirectUI document in the process. It runs std::wstring patched = PatchModernWuPageXml(xml); and then patched == xml before any cheap filter, so every DirectUI page parsed anywhere in explorer.exe (folder windows, shell dialogs, ...) costs a full allocation plus two passes over the document. DUISetXMLFromResourceHook already gates on IsWindowsUpdatePageXml first - do the same here on the raw const WCHAR* (wcsstr(xml, L"atom(pageSettings)") / a wcsstr-based IsWindowsUpdatePageXml) before constructing the string.

  • ContainsRelevantKeywordInsensitive uses towlower per character per needle and sits in front of RegOpenKeyExW/RegGetValueW, two of the hottest APIs in the shell. The needles are ASCII, so an inline (c >= L'A' && c <= L'Z') ? c | 0x20 : c avoids the locale-dependent CRT call entirely (and sidesteps the theoretical Turkish-I mismatch that would silently disable the whole virtualization layer).

  • InstallWucltuxSetSiteHook uses raw Wh_SetFunctionHook with void* casts while the rest of the file uses WindhawkUtils::SetFunctionHook. Since you already have CElementWithSiteSetSite_t, WindhawkUtils::SetFunctionHook(reinterpret_cast<CElementWithSiteSetSite_t>(target), CElementWithSiteSetSiteHook, &CElementWithSiteSetSiteOriginal) is equivalent and type-checked.

  • Leaked COM references on the nav-link path. g_wucltuxWroteToBag and PublishNativeNavigationLinks's lastPublishedBag are thread_local raw IPropertyBag* holding an AddRef, and nothing ever Releases them - the bags stay alive after the mod unloads. Likewise the fabricated NativeControlPanelNavLinks object is deliberately pinned with kPinnedNavLinksReferenceCount = 0x10000000. Both are small and bounded in practice, but they're worth a comment or a cleanup pass if you touch that code again.

  • README accuracy: "Disabling the mod ... removes the files it created" is not quite what happens - CleanupGeneratedResourceModuleFiles(true) deliberately skips files that are still mapped (the code counts them as locked=), and the .mres modules and wucltux.dll itself are intentionally never FreeLibrary'd. Worth softening the wording. The ARM64 limitation is also worth stating: IsRunningAsAmd64() correctly makes Wh_ModInit bail on a native ARM64 shell, but the README doesn't mention that the mod is x64-only.

  • CryptAcquireContextW + CALG_SHA_256 is the deprecated CryptoAPI path; BCryptOpenAlgorithmProvider(BCRYPT_SHA256_ALGORITHM) is the current one. Works fine as-is, purely a modernization note.

Functionality notes

Non-critical observations and ideas about the feature behavior itself.

  • ShowNativeCombobox ships a control that knowingly does nothing. The setting's own description says "selecting an option does not actually change Windows Update's behavior", and SettingsDirectUiSubclassProc leaves the combobox enabled purely so it "feels interactive". A user who enables it gets a dropdown that silently discards every selection, which is worse than the disabled placeholder. I'd suggest dropping the setting until the write-back is implemented (hooking the WU policy reads the way the mod already virtualizes the Control Panel registration would be the reversible way to do it) - or at minimum making the label say the choice isn't saved, so it's obvious in the UI and not only in the settings description.

  • Blast radius of the hook set. The mod installs roughly twenty process-wide hooks in explorer.exe - eight registry functions, CoCreateInstance/CoGetClassObject on both combase and ole32, LoadStringW, LoadImageW, SHLoadIndirectString, ExtractIconExW/SHDefExtractIconW/PrivateExtractIconsW, ShellExecuteW/ShellExecuteExW, PSPropertyBag_WriteUnknown and DirectUI SetXML - all live for the whole session to serve a page that's opened rarely. The filters are tight (synthetic CLSIDs, the wurestorer: prefix, g_module-scoped HINSTANCE checks, synthetic icon IDs 61002-61005, a vtable-in-payload check), so I don't think there's a concrete bug here, and the registry/COM virtualization genuinely has no cleaner alternative given the design. Just flagging it as the main risk surface of the mod - it's worth keeping the filters as the first statement in each hook and resisting adding more.

  • Hard-coded RVAs into the payload. kCElementWithSiteSetSiteRva = 0x26960 and kControlPanelNavLinksVtableRva = 0x2350, plus the reverse-engineered NativeControlPanelNavLink layout, are only valid for the one SHA-256-pinned build. That's a sound mitigation (the static_asserts on the struct layout help too), but it does mean the pinned hash, the RVAs and the struct offsets have to move together - a short comment block listing all three as a single unit would make that hard to get wrong later.

  • The download is a single point of failure for first-run setup. The msdl URL is tied to one historical build; if Microsoft ever drops it, new installs can never get the payload (existing ones keep working from cache). You already handle this gracefully with the localized notice, so this is just an FYI - it's the same trade-off the Performance Information and Tools Restorer already makes, so I don't think it blocks anything.

  • Resource-module churn. Each language change builds a new ~1 MB .mres copy and deliberately never frees the old module, so the previous file can't be deleted for the life of the process. ReuseExistingEmbeddedMuiResourceModule bounds this to roughly one file per (PID, language), which is fine - noting it only so it doesn't look like an unbounded leak if someone reads that path later.


Next steps:

  • /ai-review - after pushing fixes, to get a review of the updated code. You can repeat this as many times as you need, but each review is thorough and usually there's no need for more than 2-3 iterations.
  • /ready-for-reviewer - once you're satisfied with the state of the pull request, to hand it over to a human reviewer. If some findings above are left unaddressed, add a short note explaining why.

See the review process for details.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 13, 2026
@babamohammed2022

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-ai-review An AI review was requested and is being prepared. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 13, 2026
@windhawk-reviewer

Copy link
Copy Markdown

This pull request has already had 3 AI reviews in the last 24 hours, which is the limit, so no review was posted this time.

Comment /ai-review again after 2026-08-13 19:46 UTC (in 7 hours) to get another one.

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-author The author's turn: request an AI review, or respond to one that was posted. and removed waiting-for-ai-review An AI review was requested and is being prepared. labels Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-author The author's turn: request an AI review, or respond to one that was posted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant