Skip to content

Create the Windows 7 AutoPlay Dialog Restorer mod - #5204

Merged
m417z merged 7 commits into
ramensoftware:mainfrom
babamohammed2022:patch-32
Aug 30, 2026
Merged

Create the Windows 7 AutoPlay Dialog Restorer mod#5204
m417z merged 7 commits into
ramensoftware:mainfrom
babamohammed2022:patch-32

Conversation

@babamohammed2022

@babamohammed2022 babamohammed2022 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

This mod restores the classic Windows 7 AutoPlay dialog on Windows 10 and Windows 11.

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.

Updated system requirements and privacy setting information in the documentation.
@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 24, 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

@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 24, 2026
@babamohammed2022 babamohammed2022 changed the title Revise mod documentation for Windows compatibility Create the Windows 7 AutoPlay Dialog Restorer mod Aug 24, 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.


Nice, complete piece of work — the policy handling, the "don't auto-run a remembered program" decision, the WPD mass-storage alias filtering and the DPI-aware custom painting all show real care. The items below are mostly about how it integrates with Windhawk and Explorer.

1. This should almost certainly be a tool mod, not an explorer.exe injection.

The mod installs no function or symbol hooks at all — everything it uses (RegisterDeviceNotificationW, the WM_DEVICECHANGE/DBT_DEVTYP_VOLUME broadcast to top-level windows, CoRegisterClassObject + the ROT, ShellExecuteEx, WIC, IPortableDeviceManager) works from any process in the session. It also hand-rolls the two things the tool-mod framework gives you for free: a single-instance mutex (Local\Win7ClassicAutoPlay.Owner, L5241) and an "am I the shell process" check (IsMainExplorerShell, L5235).

Meanwhile the cost of living in Explorer is high: a full custom-painted dialog with its own message loop, WIC decoding of ~180 KB of PNG, COM/WPD device enumeration, and a recursive directory scan of untrusted removable media (ScanDirLimited, L5843) all run inside the shell process. Any fault there takes down the desktop.

Note that the native-AutoPlay suppression does not require being in Explorer: IQueryCancelAutoPlay is a documented cross-process mechanism — you already register with CLSCTX_LOCAL_SERVER and give the object a free-threaded marshaler (L5266), and the ROT is per-session, so explorer.exe reaches the object fine from windhawk.exe.

Please convert: @include windhawk.exe, rename Wh_ModInit/Wh_ModSettingsChanged/Wh_ModUninitWhTool_*, and paste the launcher snippet from Mods as tools: Running mods in a dedicated process verbatim; then delete g_hOwnerMutex, TryBecomeAutoPlayOwner, ReleaseAutoPlayOwner, GetTrayOwnerPid and IsMainExplorerShell. mods/explorer-folder-hover-menu.wh.cpp is a good reference — its boilerplate at the bottom of the file is an unmodified copy of the wiki snippet.

(This interacts with item 3 — see the note there before deciding.)

As a side effect this also fixes a startup race: the ownership decision is made once in Wh_ModInit, and Wh_ModInit runs before the process starts executing, so FindWindowW(L"Shell_TrayWnd") is normally NULL at that point. If the mutex is ever lost (e.g. the previous shell process hasn't finished exiting), the new shell silently never handles AutoPlay again until the mod is toggled — there is no retry path.

2. Wh_ModUninit can return while the UI thread is still running mod code.

if (g_evtListenerReady) WaitForSingleObject(g_evtListenerReady, 5000);
if (g_hUiThread) {
    if (g_hwndListener) PostMessageW(g_hwndListener, WMU_SHUTDOWN, 0, 0);
    WaitForSingleObject(g_hUiThread, 10000);   // <-- L7736

Windhawk FreeLibrarys the mod as soon as Wh_ModUninit returns, so by then no mod code may still be running. If the 10 s timeout expires the mod image is unmapped while AutoPlayUiThreadProc is executing — instant crash in the host, whether or not it touches mod state, because its instruction pointer and return address live in the image. The timeout is reachable: the UI thread services the WMU_SHUTDOWN message from the same loop that runs ExecuteOptionByIndex, which can sit in ShellExecuteExW with lpVerb = L"runas" (a UAC consent prompt — unbounded, L5991), in SHObjectProperties (L5962), or in IPortableDeviceManager::RefreshDeviceList (L5486/L5524), which is routinely slow.

The knock-on damage in that window is worse than the thread itself: UnregisterClassW (L7743-7746) then fails because the windows still exist, leaving AutoPlayDialogProc/ListenerWndProc registered as class window procs pointing into unmapped memory; and UnregisterCancelAutoPlay() never ran, so g_rot/g_classCookie are simply zeroed (L7749-7751) while the class object stays registered — the shell can then marshal a call into freed code on the next device arrival.

Use INFINITE for both waits, and make sure the shutdown path can't block: the message loop that must service WMU_SHUTDOWN shouldn't be the one making blocking shell calls (invoke the action and close the dialog, or hand the blocking invocation to a separate short-lived thread that is also joined).

3. The mod writes to the registry and normally leaves those writes behind.

WriteCancelAutoPlayClsid(true) (L5360) creates two values under
HKCU\Software\Microsoft\Windows\CurrentVersion\Explorer\AutoplayHandlers\CancelAutoplay\CLSID. They're deleted from Wh_ModUninit — but Wh_ModUninit does not run when the host process terminates (Explorer restart, sign-out, reboot, crash). So in ordinary use the values survive every logoff, and they survive uninstalling the mod entirely, leaving the shell with a CancelAutoplay CLSID that is registered nowhere. A mod's effects have to disappear when it's disabled; persistent registry/system changes are the one thing Windhawk asks mods not to make.

The standard Windhawk answer is to hook the read instead of writing the value — i.e. hook the shell's enumeration of that key (RegEnumValueW/RegQueryValueExW) and synthesize the CLSID entry, so nothing is ever persisted. That requires staying inside explorer.exe, which is a legitimate counter-argument to item 1 — so please pick one of the two and say which, rather than doing both. If you keep the registry write, at minimum delete the values unconditionally at startup as well (right now WriteCancelAutoPlayClsid(false) is gated on g_cancelRegWritten, which is false on a fresh load, so a leftover from a previous session is never cleaned — only overwritten), and call the leftover out in the README.

4. The autorun.inf "run program" entry is offered for USB / removable drives.

BuildDriveDialog parses autorun.inf for every ready volume regardless of drive type (L6993-6996), and BuildOptions turns it into the first, bold, visually-primary row (L6165-6189). Windows 7 does not do this: since Win7 RTM (and Vista/XP after KB971029), autorun.inf-driven AutoPlay entries are restricted to non-removable optical media — that restriction is the Conficker/USB-worm mitigation. So the mod is more permissive than the Windows 7 dialog it's restoring, and one click runs an arbitrary EXE off the stick.

It's made worse by the label and icon being attacker-controlled: prog.line1 = ar.action and MakeProgramIcon take action= and icon= straight from the .inf (L6171-6172, L6137), so a malicious stick can render a row that reads exactly like "Open folder to view files" with the folder icon.

You already do the two right things (path must resolve under the volume root, and TryExecuteRemembered deliberately refuses Program| tokens) — please also gate the program option on g_driveType == DRIVE_CDROM, matching real Windows 7 behavior.

5. Shared icon handles can dangle after WM_CREATE.

case WM_CREATE:
    g_dpi = GetBestDpiForWindow(hWnd);
    EnsureDpiResources();     // L7104-7105
    return 0;

EnsureDpiResources() calls FreeDpiResources() and recreates every shared bitmap/icon, but g_options[i].icon and g_hdrIcon hold those exact handles with shared = true and nothing rebinds them here — unlike the WM_DPICHANGED path (L7283-7285) and ShowAutoPlayDialog (L6899-6900), which both call RebuildHeaderIcon() + RebindSharedIcons() afterwards. If the DPI GetDpiForWindow reports differs from the one GetDpiForMonitor gave in ShowAutoPlayDialog, the next WM_PAINT draws through destroyed (possibly already-recycled) GDI handles. ComputeLayout() also calls EnsureDpiResources() (L6443), so the same hazard sits on the paint/hit-test path.

Simplest fix is to make recreation and rebinding a single operation — have EnsureDpiResources() report whether it recreated, and rebind at every call site (or move RebuildHeaderIcon()/RebindSharedIcons() into a small wrapper that all callers use).

6. The README needs a screenshot.

This is a mod whose entire point is a specific visual dialog, and the README has no image. Please add a screenshot (or a short GIF of a drive being inserted). Only i.imgur.com and raw.githubusercontent.com are allowed as image hosts.

7. Trim the embedded icon payload — the source is 7,772 lines / 487 KB.

Three base64 blobs account for ~180 KB of it, all decoded only at 16–48 px:

blob lines decoded PNG
WIN7_NATIVE_READYBOOST_BASE64 2495-3624 ~81 KB
WMP_PLAYER_ICON_BASE64 3625-4544 ~52 KB
USER_REMOVABLE_ICON_BASE64 1173-2009 ~48 KB

The other sixteen blobs in the same file are 1-6 KB each, which is what a 48x48 PNG should cost — these three are clearly full-resolution source images. Re-encoding them at the sizes actually used removes roughly 5,000 lines, and also cuts real work at runtime, since EnsureDpiResources() decodes and downscales all of them again on every DPI change. Two more blobs are dead and marked [[maybe_unused]]USER_PRINTER_ICON_BASE64 (L2138) and WIN7_NATIVE_WARNING_BASE64 (L2465); please drop them.

Optional improvements

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

  • The blanket try { ... } catch (...) blocks don't do what they look like they do. With the mingw-w64 Clang toolchain Windhawk uses, catch (...) catches C++ exceptions only, not access violations — so it won't save Explorer from the failure mode you're presumably guarding against, while the code it wraps (Win32 calls, COM HRESULTs) doesn't throw in the first place. std::bad_alloc is the only realistic case. I'd drop them; they add a lot of noise and an indentation level to almost every function.

  • Dead state. g_hotRects and struct HotRect are filled in PaintDialog and cleared, but never read (hit-testing goes through ComputeLayout/HitTestLayout); same for g_rcCheckHit (L6646), g_rcLink (L6670), g_headerH (L6452), g_checkBottom (L6474) and PendingVolume::firstTick (L7392, L7504). g_bmpAutoPlay16 is created and destroyed on every DPI change but never drawn.

  • -loleaut32 is unused (L10) — there's no BSTR/VARIANT/SAFEARRAY use in the mod.

  • EnableNonClientDpiScaling is called too late (L6952). It only has an effect when called during WM_NCCREATE; after CreateWindowExW has returned it fails and the non-client area won't scale on a DPI change. Move it into a WM_NCCREATE handler.

  • ComputeLayout() is re-run on every mouse move. WM_MOUSEMOVE, WM_SETCURSOR (twice — once directly and once inside HitTestLayout), WM_LBUTTONDOWN/UP, MoveFocus, SetDefaultFocus and PaintDialog each rebuild the whole layout, which means a std::vector, a screen DC and a DrawTextW(DT_CALCRECT) per option row. Computing it once per dialog build/resize and caching it would be simpler and cheaper.

  • The QueryCancelAutoPlay registered-message handler is unreachable (L7100-7101, L7540-7541). That message is sent to the foreground window; the listener is a hidden WS_EX_NOACTIVATE tool window, and the dialog doesn't exist yet when AutoPlay fires. The COM path is what's actually doing the work, so this can go.

  • Code comments are in Italian (e.g. L147-151, L162, L170, L4786, L4899, L5058). Not user-facing, so nothing breaks, but English comments make the mod easier for others to maintain.

  • IsSkipDirName lists "System Volume Information" twice (L5809, L5811) — the second comparison looks like it was meant to be another name.

  • The CreateIconIndirect fallback at L4796-4802 passes a 32-bpp DIB as hbmMask. ICONINFO::hbmMask must be a 1-bpp monochrome bitmap; that path will either fail or produce a garbage icon. CreateIconFromBase64PNG (L4695) already builds a proper mask — reuse that, or just leave g_hicoDrive48 null and let the g_bmpDrive48 path handle it.

Functionality notes

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

  • "Always do this" on the program row silently does nothing — and clobbers the previous choice. ExecuteOptionByIndex writes a Program|<name> token (L6338-6340), but TryExecuteRemembered always returns false for it (L6391-6392). That's the right security call, but the token is stored under the same software class key, so it overwrites e.g. a previously remembered OpenFolder and leaves that class permanently un-remembered. Either hide the checkbox while the program row is the selected option, or skip persisting the token.

  • Native AutoPlay is vetoed even on paths where the mod then shows nothing. AllowAutoPlay returns S_FALSE for every drive type the mod handles, but several later paths bail out without a dialog: BuildDriveDialog returns early when the volume isn't ready and isn't optical (L6982), ProcessPendingQueue drops entries after 32 tries (16 for WPD), and BuildWpdDialog returns early when the device can't be resolved. In those cases the user gets neither the Windows toast nor the classic dialog. Falling back to allowing native AutoPlay when the mod gives up would be more forgiving.

  • LooksLikeBlankOptical treats "can't read the volume" as blank (L5801-5802) — that's also what a dirty or damaged disc looks like, so those get classified as BlankDisc.

  • Video content gets the "Music options" section header. SectionTitleForGroup maps both ContentKind::Video and ContentKind::DvdMovie to lp->musicOptions (L6433-6434), and AlwaysText maps Video to alwaysGeneral (L5935). Worth a dedicated videoOptions string, or reusing mixedOptions.

  • ExecuteViewPictures quotes the path passed to ImageView_Fullscreen (L6073-6077). Photo Viewer's rundll32 entry point is generally invoked with the path unquoted; quoted paths are known to fail on some builds. Worth testing with a path that contains spaces.

  • MonitorForDialog() follows the foreground window (L6737-6743). Windows 7 showed the AutoPlay dialog on the primary monitor; following the foreground window means the dialog can appear on a different display than the user expects when a full-screen app is running elsewhere.

  • The remembered choice is keyed by content class, not by device — a choice remembered for usb applies to every removable volume. That does match Win7's per-content-type model, just noting it since the README's "The 'Always do this' choice is remembered by the mod" doesn't say what the scope is.


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 24, 2026
@babamohammed2022

Copy link
Copy Markdown
Contributor Author

Converting the mod to a toolmod has caused a regression on my side, if the maintainer believes that it should be done I'll try it then

@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 24, 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.


Good progress since the last round — the icon payload is down by ~1,400 lines, the autorun.inf program row is now restricted to optical media, EnableNonClientDpiScaling moved to WM_NCCREATE, the CreateIconIndirect mask bug and the duplicate IsSkipDirName entry are fixed, and EnsureDpiResourcesAndRebind() closes the shared-icon-handle hole cleanly. Most of what's left below comes from the two fixes that changed the threading and registry behavior — each introduced a new problem.

1. The startup registry cleanup runs in every explorer.exe, so it deletes the values the owning instance just wrote.

BOOL Wh_ModInit() {
    LoadSettings();
    WriteCancelAutoPlayClsid(false);   // L6303 - runs before any ownership check
    ...
    if (!mainShell)                return TRUE;   // L6309
    if (!TryBecomeAutoPlayOwner()) return TRUE;   // L6313

@include explorer.exe injects into every explorer.exe process, not just the shell one. Any secondary instance — a folder window when "launch folder windows in a separate process" is on, or the short-lived explorer.exe that ShellExecuteW(NULL, L"explore", ...) spawns from your own Open folder action (L4517) — loads the mod, deletes the two CancelAutoplay\CLSID values, and exits. From that point native AutoPlay is no longer suppressed and there's nothing to re-write the values until the mod is toggled or settings change. So the mod can break its own core feature the first time the user picks "Open folder".

The cleanup is also redundant: RegisterCancelAutoPlay() (L3961-3966) already starts with UnregisterCancelAutoPlay(), which deletes both values before re-adding them, and it deletes them and returns when suppressNativeAutoPlay is off. Just move the L6303 call after TryBecomeAutoPlayOwner() succeeds, or drop it entirely.

2. The new fire-and-forget action thread (L4937-4953) has three problems.

auto* pOpt = new AutoPlayOption(opt);
HANDLE hWorker = CreateThread(NULL, 0, [](LPVOID param) -> DWORD { ... }, pOpt, 0, nullptr);
if (hWorker) CloseHandle(hWorker);   // fire-and-forget
  • It is never joined, so mod code can be running after Wh_ModUninit returns. Windhawk FreeLibrarys the mod the moment Wh_ModUninit returns, and this thread's body — the lambda thunk, ExecuteProgram, ExecuteReadyBoost, … — lives in the mod image. Wh_ModUninit only waits for g_hUiThread. The worker is exactly the code you moved here because it blocks for an unbounded time (ShellExecuteExW with lpVerb = L"runas" waiting on a UAC consent prompt, SHObjectProperties), so "mod disabled/updated while a UAC prompt is up" unmaps the image out from under a live thread and crashes Explorer. This is the case the wiki calls out under Worker thread: raw HANDLE: keep the handle in a global and wait on it in Wh_ModUninit (after the UI thread is joined), or — simpler here — create one long-lived worker thread with its own message loop in Wh_ModInit, post the action to it, and PostThreadMessage(WM_QUIT) + WaitForSingleObject it in Wh_ModUninit.

  • The thread never initializes COM. It previously ran on AutoPlayUiThreadProc, which does CoInitializeEx(NULL, COINIT_APARTMENTTHREADED) (L6219). A raw CreateThread has no apartment, so SHGetDesktopFolder/IShellFolder::BindToObject in OpenWpdInExplorer (L4113-4119) fail with CO_E_NOTINITIALIZED and the MTP "Open folder" action silently degrades to the shell:MyComputerFolder fallback at L4169 instead of opening the device; SHObjectProperties and the SEE_MASK_INVOKEIDLIST/SEE_MASK_IDLIST ShellExecuteExW calls have the same requirement. Add CoInitializeEx(NULL, COINIT_APARTMENTTHREADED) / CoUninitialize around the thread body. While you're there, add SEE_MASK_NOASYNC to the SHELLEXECUTEINFOW::fMask values — the thread exits immediately after the call, and without that flag the shell can cancel the operation when the calling thread goes away.

  • It reads globals the UI thread rewrites concurrently. ExecuteOpenFolder, ExecuteProgram, ExecuteReadyBoost, ExecutePlay and ExecuteViewPictures all read g_driveRoot, g_driveLetter, g_isWpd, g_driveTitle, g_wpdId, g_wpdPath, g_hwndDialog. ExecuteOptionByIndex calls DestroyWindow(hwndDlg) before starting the thread, and WM_NCDESTROY posts WMU_PROCESS_QUEUE (L5916), which can have BuildDriveDialog reassigning those same std::wstring globals for the next pending volume while the worker is reading them — a genuine data race, and at best the wrong drive gets opened. You already copy the AutoPlayOption; extend that payload struct with the root/letter/isWpd/WPD id/owner HWND and make the Execute* functions take them as parameters instead of reading globals.

3. If the mod ever loses the ownership mutex, AutoPlay stays dead for the whole session.

Wh_ModInit decides ownership once (L6309-6316) and there is no retry path: an instance that returns early never re-checks, and Wh_ModSettingsChanged only posts to g_hwndListener, which that instance doesn't have. Two ways this bites:

  • IsMainExplorerShell() (L3795) returns true when GetTrayOwnerPid() is 0. Wh_ModInit runs before the process starts executing, so during logon several explorer.exe processes can all see no tray window and race for Local\Win7ClassicAutoPlay.Owner.
  • Whoever wins holds it for as long as it lives. If that's a transient explorer.exe, it exits without Wh_ModUninit running (the OS closes the mutex handle), and the real shell process — which returned early at L6314 — never picks the ownership back up.

At minimum, retry acquisition (e.g. from Wh_ModSettingsChanged, or on a timer in a process that lost the race). The tool-mod framework solves this for free, which is the next item.

4. The tool-mod question and the registry write are still open — please leave a note for the human reviewer.

You said the conversion caused a regression on your side, which is a fair reason to stop, so I won't re-argue it in detail. But it's worth recording the state clearly, because the two open items are linked: the mod installs no function or symbol hooks at all, and the only thing that genuinely ties it to explorer.exe is the HKCU\…\AutoplayHandlers\CancelAutoplay\CLSID write (L3920-3928) — the documented IQueryCancelAutoPlay mechanism needs that key, and hooking the read (RegEnumValueW/RegQueryValueExW) instead of writing it is only possible from inside the shell process. Everything else it does (RegisterDeviceNotificationW, the WM_DEVICECHANGE broadcast, the ROT registration, ShellExecuteEx, WIC, WPD) works from any process in the session, and item 3's hand-rolled single-instance logic is exactly what the launcher in Mods as tools exists to replace (explorer-folder-hover-menu.wh.cpp has a verbatim copy of the snippet). Meanwhile the registry values still outlive the mod on any path where Wh_ModUninit doesn't run and the mod isn't loaded again (uninstalling the mod or Windhawk while Explorer is stopped), which is what the reversibility rule is about. A short comment in the PR saying which trade-off you picked and what the regression was will let the maintainer decide quickly.

5. Waiting INFINITE is the right call, but the UI thread can still take arbitrarily long to reach WMU_SHUTDOWN.

Wh_ModUninit now blocks with no timeout (L6348, L6351), which is correct — a timeout would risk unmapping the image under a live thread. The remaining exposure is that the UI thread's own message loop still runs unbounded work: ProcessPendingQueue calls ResolveWpdDeviceIPortableDeviceManager::RefreshDeviceList (L4051) on every 250 ms tick while a WPD device is pending, and that call can wedge for a long time on a misbehaving device. While it's in there, WMU_SHUTDOWN isn't dispatched and disabling the mod hangs Explorer. Doing the WPD resolution off the message-loop thread (or setting a "shutting down" flag that ProcessPendingQueue checks and bails on) keeps the shutdown path bounded. Separately, WaitForSingleObject(g_evtListenerReady, INFINITE) waits forever if the UI thread ever dies before reaching SetEvent (L6237) — WaitForMultipleObjects on {g_evtListenerReady, g_hUiThread} costs nothing and removes that case.

6. The README still has no screenshot.

Carried over from the last round. This mod's entire purpose is a specific visual dialog, so a screenshot (or a short GIF of a drive being inserted) is the single most useful thing in the README. Only i.imgur.com and raw.githubusercontent.com are allowed as image hosts.

Optional improvements

Minor polish — none of this affects users, so it's your call. Most of these are carried over from the last review.

  • Dead state. struct HotRect / g_hotRects (L3431-3432) are filled in PaintDialog and cleared, but never read — hit-testing goes through ComputeLayout/HitTestLayout. Same for g_rcCheckHit (L3439), g_rcLink (L3440), g_headerH (L3441), g_checkBottom (L3442), PendingVolume::firstTick (L3447), g_dwUiThreadId (L179) and g_bmpAutoPlay16 (L200), which is created and destroyed on every DPI change but never drawn.

  • WIN7_NATIVE_READYBOOST_BASE64 is still 1,128 lines / ~81 KB (L1750-2878) — 18% of the file — for an icon that is only a second fallback for g_bmpReadyBoost and is only ever decoded at 32 px (L3345). The other blobs are now 1-15 KB, which is the right ballpark. Re-encoding this one at the size actually used would cut ~1,100 lines and some per-DPI-change decode work.

  • The blanket try { ... } catch (...) blocks don't do what they look like they do. With the mingw-w64 Clang toolchain Windhawk uses, catch (...) catches C++ exceptions only, not access violations — so it won't protect Explorer from the failure mode you're presumably guarding against, while the code it wraps (Win32 calls, COM HRESULTs) doesn't throw in the first place. std::bad_alloc is the only realistic case. They add an indentation level and a fair amount of noise to nearly every entry point.

  • Code comments are in Italian (e.g. L153-158, L176, L183-200, L3343, L3459, L3618, L4653). Not user-facing, but English comments make the mod easier for others to maintain.

  • The QueryCancelAutoPlay registered-message handler is unreachable (L5691-5692, L6144-6145). That message is sent to the foreground window; the listener is a hidden WS_EX_NOACTIVATE tool window and the dialog doesn't exist yet when AutoPlay fires. The COM/ROT path is what actually does the work.

  • ComputeLayout() is still re-run on every mouse move. WM_MOUSEMOVE, WM_SETCURSOR (twice — directly and again inside HitTestLayout), WM_LBUTTONDOWN/UP, MoveFocus, SetDefaultFocus, ApplyDialogSize and PaintDialog each rebuild the whole layout: a std::vector, a screen DC and a DrawTextW(DT_CALCRECT) per option row. Computing it once per dialog build/resize and caching it would be simpler and cheaper.

  • ClassifyContent probes relative paths on the WPD path. BuildWpdDialog clears g_driveRoot (L5655) and then calls BuildOptionsClassifyContentLooksLikeDvdMovie(g_driveRoot) / LooksLikeBluray(g_driveRoot), which end up calling GetFileAttributesW(L"VIDEO_TS") / GetFileAttributesW(L"BDMV") — resolved against the process's current directory. Harmless in practice, but an early if (root.empty()) return false; in those helpers makes the intent explicit.

Functionality notes

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

  • The item-4 fix has a side effect: an autorun.inf on a removable drive now suppresses the content options too. BuildOptions sets g_hasProgramSection = ar.hasProgram unconditionally (L4726) and ClassifyContent returns ContentKind::Software for any volume with a program (L4472), but the program row itself is now only emitted for DRIVE_CDROM (L4735). So a USB stick full of photos that happens to carry an autorun.inf is classified as Software, skips the whole content else if chain, and offers only "Open folder" — plus the checkbox reads "Always do this for software and games:". Computing bool offerProgram = ar.hasProgram && g_driveType == DRIVE_CDROM; once and using that for g_hasProgramSection, ClassifyContent and ClassKey restores the picture/music rows.

  • Native AutoPlay is vetoed even on paths where the mod then shows nothing. AllowAutoPlay returns S_FALSE for every drive type the mod handles, but several later paths bail out without a dialog: BuildDriveDialog returns early when the volume isn't ready and isn't optical (L5573), ProcessPendingQueue drops entries after 32 tries (16 for WPD, L6033/L6063), and BuildWpdDialog returns early when the device can't be resolved. The user then gets neither the Windows toast nor the classic dialog. Falling back to allowing native AutoPlay when the mod gives up would be more forgiving.

  • LooksLikeBlankOptical treats "can't read the volume" as blank (L4366-4367) — that's also what a dirty or damaged disc looks like, so those get classified as BlankDisc.

  • Video content gets the "Music options" section header. SectionTitleForGroup maps both ContentKind::Video and ContentKind::DvdMovie to lp->musicOptions (L5025-5026), and AlwaysText maps Video to alwaysGeneral (L4500). Worth a dedicated videoOptions string, or reusing mixedOptions.

  • ExecuteViewPictures quotes the path passed to ImageView_Fullscreen (L4638-4642). Photo Viewer's rundll32 entry point is generally invoked with the path unquoted; quoted paths are known to fail on some builds. Worth testing with a path that contains spaces.

  • MonitorForDialog() follows the foreground window (L5329-5336). Windows 7 showed the AutoPlay dialog on the primary monitor; following the foreground window means the dialog can appear on a different display than the user expects when a full-screen app is running elsewhere.

  • The remembered choice is keyed by content class, not by device — a choice remembered for usb applies to every removable volume. That matches Win7's per-content-type model, just noting it since the README's "The 'Always do this' choice is remembered by the mod" doesn't say what the scope is.


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 24, 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 24, 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.


Two things are now settled, and worth recording for the human reviewer: the README has a screenshot, and the tool-mod question is closed — this revision installs real function and vtable hooks in Explorer (ShellExecute*, CreateProcess*, TrackPopupMenu*, IContextMenu, IShellFolder::GetUIObjectOf, CDefFolderMenu_Create2), so @include explorer.exe is now clearly the right call and shouldn't be re-litigated.

That new hook surface is also where nearly all of the risk below lives. It runs on Explorer's own UI threads, on the hottest paths in the shell (every popup menu, every context-menu command, every process launch), and most of it is unsynchronized. The rest of the mod — the dialog, the device queue, the policy handling — is in much better shape than this list makes it look.

1. The g_contextMenus table is unsynchronized and holds pointers to objects it doesn't own.

Four separate problems, all in the same structure:

  • Dangling pointers. GetUIObjectOfHook stores (IContextMenu*)*ppv (L4452) without an AddRef, and nothing ever erases an entry. The menu object is released as soon as the context menu closes; the address is then free to be reused by an unrelated IContextMenu, and FindContextMenu matches purely by pointer (L4411). The mod then attributes a stale drive letter to a menu that has nothing to do with that drive, and ContextInvokeHook can hijack its InvokeCommand.
  • Unbounded growth. Same code path, no removal: every drive context menu Explorer ever creates adds a permanent entry to a vector inside the shell process.
  • CDefFolderMenuCreate2Hook pushes without the lock (L4735), while GetUIObjectOfHook and WarmUpDriveContextMenus take it. Each Explorer window runs on its own thread, so this is a real concurrent push_back on a std::vector — corruption, not a theoretical race.
  • FindContextMenu returns a pointer into the vector after releasing the lock (L4407-4413). ContextQueryHook writes through it at L4549 and ContextInvokeHook reads it at L4581, both unlocked — a concurrent push_back reallocates the buffer and those become use-after-free writes.

Plus EnsureContextMenusCS() (L4400-4402) is itself a race: it is only called eagerly from InstallDriveContextMenuHooks (L4685), which on the common path never runs (item 5), so the CRITICAL_SECTION is first initialized lazily from a hook callback on an arbitrary Explorer thread. Two threads can both see !g_csContextMenusInit and both call InitializeCriticalSection on it.

Suggested shape: key the map by vtable pointer rather than by object instance — that's stable, bounded, and doesn't require owning anything. explorer-folder-hover-menu.wh.cpp does exactly this for the same IShellFolder vtable slot. If you need per-instance state, hold a real reference and drop it in a Release hook, keep everything under one lock, and copy what you need out of the entry before unlocking (never hand out a pointer into the container).

2. CreateProcess* / ShellExecute* cancel any launch whose text contains "autoplay".

IsAutoplayLaunchText (L4190-4196) is a substring match on the file and the parameters, and CreateProcessWHook (L4211-4220) turns a match into a hard failure — SetLastError(ERROR_CANCELLED); return FALSE; — plus a WMU_CONTEXT_AUTOPLAY post for a drive letter that is guessed (FindAutoplayDriveInText, else g_driveLetter). Consequences:

  • Any program Explorer launches whose path or command line happens to contain autoplayD:\AutoPlay\setup.exe, a folder or document named AutoPlay something, an installer with an /autoplay switch — is silently blocked, and an unrelated drive's dialog opens instead.
  • The mod breaks its own "View more AutoPlay options in Control Panel" link: ExecuteControlPanelLink (L5517-5520) runs control.exe with /name Microsoft.AutoPlay, which IsAutoplayLaunchText matches on Microsoft.AutoPlay. The same applies to a user opening AutoPlay from Settings or Control Panel by any other route while the mod is enabled.

Failing an arbitrary CreateProcessW inside the shell on a substring heuristic is a big blast radius for the benefit gained. Please either drop the CreateProcess* hooks entirely and rely on the IContextMenu interception (which is the precise boundary), or narrow the match hard: require the exact AutoPlay CLSID/ms-settings:autoplay token and a drive letter that the mod actually resolved, never a bare autoplay substring, and never Microsoft.AutoPlay.

3. WarmUpDriveContextMenus() runs shell-extension code for every drive at Explorer startup, and its COM pointers are misused.

Called from Wh_ModInit (L7390), so it runs before Explorer starts executing. For every eligible drive it calls IShellFolder::GetUIObjectOf(..., IID_IContextMenu, ...) (L4502), which builds a full CDefFolderMenu — that instantiates every context-menu shell extension registered for Drive (antivirus, archivers, cloud-sync clients, third-party tools), loading each of their DLLs. It also calls IsHotplugOrCardReader per drive, which is a CreateFileW + IOCTL_STORAGE_QUERY_PROPERTY on the raw volume. That is a lot of third-party code executed on Explorer's startup path, and the menus are then held in g_warmupMenus for the whole session, so those extensions stay instantiated and their DLLs pinned forever.

Two correctness problems on top of the cost:

  • The interfaces are used and stored past CoUninitialize()ApScopedCoInit (L4479) uninitializes the apartment when the function returns, while g_warmupMenus still holds the pointers.
  • They are then Release()d from Wh_ModUninit (L7448-7449), which runs on an arbitrary thread with no COM initialization. Shell context-menu objects are apartment-threaded; releasing them from a foreign, uninitialized thread is a cross-apartment call with no proxy.

Also note the comment at L4505 ("The hooked GetUIObjectOf already associated this menu") isn't accurate — Windhawk installs the hooks only when Wh_ModInit returns, so nothing is hooked yet during the warm-up.

The simplest fix is to delete the warm-up. If the mapping is keyed by vtable (item 1) it isn't needed; if you keep it, do it lazily on the mod's own COM-initialized UI thread, release everything there, and don't hold the menus for the session.

4. Two hooks do expensive I/O on Explorer's UI thread, on paths that fire constantly.

  • ContextInvokeHook calls FindContextAutoPlayDrive() (L4581) before it knows whether the invocation is even an AutoPlay command. That function loops A–Z, and for each present drive does GetDriveTypeW ×2, VolumeAllowedByPolicy (up to six RegOpenKeyEx/RegQueryValueEx pairs), IsHotplugOrCardReader for fixed drives, and GetFileAttributesW("X:\autorun.inf") (L4334). On a machine with an optical drive that last call spins up the disc; on a mapped network drive it's a blocking round-trip. This runs for every command invoked from a shell context menu.
  • TrackPopupMenuExHook / TrackPopupMenuHook call MenuContainsAutoPlay (L4290-4304) on every popup menu Explorer shows — taskbar, tray, Start, every context menu — walking the tree to depth 4 with a GetMenuStringW per item, before the menu is even displayed.

Move the eligibility check after the "is this the AutoPlay verb?" test, and cache/derive the drive letter from the menu object instead of scanning all drives. For the TrackPopupMenu* hooks, consider dropping them: with a working IContextMenu interception they're redundant, and they cost something on every menu in the shell.

5. The drive-letter mapping hook is only installed on the path that isn't normally taken.

if (!InstallAutoplayContextMenuHooks())
    InstallDriveContextMenuHooks();   // L7375-7376

GetUIObjectOfHook — described in the comments as the reliable way to learn which drive a menu belongs to — is installed only inside InstallDriveContextMenuHooks (L4666-4673). So whenever the AutoPlay COM probe succeeds (the normal case), it is never installed, g_contextMenus is populated only by CDefFolderMenuCreate2Hook/the warm-up, FindContextMenu returns null for the AutoPlay extension's own menu object, and ContextInvokeHook falls back to FindContextAutoPlayDrive() — i.e. exactly the guessing this code was written to eliminate. With two eligible removable drives and no autorun.inf, that returns 0 and the interception silently does nothing.

Install the GetUIObjectOf hook unconditionally, independently of which IContextMenu implementation you end up hooking.

Related: InstallAutoplayContextMenuHooks hooks the vtable of a COM object it creates and then releases (L4619). If CLSID {9C60DE1E-…} lives in an on-demand DLL rather than shell32, that DLL can be unloaded (CoFreeUnusedLibraries) while the hook trampolines still point into it. explorer-folder-hover-menu.wh.cpp hits the same problem and documents the reasoning; at minimum confirm which module the class lives in.

6. Several user-facing strings are hardcoded Italian.

The new option rows bypass the LangPack table entirely:

o.line1 = L"Importa brani nel Windows Media Player";   // L5787
o.line1 = L"Masterizza disco";                          // L5794-5795
o.line1 = L"Importa immagini e video";                  // L5804-5805
o.line1 = L"Avvia presentazione";                       // L5813
o.line1 = L"Sincronizza dispositivo";                   // L5822-5823

Every user, on every UI language, sees these in Italian. Windhawk mods default to English, with other languages via the localization mechanism you already have — please add these as LangPack fields.

Same issue in the menu-text matching: MenuTextIsAutoPlay (L4285-4287) and ContextQueryHook (L4545-4546) match only "autoplay", "auto play" and "riproduzione automatica", so the context-menu interception works on English and Italian Windows and silently doesn't on any other UI language. Matching on localized menu text is fragile in general — the canonical verb ("autoplay", which you already check in ContextInvocationIsAutoPlay) is the language-independent identifier.

7. The action worker still races the globals it reads.

ProcessPendingQueue is gated on g_hActionWorker (L7011), but two other paths call BuildDriveDialog/BuildWpdDialog without that gate: WMU_REBUILD (L7195-7204, posted by Wh_ModSettingsChanged) and WMU_CONTEXT_AUTOPLAY (L7213-7222, posted from the shell hooks). Either one reassigns g_driveRoot, g_driveTitle, g_driveLetter, g_driveType, g_isWpd, g_wpdId while the worker thread is reading them in ExecuteOpenFolder / ExecuteProgram / ExecutePlay / ExecuteViewPictures. Concurrent read and assignment of a std::wstring is a real data race — the worker can read a freed buffer and crash Explorer, and the benign outcome is opening the wrong drive.

You already copy the AutoPlayOption into the worker payload; extend that struct with the root, letter, drive type, WPD id and owner HWND, and make the Execute* functions take them as parameters instead of reading globals. That removes the need for the g_hActionWorker gate in ProcessPendingQueue too.

8. Wh_ModUninit deletes the CancelAutoplay values in every injected explorer.exe, not just the owner.

WriteCancelAutoPlayClsid(false);   // L7446

Wh_ModUninit runs in every process the mod was loaded into, including the secondary explorer.exe instances that returned early at L7345/L7349 and never wrote anything. This is the same failure mode as the previous round's item 1, just moved from init to uninit: on a mod update or settings-driven reload, a non-owner instance's teardown can delete the values the shell instance owns, and nothing re-writes them until the next toggle. Guard the call on actually holding the ownership mutex (or drop it — UnregisterCancelAutoPlay() on the UI thread already deletes them, and the UI thread is joined before this point).

While you're there: the README says these values "are also cleaned up unconditionally every time the mod starts, so a crash or a forced Explorer restart cannot leave them behind for more than one session". That's only true while the mod stays installed and enabled — uninstalling the mod (or Windhawk) after a crash, or with Explorer stopped, leaves them behind permanently. Worth stating that limitation accurately.

9. Attacker-controlled version-resource data reaches swprintf_s and an unbounded string read.

return std::wstring((const wchar_t*)data);              // L3845, ignores `len`
...
wchar_t buf[512];
swprintf_s(buf, ARRAYSIZE(buf), lp->publishedBy, company.c_str());   // L5673-5674

GetCompanyName builds the string by scanning for a NUL and ignores the length VerQueryValueW returned, so a crafted (or merely truncated) version resource with no terminator reads past the buffer. And swprintf_s does not truncate — on overflow it invokes the CRT's invalid-parameter handler, which in a release build terminates the process. CompanyName has no length limit, and it comes from an EXE on inserted media.

Both are cheap to fix: std::wstring((const wchar_t*)data, wcsnlen((const wchar_t*)data, len)), and either clamp company or use _snwprintf_s(..., _TRUNCATE) / build the line with std::wstring concatenation. The same swprintf_s pattern at L5666-5669 takes a filename that can be up to MAX_PATH into a 280-char buffer — also worth widening or truncating.

10. Ownership is still decided once, with no retry.

Carried over from the last round and unchanged: Wh_ModInit decides at L7345/L7349 and an instance that loses never re-checks (Wh_ModSettingsChanged posts to g_hwndListener, which a non-owner doesn't have). If Explorer is restarted while the previous process is still exiting and holding Local\Win7ClassicAutoPlay.Owner, the new shell process returns early, installs no hooks, and AutoPlay handling is dead for the rest of the session with no way back except toggling the mod. Re-attempting acquisition from Wh_ModSettingsChanged would cover the common case cheaply.

Optional improvements

Minor polish — none of this affects users, so it's your call. Several are carried over from the previous rounds.

  • Use WindhawkUtils::SetFunctionHook() instead of raw Wh_SetFunctionHook with (void*)/(void**) casts (L7354-7384, L4613-4616, L4656-4672). It's type-checked, so a signature mismatch becomes a compile error instead of a stack corruption. Also, none of the return values are checked — a hook that silently fails to install just makes a feature quietly not work.

  • Dead state. struct HotRect / g_hotRects (L3662-3663) are filled in PaintDialog and cleared, but never read — hit-testing goes through ComputeLayout/HitTestLayout. Same for g_rcCheckHit (L3670), g_rcLink (L3671), g_headerH (L3672), g_checkBottom (L3673), PendingVolume::firstTick (L3678), g_dwUiThreadId (L277) and g_bmpAutoPlay16 (L300), which is created and destroyed on every DPI change but never drawn.

  • WaitForSingleObject(g_evtListenerReady, INFINITE) (L7422) waits forever if the UI thread ever dies before reaching SetEvent (L7273) — e.g. the catch (...) at L7322 doesn't set it. WaitForMultipleObjects on {g_evtListenerReady, g_hUiThread} costs nothing and removes the hang.

  • The ShellExecuteA / ShellExecuteExA / CreateProcessA hooks are almost certainly dead weight in a modern Explorer, and CreateProcessAHook puts a 4 KB WCHAR wcmd[2048] on an arbitrary caller's stack (L4226) and doesn't check the MultiByteToWideChar result, which returns 0 without a guaranteed terminator when the command line doesn't fit.

  • The blanket try { ... } catch (...) blocks don't do what they look like they do. With the mingw-w64 Clang toolchain Windhawk uses, catch (...) catches C++ exceptions only, not access violations — so it won't protect Explorer from the failure mode you're presumably guarding against, while the code it wraps (Win32 calls, COM HRESULTs) doesn't throw in the first place. std::bad_alloc is the only realistic case.

  • Code comments are in Italian (e.g. L249-256, L266, L281-300, L3562, L3690, L3849, L3935). Not user-facing, but English comments make the mod easier for others to maintain.

  • The QueryCancelAutoPlay registered-message handler is unreachable (L6687-6688, L7162-7163). That message is sent to the foreground window; the listener is a hidden WS_EX_NOACTIVATE tool window and the dialog doesn't exist yet when AutoPlay fires. The COM/ROT path is what actually does the work.

  • ComputeLayout() is still re-run on every mouse move. WM_MOUSEMOVE, WM_SETCURSOR (twice — directly and again inside HitTestLayout), WM_LBUTTONDOWN/UP, MoveFocus, SetDefaultFocus, ApplyDialogSize and PaintDialog each rebuild the whole layout: a std::vector, a screen DC and a DrawTextW(DT_CALCRECT) per option row. Computing it once per dialog build/resize and caching it would be simpler and cheaper.

  • ClassifyContent probes relative paths on the WPD path. BuildWpdDialog clears g_driveRoot (L6651) and then calls BuildOptionsClassifyContentLooksLikeDvdMovie(g_driveRoot) / LooksLikeBluray(g_driveRoot), which end up calling GetFileAttributesW(L"VIDEO_TS") / GetFileAttributesW(L"BDMV") against the process's current directory. Harmless in practice, but an early if (root.empty()) return false; in those helpers makes the intent explicit.

Functionality notes

Non-critical observations and ideas about the feature behavior itself. Most are carried over.

  • An autorun.inf on a removable drive still suppresses the content options. BuildOptions sets g_hasProgramSection = ar.hasProgram unconditionally (L5648) and ClassifyContent returns ContentKind::Software for any volume with a program (L5302), but the program row itself is only emitted for DRIVE_CDROM (L5657). So a USB stick full of photos that happens to carry an autorun.inf is classified as Software, skips the whole content else if chain, and offers only "Open folder" — with a checkbox reading "Always do this for software and games". Computing bool offerProgram = ar.hasProgram && g_driveType == DRIVE_CDROM; once and using it for g_hasProgramSection, ClassifyContent and ClassKey restores the picture/music rows.

  • A click is silently swallowed while a previous action is still running. ExecuteOptionByIndex destroys the dialog (L5910) and then returns early if g_hActionWorker is set (L5915). If the earlier action is sitting on a UAC prompt or a slow shell call, the user's click does nothing at all, with no feedback.

  • Disabling the mod blocks until an in-flight action finishes. Wh_ModUninit joins the worker with INFINITE (L7431), which is correct — but the worker can be inside ShellExecuteExW with lpVerb = L"runas" waiting on a UAC consent prompt. There isn't a clean alternative (a timeout would unmap the image under a live thread), so this is mostly an FYI; making the worker's shell invocations non-blocking would bound it.

  • Native AutoPlay is vetoed even on paths where the mod then shows nothing. AllowAutoPlay returns S_FALSE for every drive type the mod handles, but several later paths bail out without a dialog: BuildDriveDialog returns early when the volume isn't ready and isn't optical (L6569), ProcessPendingQueue drops entries after 32 tries (16 for WPD, L7081/L7041), and BuildWpdDialog returns early when the device can't be resolved. The user then gets neither the Windows toast nor the classic dialog. Falling back to allowing native AutoPlay when the mod gives up would be more forgiving.

  • LooksLikeBlankOptical treats "can't read the volume" as blank (L5196-5197) — that's also what a dirty or damaged disc looks like, so those get classified as BlankDisc.

  • Video content gets the "Music options" section header. SectionTitleForGroup maps both ContentKind::Video and ContentKind::DvdMovie to lp->musicOptions (L6013-6014), and AlwaysText maps Video to alwaysGeneral (L5330). Worth a dedicated videoOptions string, or reusing mixedOptions.

  • ExecuteViewPictures quotes the path passed to ImageView_Fullscreen (L5468-5472). Photo Viewer's rundll32 entry point is generally invoked with the path unquoted; quoted paths are known to fail on some builds. Worth testing with a path that contains spaces.

  • MonitorForDialog() follows the foreground window (L6321-6328). Windows 7 showed the AutoPlay dialog on the primary monitor; following the foreground window means the dialog can appear on a different display than the user expects when a full-screen app is running elsewhere.


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 24, 2026
Refactor AutoPlay handling and context menu management to improve safety and reliability. Adjust string handling to prevent over-read vulnerabilities and enhance context menu association logic.
@babamohammed2022

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer windhawk-reviewer Bot added the waiting-for-ai-review An AI review was requested and is being prepared. label Aug 25, 2026
@windhawk-reviewer windhawk-reviewer Bot removed the waiting-for-author The author's turn: request an AI review, or respond to one that was posted. label Aug 25, 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.


Good progress on several fronts: the IsAutoplayLaunchText heuristic is no longer a bare "autoplay" substring match, the context-menu table is now locked and bounded, the warm-up moved off Explorer's startup path onto the mod's own COM-initialised thread, EnsureContextMenusCS() is initialised eagerly, the version-resource over-read and the swprintf_s overflow are fixed, and the "always do this" clobber on the program row is gone. Most of what's left is in the shell-hook layer, plus a few things carried over unchanged from the last round.

1. The mod owns IContextMenu objects created on other threads, and releases them from the wrong thread.

SetContextMenuLetter now AddRefs and stores the menu (L4451-4452), and entries are released in three places, none of which runs on the thread that created the object:

  • ReleaseAllContextMenuEntries() from Wh_ModUninit (L7657) — an arbitrary thread with no COM initialisation at all, after the mod's own UI thread has already called CoUninitialize() (L7510).
  • ReleaseContextMenuEntriesForLetter() from HandleVolumeRemoved (L7327) — the mod's UI thread.
  • the eviction loop in SetContextMenuLetter (L4453-4457) — whichever Explorer window thread happened to right-click.

These are apartment-threaded in-proc shell objects: each CDefFolderMenu was built on one Explorer window's own STA thread (or, for the warm-up ones, on the mod's UI thread), and it aggregates every context-menu shell extension registered for Drive. Releasing them from a foreign thread runs those extensions' destructors on a thread they were never created on — and by Wh_ModUninit the creating apartment is usually gone entirely. See Global objects and process shutdown for the general rule; this is the cross-thread-ownership case.

Two related problems in the same structure:

  • All three release paths run under g_csContextMenus. A shell extension's Release can take locks or send messages, so holding the critical section across Release() is a deadlock hazard — copy the pointers out, leave the section, then release.
  • The warm-up still instantiates every Drive context-menu shell extension. WarmUpDriveContextMenus() (L4531, called at L7481) calls GetUIObjectOf(..., IID_IContextMenu, ...) for each eligible drive, which loads and instantiates every third-party Drive extension (antivirus, archivers, cloud sync). ReleaseContextMenuWarmupMenus() (L7482) only drops the warm-up's own reference — the SetContextMenuLetter reference keeps them all instantiated for the whole session, so the comment at L4407-4409 ("kept alive for the whole session") is still accurate, just via a different owner.

The clean fix is not to own anything. IShellFolder::GetUIObjectOf, IContextMenu::QueryContextMenu and IContextMenu::InvokeCommand for a given menu all run on the same Explorer window thread, so a thread_local "last drive menu / last drive letter" pair removes the ownership, the lock, the eviction policy and the warm-up in one go. If you prefer a table, key it by vtable pointer rather than by instance — that's stable and requires owning nothing; explorer-folder-hover-menu.wh.cpp does exactly that for the same IShellFolder slot.

2. The CreateProcess* / ShellExecute* redirect hijacks attempts to open AutoPlay settings.

IsAutoplayLaunchText (L4200-4209) is much better than the old bare-substring match, but it still matches the AutoPlay Control Panel CLSID and ms-settings:autoplay — i.e. exactly the two ways a user asks for the AutoPlay settings page. RedirectAutoplayLaunch (L4211-4222) then cancels the launch and posts WMU_CONTEXT_AUTOPLAY for a drive letter that, for those launches, is never in the text — so it falls back to g_driveLetter, whatever volume the mod last handled:

int letter = FindAutoplayDriveInText(file);
if (!letter) letter = FindAutoplayDriveInText(parameters);
if (!letter) letter = g_driveLetter;      // L4216 - unrelated drive
if (RedirectAutoplayLaunch(app, cmd)) {
    SetLastError(ERROR_CANCELLED);
    return FALSE;                          // L4229-4230 - launch fails
}

So after any device has been inserted, opening AutoPlay from Explorer's Control Panel view (or anything else in Explorer that launches that CLSID) silently fails and pops the classic dialog for an unrelated drive instead. Failing an arbitrary CreateProcessW inside the shell on a text match is a large blast radius for a case the precise path already covers.

QueueContextAutoPlay (L4178-4188) is the correct boundary — it requires lpVerb == "autoplay" and an X:\ lpFile, so it can't misfire. I'd drop CreateProcessWHook/CreateProcessAHook and RedirectAutoplayLaunch entirely and keep only that.

3. Several user-facing strings are still hardcoded Italian.

Carried over unchanged from the last round — these option rows bypass the LangPack table:

o.line1 = L"Importa brani nel Windows Media Player";      // L5957
o.line1 = L"Masterizza disco";                            // L5964
o.line2 = L"utilizzando Windows Disc Image Burner";       // L5965
o.line1 = L"Importa immagini e video";                    // L5974
o.line2 = L"utilizzando Windows Photo Gallery";           // L5975
o.line1 = L"Avvia presentazione";                         // L5983
o.line1 = L"Sincronizza dispositivo";                     // L5992
o.line2 = L"utilizzando Windows Mobile Center";           // L5993

Every user on every UI language sees these in Italian. Windhawk mods default to English, with other languages through the localization mechanism you already have — please add them as LangPack fields like every other string in the dialog.

4. Only one IContextMenu implementation can ever be hooked, and the preferred one is probably the wrong class.

if (!InstallAutoplayContextMenuHooks())
    InstallDriveContextMenuHooks();        // L7570-7571

Both functions write the same two globals (g_ContextQueryOriginal / g_ContextInvokeOriginal), which is why only one can run — the comment at L7568-7569 says as much. But InstallAutoplayContextMenuHooks (L4680) hooks the vtable of CLSID {9C60DE1E-…}, the AutoPlay Control Panel item, whereas a drive's right-click menu is a CDefFolderMenu — a different class with a different vtable. So whenever that probe succeeds, the class that actually implements "Open AutoPlay…" on a drive is never hooked, ContextInvokeHook never fires for it, and the interception silently does nothing.

Key the originals by vtable pointer (a small std::unordered_map<void**, Origs>, resolved from *(void***)pThis at the top of each hook) so both implementations can be hooked without one clobbering the other's originals — explorer-folder-hover-menu.wh.cpp shows the lookup, and its HookFolderVtable also handles the second half of this problem: it pins the class' module so an on-demand shell-extension DLL can't be unloaded out from under a still-installed trampoline. Your code releases the probe object at L4703 and keeps no reference, so if that CLSID ever lives outside shell32 the hook points into memory CoFreeUnusedLibraries can free. (Pinning a third-party DLL is fine here — it's the mod's own module that must never be pinned.)

5. TrackPopupMenu* walks every popup menu Explorer shows, and only recognises two languages.

MenuContainsAutoPlay (L4303-4317) runs from both TrackPopupMenuExHook and TrackPopupMenuHook on every menu the shell displays — Start, tray, taskbar, every context menu — recursing to depth 4 with a GetMenuStringW per item, before the menu is even on screen. And MenuTextIsAutoPlay (L4294-4301) matches only "autoplay", "auto play" and "riproduzione automatica", so on any other UI language the interception silently doesn't work, while the cost is paid on every menu regardless. It can also false-positive on an unrelated Explorer menu item containing that text and swallow the command (return FALSE at L4375/L4395).

With a working IContextMenu interception (item 4) these hooks are redundant — I'd remove them. If you keep them, at minimum bail out early when !(flags & TPM_RETURNCMD), which is the only case the hook can act on anyway.

6. autorun.inf can point the icon at a UNC path, so the mod fetches a file from a remote server.

out.iconFile = MakeAbsolute(iconSpec, root);                       // L5248
if (GetFileAttributesW(out.iconFile.c_str()) == INVALID_FILE_ATTRIBUTES)
    out.iconFile.clear();

MakeAbsolute returns the value unchanged when it already starts with X: or \\ (L5153-5154), and unlike programPath (L5231-5233) the icon path is never checked with PathIsUnderRoot. So icon=\\attacker\share\x.dll,0 on inserted media makes GetFileAttributesW and then ExtractIconExW (L5747) reach out to an SMB server — a mod must be self-contained and must not contact external resources, and loading an arbitrary remote binary as an icon source is unnecessary attack surface. One-line fix, matching what you already do for the program:

out.iconFile = MakeAbsolute(iconSpec, root);
if (!PathIsUnderRoot(out.iconFile, root) ||
    GetFileAttributesW(out.iconFile.c_str()) == INVALID_FILE_ATTRIBUTES)
    out.iconFile.clear();

7. The CancelAutoplay\CLSID values still outlive the mod.

WriteCancelAutoPlayClsid(true) (L4866) writes two values under HKCU\…\AutoplayHandlers\CancelAutoplay\CLSID. They're removed on a clean unload — but Wh_ModUninit does not run when Explorer restarts, crashes, or the user signs out, and the README's claim that the startup cleanup bounds this to "one session" only holds while the mod stays installed and enabled. Uninstall the mod (or Windhawk) after a crash, or with Explorer stopped, and the values stay in the registry forever. A mod's effects have to disappear when it's disabled; persistent registry changes are the one thing Windhawk asks mods not to make.

Last round this was weighed against converting to a tool mod, and that question is now closed — the mod installs real hooks and belongs in explorer.exe. That also makes the standard answer available: hook the read instead of writing the value. The shell enumerates that key to find registered IQueryCancelAutoPlay CLSIDs, so hooking RegEnumValueW / RegQueryInfoKeyW on a handle opened to …\CancelAutoplay\CLSID and synthesising your entry gives the same effect with nothing persisted. If you'd rather keep the write, please say so explicitly in the PR and make the README's limitation accurate.

8. Ownership is still decided once, with no retry.

Unchanged from the last two rounds. Wh_ModInit decides at L7535/L7539 and an instance that loses never re-checks — Wh_ModSettingsChanged (L7668) only posts to g_hwndListener, which a non-owner doesn't have, and it installs no hooks either. If Explorer is restarted while the previous process is still exiting and holding Local\Win7ClassicAutoPlay.Owner, the new shell process returns early and AutoPlay handling is dead for the rest of the session, with no way back except toggling the mod. Retrying acquisition from Wh_ModSettingsChanged covers the common case cheaply; the hooks would need Wh_ApplyHookOperations() after being set at that point.

9. Disabling the mod can hang Explorer.

Wh_ModUninit correctly waits INFINITE (L7621, L7624), but the UI thread it's waiting on can be stuck outside its message loop:

  • ProcessPendingQueue calls ResolveWpdDeviceIPortableDeviceManager::RefreshDeviceList (L4997, and again in WpdStillPresent L5035) on every 250 ms tick while a WPD device is pending. That call can wedge for a long time on a misbehaving device, and while it's in there WMU_SHUTDOWN is never dispatched.
  • WaitForSingleObject(g_evtListenerReady, INFINITE) (L7621) waits forever if the UI thread ever exits before SetEvent — the catch (...) at L7512 doesn't set it.

A "shutting down" flag that ProcessPendingQueue/HandleVolumeArrival check and bail on bounds the first; WaitForMultipleObjects on {g_evtListenerReady, g_hUiThread} costs nothing and removes the second.

10. The embedded icon payload grew back — one blob is a single 100,048-character line.

The last commit added PHOTO_VIEWER_BASE64 (L1055) as one line of 100,048 characters — a ~75 KB, 256×256 PNG that is decoded and downscaled to 32×32 in BuildOptions on every dialog build (L5867, L5941). A single 100 k-char line is also effectively unreviewable in a diff. And WIN7_NATIVE_READYBOOST_BASE64 is still 1,130 lines / ~81 KB (L1961-3090) for something that is only the second fallback for g_bmpReadyBoost (L3565) and is only ever used at 32 px. Every other blob in the file is 1-14 KB, which is the right ballpark for a 48×48 PNG. Re-encoding these two at the sizes actually used removes ~1,100 lines and ~150 KB, and also cuts real per-dialog and per-DPI-change decode work.

Optional improvements

Minor polish — none of this affects users, so it's your call. Several are carried over from previous rounds.

  • Use WindhawkUtils::SetFunctionHook() instead of raw Wh_SetFunctionHook with (void*)/(void**) casts (L7544-7584, L4697-4700, L4739-4741, L4783-4788). It's type-checked, so a signature mismatch becomes a compile error instead of stack corruption. None of the return values are checked either — a hook that silently fails to install just makes a feature quietly not work.

  • Broken indentation around the photo-viewer icon blocks (L5867-5874 and L5941-5948) — the inserted if (customIcon) bodies sit at column 0 and g_options.push_back ends up on its own mis-indented line. Looks like a paste artifact.

  • Dead state. struct HotRect / g_hotRects (L3664-3665) are filled in PaintDialog and cleared, but never read — hit-testing goes through ComputeLayout/HitTestLayout. Same for g_rcCheckHit (L3672), g_rcLink (L3673), g_headerH (L3674), g_checkBottom (L3675), PendingVolume::firstTick (L3680), g_dwUiThreadId (L277) and g_bmpAutoPlay16 (L300), which is created and destroyed on every DPI change but never drawn.

  • ShellExecuteExWHook returns TRUE without touching the caller's SHELLEXECUTEINFOW (L4272-4276). A caller that passed SEE_MASK_NOCLOSEPROCESS will read an uninitialised hProcess, and hInstApp is left as-is. At least set sei->hInstApp = (HINSTANCE)42; and sei->hProcess = nullptr; before returning.

  • CreateProcessAHook puts a 4 KB WCHAR wcmd[2048] on an arbitrary caller's stack (L4239) and doesn't check the MultiByteToWideChar result — on a command line longer than the buffer the call fails and may leave the buffer filled without a terminator, which StrStrIW then over-reads. The ShellExecuteA/ShellExecuteExA/CreateProcessA hooks are almost certainly dead weight in a modern Explorer anyway.

  • The blanket try { … } catch (…) blocks don't do what they look like they do. With the mingw-w64 Clang toolchain Windhawk uses, catch (...) catches C++ exceptions only, not access violations — so it won't protect Explorer from the failure mode you're presumably guarding against, while the code it wraps (Win32 calls, COM HRESULTs) doesn't throw in the first place. std::bad_alloc is the only realistic case.

  • Code comments are in Italian (e.g. L249-256, L266, L281-300, L3692, L3857, L3943). Not user-facing, but English comments make the mod easier for others to maintain.

  • The QueryCancelAutoPlay registered-message handler is unreachable (L6861-6862, L7337-7338). That message is sent to the foreground window; the listener is a hidden WS_EX_NOACTIVATE tool window and the dialog doesn't exist yet when AutoPlay fires. The COM/ROT path is what actually does the work.

  • ComputeLayout() is still re-run on every mouse move. WM_MOUSEMOVE, WM_SETCURSOR (twice — directly and again inside HitTestLayout), WM_LBUTTONDOWN/UP, MoveFocus, SetDefaultFocus, ApplyDialogSize, PlaceNativeCheck and PaintDialog each rebuild the whole layout: a std::vector, a screen DC and a DrawTextW(DT_CALCRECT) per option row. Computing it once per dialog build/resize and caching it would be simpler and cheaper.

  • ClassifyContent probes relative paths on the WPD path. BuildWpdDialog clears g_driveRoot (L6825) and then calls BuildOptionsClassifyContentLooksLikeDvdMovie(g_driveRoot) / LooksLikeBluray(g_driveRoot) (L5416), which end up calling GetFileAttributesW(L"VIDEO_TS") / GetFileAttributesW(L"BDMV") against the process's current directory. Harmless in practice, but an early if (root.empty()) return false; makes the intent explicit.

  • IsSystemDriveLetter calls GetWindowsDirectoryW on every invocation (L3753) and is used inside A-Z loops (FindContextAutoPlayDrive, HandleVolumeArrival). Caching the system drive letter once would be cheaper and clearer.

Functionality notes

Non-critical observations and ideas about the feature behavior itself. Most are carried over.

  • FindContextAutoPlayDrive can still open the wrong drive even when the right one is known. When snap.letter is mapped but not eligible (e.g. blocked by policy), ContextTargetIsEligible(preferredLetter, true) fails and the function falls through to the A-Z scan (L4334-4357), which can return an unrelated volume. When the menu's own drive is known, "not eligible" should mean "do nothing", not "pick another drive".

  • A remembered "View pictures" never fires on Windows 10/11. BuildOptions deliberately no longer requires Photo Viewer to offer the row (L5856-5875), but TryExecuteRemembered still gates the remembered action on HasWindowsPhotoViewer() (L6140) — so on a machine without PhotoViewer.dll the user can tick "always do this", and the dialog reappears every time anyway.

  • An autorun.inf on a removable drive still suppresses the content options. BuildOptions sets g_hasProgramSection = ar.hasProgram unconditionally (L5800) and ClassifyContent returns ContentKind::Software for any volume with a program (L5418), but the program row itself is only emitted for DRIVE_CDROM (L5809). So a USB stick full of photos that happens to carry an autorun.inf is classified as Software, skips the whole content else if chain, and offers only "Open folder" — with a checkbox reading "Always do this for software and games". Computing bool offerProgram = ar.hasProgram && g_driveType == DRIVE_CDROM; once and using it for g_hasProgramSection, ClassifyContent and ClassKey restores the picture/music rows.

  • A click is silently swallowed while a previous action is still running. ExecuteOptionByIndex destroys the dialog (L6084) and then returns early if g_hActionWorker is set (L6089). If the earlier action is sitting on a UAC prompt or a slow shell call, the user's click does nothing at all, with no feedback.

  • Disabling the mod blocks until an in-flight action finishes. Wh_ModUninit joins the worker with INFINITE (L7630), which is correct — but the worker can be inside ShellExecuteExW with lpVerb = L"runas" waiting on a UAC consent prompt. There isn't a clean alternative (a timeout would unmap the image under a live thread), so this is an FYI; making the worker's shell invocations non-blocking would bound it.

  • Native AutoPlay is vetoed even on paths where the mod then shows nothing. AllowAutoPlay returns S_FALSE for every drive type the mod handles, but several later paths bail out without a dialog: BuildDriveDialog returns early when the volume isn't ready and isn't optical (L6743), ProcessPendingQueue drops entries after 32 tries (16 for WPD, L7215/L7255), and BuildWpdDialog returns early when the device can't be resolved. The user then gets neither the Windows toast nor the classic dialog. Falling back to allowing native AutoPlay when the mod gives up would be more forgiving.

  • LooksLikeBlankOptical treats "can't read the volume" as blank (L5312-5313) — that's also what a dirty or damaged disc looks like, so those get classified as BlankDisc.

  • Video content gets the "Music options" section header. SectionTitleForGroup maps both ContentKind::Video and ContentKind::DvdMovie to lp->musicOptions (L6187-6188), and AlwaysText maps Video to alwaysGeneral (L5446) while ClassKey stores it under "video" (L5262). Worth a dedicated videoOptions string, or reusing mixedOptions.

  • ExecuteViewPictures quotes the path passed to ImageView_Fullscreen (L5611-5615). Photo Viewer's rundll32 entry point is generally invoked with the path unquoted; quoted paths are known to fail on some builds. It's only the fallback now, but worth testing with a path that contains spaces.

  • MonitorForDialog() follows the foreground window (L6495-6502). Windows 7 showed the AutoPlay dialog on the primary monitor; following the foreground window means the dialog can appear on a different display than the user expects when a full-screen app is running elsewhere.

  • The non-shared photo-viewer bitmap isn't rebuilt on a DPI change. It's created at Scale(32) in BuildOptions (L5867) with whatever g_dpi was current, and RebindSharedIcons skips non-shared icons — so after WM_DPICHANGED it's drawn stretched from the old size while every other row's icon is re-decoded.


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 25, 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 25, 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.


Real progress this round, and two of the long-running items are now closed: nothing is written to the registry any more (the RegQueryInfoKeyW/RegEnumValueW synthesis replaces the CancelAutoplay\CLSID write, so the reversibility objection is gone), and the hardcoded Italian option rows are now LangPack fields. Also good: the CreateProcess*/ShellExecute* text-matching redirect is gone in favour of the precise lpVerb == "autoplay" + X:\ check, the TrackPopupMenu* hooks are gone, the context-menu table is now a thread_local non-owning pair (no AddRef, no lock, no cross-apartment Release, no warm-up), both IContextMenu implementations are hooked and keyed by vtable with the module pinned, the autorun.inf icon is checked with PathIsUnderRoot, and the shutdown path has a g_shuttingDown flag plus WaitForMultipleObjects.

What's left is concentrated in the shell-hook layer, which still runs on Explorer's hottest paths and now installs itself during Wh_ModInit.

1. A partially-failed hook registration leaves the hooks installed with no table entry — and then every context menu of that class fails.

bool q = Wh_SetFunctionHook((void*)query,  (void*)ContextQueryHook,  (void**)&slot.query);
bool i = Wh_SetFunctionHook((void*)invoke, (void*)ContextInvokeHook, (void**)&slot.invoke);
if (!q || !i) {
    slot = ContextMenuOrigs{};      // L3437 - entry dropped...
    return false;                   // ...but the call that succeeded stays registered
}

g_contextMenuOrigsCount is only incremented on full success (L3442), so on a partial failure the slot is invisible to OrigsForMenu while the successful hook is still applied when Wh_ModInit returns. The hook then runs and takes this path:

ContextMenuOrigs* origs = OrigsForMenu(menu);
if (!origs || !origs->query) return E_UNEXPECTED;   // L3488 (and L3542)

E_UNEXPECTED out of CDefFolderMenu::QueryContextMenu means the shell can't build that context menu — i.e. right-click is broken in Explorer, not just for drives.

There is a concrete way to get there now that both installers run (L6507-6508): HookContextMenuVtable dedupes by vtable (OrigsForMenu, L3425), but Wh_SetFunctionHook hooks the function. If the AutoPlay CPL object and CDefFolderMenu have distinct vtables whose slot 3/4 point at the same implementation, the second registration is a duplicate hook on an already-hooked target — and whatever Windhawk returns for that, the second slot is dropped while the first one's hook is live for both vtables.

Make it impossible for the hook to exist without a usable entry:

  • reserve the entry before hooking (create it, then fill it), and on failure Wh_RemoveFunctionHook the call that succeeded rather than dropping the entry — explorer-folder-hover-menu.wh.cpp does it this way (HookFolderVtable(folder, g_folderOrigs[vtable]) — the map entry is created first and never removed, so a failed hook just leaves a null trampoline that can never fire);
  • also skip a vtable whose slot-3/slot-4 addresses are already recorded in another entry, and keep those addresses in the entry so a lookup miss can fall back to matching by implementation address.

2. ContextQueryHook walks every context menu the shell builds, including the ones it has no mapping for.

bool mapped = SnapshotContextMenu(menu, &snap);          // L3491
...
if (FAILED(hr) || !menu || !hmenu || first > last) return hr;   // L3494
for (int i = 0; i < n; ++i) { ... menu->GetCommandString(offset, GCS_VERBW, ...) ... }

RememberAutoPlayOffset (L3506/L3513) is a no-op unless t_lastDriveMenu.menu == menu, so when mapped is false the entire loop is wasted work — and it isn't cheap. CDefFolderMenu is the class behind essentially every file and folder context menu in Explorer, and GetCommandString on an extension-contributed item dispatches into that extension, up to two calls per item (GCS_VERBW then GCS_VERBA), before the menu is on screen. That's the same objection as the MenuContainsAutoPlay walk you removed last round. One line:

if (FAILED(hr) || !mapped || !hmenu || first > last) return hr;

3. InstallDriveContextMenuHooks() builds a real drive context menu inside Wh_ModInit.

hr = parent->GetUIObjectOf(nullptr, 1, &child, IID_IContextMenu, nullptr, (void**)&menu);  // L3645

That constructs a full CDefFolderMenu, which instantiates every context-menu shell extension registered for Drive (antivirus, archivers, cloud-sync clients, third-party tools) and loads each of their DLLs. Wh_ModInit runs before explorer.exe starts executing, so all of that third-party code is loaded and run ahead of the shell's own initialization. This is exactly the cost WarmUpDriveContextMenus was removed for — it's the same call, just once instead of per drive. (Binding the folder is fine: explorer-folder-hover-menu.wh.cpp does SHParseDisplayName + BindToObject at init on purpose. It's GetUIObjectOf that builds the menu.)

You already hook CDefFolderMenu_Create2 (L6512-6515), so the first real menu creation hands you an IContextMenu of exactly that class. Hook its vtable there and apply once with Wh_ApplyHookOperations() under a lock — EnsureFolderHooked and its SRW-locked table are the pattern (note g_contextMenuOrigs would then need the same locking, since it would be written from an Explorer thread while other threads read it). Doing the probe on the mod's own UI thread after StartUiThread() and applying there would also work; either way it's off the startup path.

4. The suppression now depends on CancelAutoplay\CLSID already existing — please confirm it does on a clean machine.

RegQueryInfoKeyWHook / RegEnumValueWHook only fire on an already-open HKEY. If HKCU\Software\Microsoft\Windows\CurrentVersion\Explorer\AutoplayHandlers\CancelAutoplay\CLSID isn't present, the shell's RegOpenKeyEx fails, neither hook is ever reached, and native AutoPlay is never suppressed — silently, with the user getting both the Windows toast and the classic dialog. WriteCancelAutoPlayClsid (L3825-3836) only opens the key to delete values, so the mod never creates it; but on your own machines the key exists because the earlier versions of this mod created it when they wrote the values. Worth verifying on a machine that has never run the mod, and covering the open (or qualifying the README) if it isn't there by default.

Same path, worth checking while you're at it: only RegEnumValueW and RegQueryInfoKeyW are hooked. If the shell reads the value back with RegQueryValueExW/SHRegGetValueW, or enumerates with RegEnumValueA, the synthesized entry isn't visible on those paths.

5. Ownership is still decided once per process; only a settings change retries it.

The Wh_ModSettingsChanged retry (L6604-6611, with Wh_ApplyHookOperations()) is a good addition and covers the common case. What's left: Wh_ModInit runs before the new explorer.exe executes, so when Explorer restarts while the previous shell process is still exiting, the new one sees either the old tray window (IsMainExplorerShell, L6527) or the still-held Local\Win7ClassicAutoPlay.Owner mutex, returns at L6532/L6536, installs no hooks and starts no thread — and AutoPlay handling is dead for the rest of the session unless the user changes a setting or toggles the mod. Since hooks have to be registered in Wh_ModInit anyway, one option is to install them unconditionally and gate their behaviour on g_ownsAutoPlay, then re-attempt acquisition from the mod's own UI thread on a bounded schedule.

Optional improvements

Minor polish — none of this affects users, so it's your call. Several are carried over from previous rounds.

  • Is the {9C60DE1E-…} hook doing anything? InstallAutoplayContextMenuHooks (L3567) hooks the AutoPlay control panel item's IContextMenu, but the drive's "Open AutoPlay…" verb is invoked on the CDefFolderMenu, which is a different class. If it never fires, dropping it removes a CoCreateInstance from Wh_ModInit and one of the two vtables that can collide in item 1.

  • Dead state. struct HotRect / g_hotRects (L2738-2739) are filled in PaintDialog and cleared but never read — hit-testing goes through ComputeLayout/HitTestLayout. Same for g_rcCheckHit (L2746), g_rcLink (L2747), g_headerH (L2748), g_checkBottom (L2749), g_firstGeneralIdx (L2714), PendingVolume::firstTick (L2754), g_dwUiThreadId (L277), g_cancelRegWritten (L3823, now write-only), and g_bmpAutoPlay16 (L300), which is created and destroyed on every DPI change but never drawn. ApScopedHandle (L153) and ApScopedCriticalSection (L234) are no longer used by anything.

  • QueueContextAutoPlayA doesn't check MultiByteToWideChar (L3264-3265). When the ANSI verb or path doesn't fit, the call returns 0 and may leave the buffer filled without a terminator, which _wcsicmp/GetAutoPlayVerbDrive then over-reads. wfile[MAX_PATH] is reachable with a long path. The ShellExecuteA/ShellExecuteExA hooks are almost certainly dead weight in a modern Explorer anyway.

  • ShellExecuteExWHook returns TRUE without touching the caller's SHELLEXECUTEINFOW (L3284-3288). A caller that passed SEE_MASK_NOCLOSEPROCESS reads an uninitialised hProcess, and hInstApp is left as-is. At least set sei->hInstApp = (HINSTANCE)42; sei->hProcess = nullptr; first.

  • AutoPlayUiThreadProc continues to CreateWindowExW when RegisterClassW fails (L6407-6412). If the class ever survives a previous load (ERROR_CLASS_ALREADY_EXISTS), the window is created against a stale class whose lpfnWndProc points into an unmapped image. ShowAutoPlayDialog (L5589-5592) already gets this right by returning; the listener should do the same.

  • ShouldSpoofCancelClsid costs an NtQueryKey syscall plus a heap allocation on every RegEnumValueW/RegQueryInfoKeyW call in Explorer (L3729-3752) once the owner instance is up. Comparing the suffix in place against info->Name (and bailing early on NameLength) instead of building a std::wstring removes the allocation.

  • Use WindhawkUtils::SetFunctionHook() for the plain exports (L6492-6503, L6514) instead of raw Wh_SetFunctionHook with (void*)/(void**) casts — it's type-checked, so a signature mismatch is a compile error instead of stack corruption. None of the return values are checked either, so a hook that fails to install just makes a feature quietly not work.

  • Broken indentation around the photo-viewer icon blocks (L4815-4822 and L4889-4896) — the if (customIcon) bodies sit at column 0 and g_options.push_back ends up on its own mis-indented line. Carried over; looks like a paste artifact.

  • BuildWpdDialog's g_contentKind = ContentKind::Portable is dead (L5790): BuildOptions immediately overwrites it with ClassifyContent(...) (L4750), which for a WPD device sees an empty inventory and returns ContentKind::Empty. Related, carried over: ClassifyContent calls LooksLikeDvdMovie(g_driveRoot)/LooksLikeBluray(g_driveRoot) (L4364) with g_driveRoot cleared (L5773), so GetFileAttributesW(L"VIDEO_TS") is resolved against the process's current directory. An early if (root.empty()) return false; makes the intent explicit.

  • The blanket try { … } catch (…) blocks don't do what they look like they do. With the mingw-w64 Clang toolchain Windhawk uses, catch (...) catches C++ exceptions only, not access violations — so it won't protect Explorer from the failure mode you're presumably guarding against, while the code it wraps (Win32 calls, COM HRESULTs) doesn't throw in the first place. std::bad_alloc is the only realistic case.

  • Code comments are in Italian (e.g. L249-256, L266, L274-300, L2636, L2766, L2931, L3017). Not user-facing, but English comments make the mod easier for others to maintain.

  • The QueryCancelAutoPlay registered-message handler is unreachable (L5809-5810, L6294-6295). That message is sent to the foreground window; the listener is a hidden WS_EX_NOACTIVATE tool window and the dialog doesn't exist yet when AutoPlay fires. The COM/ROT path is what actually does the work.

  • ComputeLayout() is still re-run on every mouse move. WM_MOUSEMOVE, WM_SETCURSOR (twice — directly and again inside HitTestLayout), WM_LBUTTONDOWN/UP, MoveFocus, SetDefaultFocus, ApplyDialogSize, PlaceNativeCheck and PaintDialog each rebuild the whole layout: a std::vector, a screen DC and a DrawTextW(DT_CALCRECT) per option row. Computing it once per dialog build/resize and caching it would be simpler and cheaper.

  • IsSystemDriveLetter calls GetWindowsDirectoryW on every invocation (L2827) and is used inside A-Z loops (FindContextAutoPlayDrive, HandleVolumeArrival). Caching the system drive letter once would be cheaper and clearer.

Functionality notes

Non-critical observations and ideas about the feature behavior itself. Most are carried over.

  • t_lastDriveMenu matches by raw address, and a non-drive menu doesn't clear it. CDefFolderMenuCreate2Hook (L3700-3704) and GetUIObjectOfHook (L3468-3477) only set the association; when no drive letter is derived they leave the previous one in place. A freed drive menu's address can be reused by an unrelated IContextMenu on the same thread, which then inherits that drive letter. It can only misfire on a menu that also carries an autoplay verb, so the blast radius is "wrong drive opened", but clearing the entry when *outMenu/*ppv matches and no letter was found closes it.

  • An autorun.inf on a removable drive still suppresses the content options. BuildOptions sets g_hasProgramSection = ar.hasProgram unconditionally (L4748) and ClassifyContent returns ContentKind::Software for any volume with a program (L4366), but the program row itself is only emitted for DRIVE_CDROM (L4757). So a USB stick full of photos that happens to carry an autorun.inf is classified as Software, skips the whole content else if chain, and offers only "Open folder" — with a checkbox reading "Always do this for software and games". Computing bool offerProgram = ar.hasProgram && g_driveType == DRIVE_CDROM; once and using it for g_hasProgramSection, ClassifyContent and ClassKey restores the picture/music rows.

  • A remembered "View pictures" never fires on Windows 10/11. BuildOptions deliberately no longer requires Photo Viewer to offer the row (L4804-4808), but TryExecuteRemembered still gates the remembered action on HasWindowsPhotoViewer() (L5088) — so on a machine without PhotoViewer.dll the user can tick "always do this" and the dialog reappears every time anyway.

  • A click is silently swallowed while a previous action is still running. ExecuteOptionByIndex destroys the dialog (L5032) and then returns early if g_hActionWorker is set (L5037). If the earlier action is sitting on a UAC prompt or a slow shell call, the user's click does nothing, with no feedback.

  • Disabling the mod blocks until an in-flight action finishes. Wh_ModUninit joins the worker with INFINITE (L6575), which is correct — but the worker can be inside ShellExecuteExW with lpVerb = L"runas" waiting on a UAC consent prompt. There isn't a clean alternative (a timeout would unmap the image under a live thread), so this is an FYI; making the worker's shell invocations non-blocking would bound it.

  • FindContextAutoPlayDrive() still scans A-Z when the menu isn't mapped (L3324-3339), doing GetDriveTypeW, up to six policy registry reads, IsHotplugOrCardReader (a CreateFileW + IOCTL_STORAGE_QUERY_PROPERTY on the raw volume) and GetFileAttributesW("X:\autorun.inf") per drive — that last one spins up an optical disc. It only runs on an actual AutoPlay invocation now, so the cost is bounded, but it can still pick an unrelated volume.

  • Native AutoPlay is vetoed even on paths where the mod then shows nothing. AllowAutoPlay returns S_FALSE for every drive type the mod handles, but several later paths bail out without a dialog: BuildDriveDialog returns early when the volume isn't ready and isn't optical (L5691), ProcessPendingQueue drops entries after 32 tries (16 for WPD, L6172/L6212), and BuildWpdDialog returns early when the device can't be resolved. The user then gets neither the Windows toast nor the classic dialog. Falling back to allowing native AutoPlay when the mod gives up would be more forgiving.

  • LooksLikeBlankOptical treats "can't read the volume" as blank (L4260-4261) — that's also what a dirty or damaged disc looks like, so those get classified as BlankDisc.

  • Video content gets the "Music options" section header. SectionTitleForGroup maps both ContentKind::Video and ContentKind::DvdMovie to lp->musicOptions (L5135-5136), and AlwaysText maps Video to alwaysGeneral (L4394) while ClassKey stores it under "video" (L4210). Worth a dedicated videoOptions string, or reusing mixedOptions.

  • ExecuteViewPictures quotes the path passed to ImageView_Fullscreen (L4559-4563). Photo Viewer's rundll32 entry point is generally invoked with the path unquoted; quoted paths are known to fail on some builds. It's only the fallback now, but worth testing with a path that contains spaces.

  • MonitorForDialog() follows the foreground window (L5443-5450). Windows 7 showed the AutoPlay dialog on the primary monitor; following the foreground window means the dialog can appear on a different display than the user expects when a full-screen app is running elsewhere.

  • The non-shared photo-viewer bitmap isn't rebuilt on a DPI change. It's created at Scale(32) in BuildOptions (L4815, L4889) with whatever g_dpi was current, and RebindSharedIcons skips non-shared icons (L4659) — so after WM_DPICHANGED it's drawn stretched from the old size while every other row's icon is re-decoded.


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 25, 2026
@babamohammed2022

Copy link
Copy Markdown
Contributor Author

I have tested on Windows 10 21H2 and Windows 11 24H2 and it seems to work fine in practice. Without being able to reproduce #1, #2, #3, touching that code could risk regressions on working systems. If the maintainer believes that #4 and #5 need to be addressed, I'll try to fix them.
I'm open to fix the reported problems if they maintainer requests it or if a user opens an issue.
/ready-for-reviewer

@windhawk-reviewer windhawk-reviewer Bot added waiting-for-reviewer Ready for a human reviewer, and in the queue for one. and removed waiting-for-author The author's turn: request an AI review, or respond to one that was posted. labels Aug 25, 2026
@m417z
m417z merged commit a9855f7 into ramensoftware:main Aug 30, 2026
5 checks passed
@windhawk-reviewer windhawk-reviewer Bot removed the waiting-for-reviewer Ready for a human reviewer, and in the queue for one. label Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants