Skip to content

Windows 7 Blocked Shutdown UX & Logoff Sequence 1.0 - #5215

Open
Ingan121 wants to merge 39 commits into
ramensoftware:mainfrom
Ingan121:main
Open

Windows 7 Blocked Shutdown UX & Logoff Sequence 1.0#5215
Ingan121 wants to merge 39 commits into
ramensoftware:mainfrom
Ingan121:main

Conversation

@Ingan121

@Ingan121 Ingan121 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor
  • This is a direct port of my AuthUX BSDR fork.
  • Tested on Windows 10 LTSC 2021 and 11 25H2.

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): (Code review only)

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

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

@Ingan121

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

Something went wrong while preparing the AI review, so there is nothing to post this time. This is a problem on the reviewer's side, not with this pull request.

Comment /ai-review to try again.

@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
@Ingan121

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

Something went wrong while preparing the AI review, so there is nothing to post this time. This is a problem on the reviewer's side, not with this pull request.

Comment /ai-review to try again.

@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
@Ingan121

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

Something went wrong while preparing the AI review, so there is nothing to post this time. This is a problem on the reviewer's side, not with this pull request.

Comment /ai-review to try again.

@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
Added known issues section and important usage note for the mod.
@Ingan121

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

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

Comment /ai-review again after 2026-08-26 16:59 UTC (in 23 hours) to get another one.

@Ingan121

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 30, 2026
@windhawk-reviewer

Copy link
Copy Markdown

Submission review

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

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

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


The previous round is resolved: the settings-change path no longer re-runs the probe for settings winlogon doesn't read, TerminateProcess on the probe child is gone, the message loop now dispatches when hDlg is NULL, disableAsyncLogoff defaults to false with the auto-logon caveat in its $description, g_hDoModalExitEvent is an std::atomic that's cleared when DoModal returns, the window class is created with HINST_THISCOMPONENT, DWLP_MSGRESULT is set in the dialog proc, and the wheel/thumbtrack/quadratic-layout notes are all fixed. I also re-checked the pieces that are easy to get wrong and they hold up: every hardcoded bitmap blob is exactly 40 + w*h*4 bytes with a matching BITMAPINFOHEADER (no overread on the hardcoded path), the DLGITEMTEMPLATEEX<N> title arrays are sized to the exact string length + terminator so the compiler's tail padding lands precisely where the template format wants its DWORD alignment, the [[clang::no_destroy]] shapes and their releases in Wh_ModUninit are right, the thread is joined and the class unregistered, and the settings block matches the code. Only one item left in this section.

1. The winlogon probe still runs inline on winlogon.exe's startup path.

Wh_ModAfterInitIsLogonUiInjectionEnabled()CreateProcessW(LogonUI.exe) + WaitForSingleObject(pi.hProcess, 2000). Per the mod lifetime chart, when a mod is loaded before the target process starts, Wh_ModInit/Wh_ModAfterInit run on that process's main thread before it begins executing — so on every boot this creates a LogonUI.exe while winlogon.exe itself hasn't initialized yet, and holds winlogon's main thread until the child exits (or 2 s elapses, after which the child is simply abandoned).

I raised the probe design twice already and you've kept it, which is your call — I'm not re-litigating that. But the one concrete piece from that discussion is still open and is cheap: g_fShutdownResolverDisabled is only consulted at shutdown time, so the probe doesn't have to finish before winlogon starts running. Move it off the startup path:

static HANDLE g_hProbeThread, g_hProbeStopEvent;

void Wh_ModAfterInit() {
    if (!g_isWinlogon || !p_g_fShutdownResolverDisabled) return;
    g_origResolverDisabledState = *p_g_fShutdownResolverDisabled;
    if (g_origResolverDisabledState) return;
    g_hProbeStopEvent = CreateEventW(nullptr, TRUE, FALSE, nullptr);
    g_hProbeThread = CreateThread(nullptr, 0, [](LPVOID) -> DWORD {
        if (IsLogonUiInjectionEnabled())      // waits on {pi.hProcess, g_hProbeStopEvent}
            *p_g_fShutdownResolverDisabled = 1;
        return 0;
    }, nullptr, 0, nullptr);
}

with the matching signal + WaitForSingleObject(g_hProbeThread, INFINITE) + CloseHandle in Wh_ModUninit, so the mod stays unloadable (same shape as the existing CustomBSDR::hThread, no [[clang::no_destroy]] needed — see Global objects and process shutdown). Waiting on {pi.hProcess, stopEvent} rather than the process alone also means an unload during the 2 s window doesn't block.

Optional improvements

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

  • maxHeight isn't clamped to minHeight in UpdateAppListLayout, unlike the same computation in WM_INITDIALOG (lines 2760–2761). On a short screen at a high scale factor — e.g. 768 px at 175 %, where screenHeight - MulDiv(335, dpi, 96) lands below the template's list height — the layout path shrinks the list below minHeight and drags the controls under it upward, while the init path would have kept minHeight. Adding the same two lines keeps the two paths consistent:

    int maxHeight = screenHeight - MulDiv(335, dpi, 96);
    if (maxHeight < minHeight)
        maxHeight = minHeight;
  • Uninitialized RECTs when the API fails. CenterWindow uses rcWindow/rcWorkArea without checking GetWindowRect / SystemParametersInfoW(SPI_GETWORKAREA, ...), and UpdateAppListLayout does GetWindowRect(hScrollBar, &rcScrollBar) with no null guard on hScrollBar (every other use of it in that function is guarded or harmless, but scrollBarWidth would be garbage). Both only matter with a malformed external resource DLL, but the checks are one line each.

  • Module reference leaked on one Wh_ModInit failure path. When HookSymbols(g_hBlockedShutdownDll, ...) fails, the code frees g_hLogonControllerDll and returns FALSE, but not g_hBlockedShutdownDll — and Wh_ModUninit doesn't run after a FALSE init, so that reference is never released.

  • g_hDoModalExitEvent can still be signalled after the event is closed. In the IDCANCEL handler, Resolve() runs first and can cause DoModal to return on the other thread (which is exactly the outcome it's asking for), after which LogonUI may CloseHandle the event before the SetEvent(hDoModalExitEvent) two lines later — signalling whatever kernel object that handle value now names. DuplicateHandle the captured event into a mod-owned handle at capture time (and CloseHandle it in Wh_ModUninit) and the recycling window disappears entirely; signalling an already-abandoned event is harmless.

  • SYMBOL_HOOK entries that don't want the original. windhawk_utils.h has a nullptr_t overload (windhawk_utils.h#L45-L54) that takes the hook as a typed Prototype*. Passing bare nullptr instead of (void**)nullptr selects it and lets you drop the (void*) casts, which currently discard all type checking on nine hook signatures:

    { { L"public: virtual long __cdecl BlockedShutdownUXImpl::Hide(void)" },
      nullptr, BlockedShutdownUXImpl_Hide_hook, FALSE },
  • <regex> for one literal substitution. regex_replace(titleFormat, std::wregex(L"%d"), ...) still builds a fresh std::wregex on every layout update to replace a fixed two-character token; a plain find/replace on the std::wstring keeps the "don't feed a DLL-supplied format string to swprintf" property and drops a heavy header.

  • The bitmap blobs are still ~1,240 lines of opaque hex. Flagging only so the maintainer's original request isn't lost — the dialog template was converted to a readable struct, but RES_BSDR_SEPARATOR / RES_BSDR_BTN_* remain unreviewable binary. For what it's worth they do validate structurally (correct headers, exact sizes, no padding), and the five button skins are 35×28 BGRA with large flat runs, so a small RLE or a procedural generator would cut about a third of the file.

Functionality notes

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

  • get_ScaleFactor hard-returns 100 while the rest of the dialog scales with GetDpiForWindow. The scale factor is what the backend uses to decide how big the get_Icon stream should be, so on a 150 %/200 % display you'll get a 100 %-sized icon dropped into a MulDiv(32, dpi, 96) slot. It won't stretch to fill either: tile.hIcon is a plain SS_BITMAP static without SS_REALSIZECONTROL, so after UpdateAppListLayout resizes the control the bitmap is drawn at its native size and clipped, not scaled — which is probably also why the SetStretchBltMode(HALFTONE) in WM_CTLCOLORSTATIC has no visible effect today. Either return the real scale (MulDiv(dpi, 100, 96)) or add SS_REALSIZECONTROL to the icon static; worth an explicit check at 200 % since it's the kind of thing that only shows up on a scaled display.

  • Every backend refresh rebuilds the tiles. Because the BSDR backend re-sends the full app list as AddApplication when the blocking app changes, the new dedup in CreateAppTileControls destroys and recreates the icon/title/reason windows (and re-decodes the icon bitmap) for every app that was already on screen. WS_EX_COMPOSITED hides most of it, but updating a matching tile in place instead of RemoveAppTileControls + recreate would avoid the churn on a long list.

  • CustomBSDR::hDesktop is only closed in Wh_ModUninit. After the BSDR closes normally, the input-desktop handle stays open for the life of the LogonUI process. Harmless in practice (the process is about to end, and Start is one-shot anyway), just noting it since the rest of the teardown is thorough.


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.

@Ingan121

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer

Copy link
Copy Markdown

Submission review

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

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

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


The mod is in good shape — the lifecycle handling is correct (worker thread signalled and joined in Wh_ModUninit, UnregisterClass on the same thread that registered it, [[clang::no_destroy]] std::optional<> for pendingApps with an explicit reset(), LOAD_LIBRARY_SEARCH_SYSTEM32 on every LoadLibraryEx, resource-only flags for the user-supplied DLL, settings block in sync with the code). One item worth fixing:

The LogonUI.exe injection probe can leave an orphaned process behind. In IsLogonUiInjectionEnabled(), when the wait times out the mod just closes the handles and hopes the probe exits on its own:

DWORD result = WaitForSingleObject(pi.hProcess, 2000);
if (result != WAIT_OBJECT_0) {
    // Should exit immediately but just to be safe
    Wh_Log(L"LogonUI wait timed out or failed");
    // Let it exit automatically
}

The "auto-exit on an unknown argument" behaviour is undocumented and could change in a future build. If it ever does, you're left with a stray LogonUI.exe running as SYSTEM on the Winlogon desktop, spawned from winlogon at every session start — which is exactly the kind of thing that can wedge the logon screen on a critical-process path. Since the mod owns this process and it has nothing to clean up, terminate it instead of leaving it:

DWORD result = WaitForSingleObject(pi.hProcess, 2000);
if (result != WAIT_OBJECT_0) {
    Wh_Log(L"LogonUI load check timed out (GLE=%u), terminating the probe", GetLastError());
    TerminateProcess(pi.hProcess, 0);
}
Optional improvements

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

  • RedrawWindow with a null hDlg repaints the desktop, not nothing. RedrawWindow(NULL, ...) targets the desktop window, so the two unguarded calls — the WM_SYSKEYDOWN branch in ThreadProc and WM_DISPLAYCHANGE in WndProc — would invalidate the whole desktop rather than no-op. This is reachable: when CreateDialog* fails in WM_CREATE the code bails out with hBgWnd alive and hDlg == nullptr. Add if (hDlg) around both (the neighbouring CenterWindow/UpdateAppListLayout calls already self-guard).

  • hDlg / hBgWnd are read from the LogonUI thread without the synchronisation used elsewhere. AddApplication/RemoveApplication correctly take pendingAppsMutex before touching hDlg, but BlockedShutdownUXImpl_get_ScaleFactor_hook reads it bare, and CustomBSDR::Hide() reads hBgWnd (written on the BSDR thread). Worst case is a stale HWND — GetDpiForWindow then returns 0 and *scaleFactor becomes 0. Either take the same lock in get_ScaleFactor, or make hBgWnd/hDlg std::atomic<HWND>.

  • WM_ADD_APP leaks a reference when the dialog closes first. AddApplication does AddRef() + PostMessageW, and the handler Attaches it — but if the dialog is destroyed before the posted message is dispatched, the message is discarded and the reference is never released. Draining the queue for WM_ADD_APP in the WM_CLOSE path (PeekMessageW(&msg, hDlg, WM_ADD_APP, WM_ADD_APP, PM_REMOVE) before DestroyWindow) would balance it.

  • <regex> for a single %d substitution. UpdateAppListLayout builds a std::wregex on every layout pass just to replace %d; that pulls a fairly heavy header (and locale machinery) into a mod that runs in LogonUI.exe. A wcsstr/std::wstring::find + replace does the same job and lets you drop the #include <regex>.

  • Hardcoded TEB offset. OFFSET_SAME_TEB_FLAGS = 0x17EE is an undocumented, version-specific layout detail. If it ever moves you silently read an unrelated bit and take the wrong branch in IsLogonUiInjectionEnabled(). It's only a diagnostic path, so it's not urgent, but a comment noting which builds it was verified against (or dropping the check and relying on the LogonUiLoadCheck fallback alone) would age better.

  • CoCreateInstance(CLSID_WICImagingFactory2, ...) per app icon. GetBitmapFromRandomStream creates a fresh WIC factory for every blocking app. Creating it once on the BSDR thread after CoInitializeEx and passing it down would be cheaper and simpler.

  • External-DLL layout path assumes every control exists. In UpdateAppListLayout (and WM_INITDIALOG), GetWindowRect(hTitleText, &rcTitle) / GetWindowRect(hNoButton, &rcNo) are unchecked — if a user-supplied winsrv.dll template is missing a control, GetDlgItem returns NULL, the RECTs stay uninitialised and newDialogHeight becomes garbage. A single up-front check that all GetDlgItem results are non-null (falling back to the hardcoded template if not) would make the external-resource path safe against a wrong DLL.

  • IsAuthUxInstalled() is broader than its name. It returns true for any non-stock BlockedShutdownResolverUX DLL path, not just AuthUX. That's probably the behaviour you want, but a comment or a name like IsThirdPartyBsdrInstalled() would avoid confusion later.

  • Typo in the Korean description for noSafetyChecks: 검사힙니다검사합니다.

Functionality notes

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

  • The CreateEventW capture is a heuristic. Grabbing the first unnamed, auto-reset, non-signalled event created on the DoModal thread identifies the exit event only by shape. If a future LogonController.dll creates a different event first on that thread, SetEvent fires on the wrong object with unpredictable results in LogonUI. You've documented that it works on LTSC 2021 and 25H2, so this is just an FYI — but if there's a way to narrow it (e.g. only capturing during a specific window of DoModal, or sanity-checking the handle before signalling it), it would be worth it.

  • ExitProcess(0) fallback in the IDCANCEL handler. Hard-killing LogonUI from a mod is drastic, and the preceding Sleep(1000) blocks the BSDR UI thread. The comment explains why it's there and it's only reached when the event capture failed and the BSDR was forced onto the default desktop, so it's a reasonable last resort — just flagging that this is the path most likely to surprise a user if the capture ever regresses.

  • Internal failures resolve as Force. Start called twice, CreateThread failing, RegisterClassExW/CreateWindowExW failing, and dialog creation failing all end in Resolve(BlockedShutdownResolution_Force), which discards unsaved work in the blocking apps. The comment argues this matches what Windows does when the BSDR is unavailable, which is fair — but a Cancel on the recoverable failures (e.g. Start called a second time) would be less destructive than forcing.

  • Cross-apartment use of IShutdownBlockingApp. The interface pointers arrive on the LogonUI thread and are used (get_Caption, get_Icon, …) on the BSDR STA thread, and pendingApps.reset() releases them from the Windhawk engine thread. That's only correct if the objects are agile/free-threaded. It evidently works, so this is likely fine — worth keeping in mind if you ever see odd RPC_E_WRONG_THREAD failures.

  • The no-password auto-relogon known issue. It's documented in the README, which is the right call for now. If you ever track down the cause it'd be worth revisiting, since silently logging back in is a fairly surprising side effect of enabling the option.

  • The probe costs one extra LogonUI.exe launch per session start (plus up to 2s on the Windhawk init thread in winlogon). Given the constraint you describe in the comment above IsLogonUiInjectionEnabled() there doesn't seem to be a cleaner alternative, so this is just an FYI.


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.

@Ingan121

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer

Copy link
Copy Markdown

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

Comment /ai-review again after 2026-08-31 03:47 UTC (in 1 hour) to get another one.

@Ingan121

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer

Copy link
Copy Markdown

Submission review

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

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

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


The item from last round is fixed — the probe child is terminated instead of being left running — and the new hardening in this round is good: the WM_INITDIALOG control-presence check makes the external-resource path safe against a wrong winsrv.dll, the WIC factory is created once per thread and released on its owning thread, the <regex> dependency is gone, and the unguarded RedrawWindow/GetWindowRect calls are all guarded now. Two things came in with the synchronisation change, though.

1. CustomBSDR::Hide() holds pendingAppsMutex across a cross-thread ShowWindow — that can deadlock the session end (lines 3472-3477).

void CustomBSDR::Hide() {
    std::lock_guard lock(pendingAppsMutex);
    if (hBgWnd) {
        ShowWindow(hBgWnd, SW_HIDE);
    }
}

Hide() runs on LogonUI's thread (BlockedShutdownUXImpl::Hide hook), but hBgWnd belongs to the BSDR thread. ShowWindow on another thread's window is synchronous — it sends WM_WINDOWPOSCHANGING/WM_WINDOWPOSCHANGED/WM_SHOWWINDOW to the owning thread and blocks until they're processed (that's exactly why ShowWindowAsync exists). Your own WndProc handles WM_WINDOWPOSCHANGING, so the BSDR thread definitely has to run for the call to return.

Meanwhile the BSDR thread takes pendingAppsMutex in WM_CLOSE (line 3208) and in WM_INITDIALOG (line 2724). So on the ordinary resolution path — Resolve() → LogonController calls Hide() on its thread while the BSDR thread picks up the WM_CLOSE that IDYES/IDCANCEL posted — you get:

  • LogonUI thread: acquires pendingAppsMutex, blocks inside ShowWindow waiting for the BSDR thread.
  • BSDR thread: enters WM_CLOSE, blocks on pendingAppsMutex.

Neither ever proceeds: the dialog is never destroyed, Wh_ModUninit would block forever on its WaitForSingleObject(hThread, INFINITE), and the user is left with a full-screen topmost dimmed window and a logoff that never completes. This is the lock-and-SendMessage pattern described in Development tips — the rule is to copy what you need out of shared state, release the lock, then make the cross-thread call.

Note the lock doesn't buy anything here either: hBgWnd is written without it (WM_CLOSE, line 3225, and the assignment at line 3316), so Hide() is synchronising against a variable nobody else locks. The previous round's suggestion offered std::atomic<HWND> as the alternative, and that's the one that fits hBgWnd:

static std::atomic<HWND> hBgWnd = nullptr;

void CustomBSDR::Hide() {
    HWND hBgWndLocal = hBgWnd.load();
    if (hBgWndLocal) {
        ShowWindow(hBgWndLocal, SW_HIDE);   // no lock held
    }
}

(ShowWindow on a handle that was destroyed in the meantime is a harmless no-op.) GetScaleFactor() can keep the mutex — GetDpiForWindow doesn't send any messages — or read an std::atomic<HWND> hDlg the same way.

2. get_ScaleFactor now reports raw DPI instead of a percentage (lines 3463-3470, 3499).

int CustomBSDR::GetScaleFactor() {
    std::lock_guard lock(pendingAppsMutex);
    if (hDlg) {
        return GetDpiForWindow(hDlg);
    } else {
        return GetDpiForSystem();
    }
}
...
*scaleFactor = CustomBSDR::GetScaleFactor();

The MulDiv(dpi, 100, 96) that used to be in the hook was dropped when this moved into a helper, so the backend now gets 96 at 100 %, 144 at 150 %, 192 at 200 % — where it expects 100 / 150 / 200 (the usual DEVICE_SCALE_FACTOR convention). That's the value the BSDR backend uses to decide how large an icon stream to hand back through get_Icon, so every scale ends up asking for a slightly-wrong size, and 96 isn't a valid scale factor at all. Restore the conversion in the helper (which also makes it match its name):

int CustomBSDR::GetScaleFactor() {
    std::lock_guard lock(pendingAppsMutex);
    int dpi = hDlg ? GetDpiForWindow(hDlg) : GetDpiForSystem();
    return MulDiv(dpi, 100, 96);
}
Optional improvements

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

  • DestroyWindow(hWndDlg) from inside WM_INITDIALOG (line 2718) leaves the dialog manager working on a destroyed window: after your handler returns, CreateDialogParamW's internals still do the default-focus SetFocus and the WS_VISIBLE ShowWindow on that HWND. The !IsWindow(hDlgLocal) guard you added in WM_CREATE catches the outcome, but signalling failure without self-destructing is cleaner — e.g. set a static bool g_dlgInitFailed in WM_INITDIALOG, return FALSE, and let WM_CREATE do the DestroyWindow + fall back to the hardcoded template.

  • The WM_ADD_APP drain dereferences lParam unchecked (line 3217), while the WM_ADD_APP handler itself guards with if (app). if (msg.lParam) reinterpret_cast<IShutdownBlockingApp*>(msg.lParam)->Release(); keeps the two consistent.

  • GetModuleHandleW(L"kernelbase.dll") isn't checked before GetProcAddress (lines 3944-3946); the null flows into SetFunctionHook. It can't realistically fail, but the log line would then say "hook failed" rather than "module not found".

  • LoadAlphaBitmap returns nullptr when CreateDIBSection fails (line 2100) instead of falling back to the hardcoded resource the way every other failure path in the function does, so a transient GDI failure on the external path silently drops that element into the high-contrast drawing path.

Functionality notes

Non-critical observations about the feature behavior itself — mostly carried over so nothing gets lost.

  • The CreateEventW capture remains a heuristic (first unnamed, auto-reset, non-signalled event created on the DoModal thread). You've documented that it holds on LTSC 2021 and 25H2, and duplicating the handle removed the recycling hazard, so this is just an FYI: if a future LogonController.dll creates a different event first on that thread, SetEvent fires on the wrong object.

  • ExitProcess(0) is still the fallback when the capture fails (line 2976), preceded by Sleep(1000) on the BSDR UI thread. It should be rare now that the event path exists, and the comment explains the reasoning — flagging it only because it's the path most likely to surprise a user if the capture ever regresses.

  • Internal failures resolve as Force (Start called twice, CreateThread/RegisterClassExW/CreateWindowExW/dialog creation failing). Matching what Windows does when the BSDR is unavailable is defensible, but Cancel on the recoverable ones (a second Start, in particular) would cost a retry instead of unsaved work.

  • Mixed-DPI multi-monitor is worth an explicit test. The background spans the virtual screen from one GetDC(nullptr) screenshot while the dialog metrics come from GetDpiForWindow(hDlg) and maxHeight from SM_CYSCREEN; a 100 % + 200 % pair is the configuration most likely to show a mis-scaled dim layer or a clipped dialog. Fixing item 2 above changes what icon sizes the backend hands you, so this is a good time to check it.

  • The probe costs one extra LogonUI.exe launch per session start (and one per noSafetyChecks change), inline on winlogon's startup path. You've explained the constraint that rules out the passive alternative, so this is an FYI for the human reviewer rather than a change request.


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.

…checks

Added a noInvoke parameter to the Resolve function to control invocation behavior. Updated various static variables to non-static for better thread safety and mutex handling. Enhanced dialog initialization checks and improved resource management.
@Ingan121

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer

Copy link
Copy Markdown

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

Comment /ai-review again after 2026-08-31 14:36 UTC (in 6 hours) to get another one.

@Ingan121

Copy link
Copy Markdown
Contributor Author

/ai-review

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


Overall this is a well-built mod: the worker thread is properly signalled and joined in Wh_ModUninit, the window class is unregistered on the thread that registered it, the [[clang::no_destroy]] globals all have matching explicit releases, cross-thread state is guarded by mutexes without any lock-and-SendMessage patterns, and the DLLs are loaded with LOAD_LIBRARY_SEARCH_SYSTEM32. A few things below.

1. Failing to create the dialog leaves a full-screen black window on screen. In WndProc's WM_CREATE the bail-out path returns 0:

} else {
    // Bail out
    Wh_Log(L"Dialog creation failed!!");
    Resolve(BlockedShutdownResolution_Force);
    return 0;
}

Returning 0 from WM_CREATE means "continue creating the window", so CreateWindowExW succeeds, hBgWnd is stored, WM_BSDR_SETFOCUS shows it and forces it to the foreground — but bgBitmap was never created (that block is after the return), so WM_PAINT draws nothing and the class BLACK_BRUSH leaves an opaque, topmost, full-screen black window covering everything until Stop() arrives. Return -1 instead, so CreateWindowExW fails and ThreadProc takes its existing CreateWindowExW failed path (which unregisters the class and force-resolves).

2. The "previous thread still running" bail-out in CustomBSDR::Start doesn't actually resolve. Resolve() returns early when g_resolvedValue != BlockedShutdownResolution_None, and g_resolvedValue is only reset further down in Start — which this path never reaches:

        lock.unlock();
        Resolve(BlockedShutdownResolution_Force);   // no-op: still holds the previous session's value
        return;

So the intended "force" never happens; the new session ends up resolved with whatever the previous session resolved to, via the stale value that add_Resolved_hook replays (e.g. a Cancel from the previous shutdown attempt silently cancels the new one). Reset the resolution state before the early return:

{
    std::lock_guard lock(g_resolvedMutex);
    g_resolvedValue = BlockedShutdownResolution_None;
    g_wasClicked = false;
}
lock.unlock();
Resolve(BlockedShutdownResolution_Force);

3. Icon bitmaps are deleted while the controls still reference them. In WM_CLOSE:

for (auto& tile : appTiles) {
    if (tile.hIconBitmap) DeleteObject(tile.hIconBitmap);
}
appTiles.clear();
...
DestroyWindow(hDlgLocal);

The SS_BITMAP statics were given those handles with STM_SETIMAGE and are still alive at that point, so between the DeleteObject and DestroyWindow the controls hold dangling GDI handles (and a repaint in that window would draw with a freed/possibly-recycled object). RemoveAppTileControls already gets this right — destroy the windows first, then delete the bitmap. Simplest fix is to move the loop below DestroyWindow(hDlgLocal).

4. Provenance of the embedded bitmap resources. RES_BSDR_SEPARATOR / RES_BSDR_BTN_* (~1,300 lines) are, per the comment, the original bitmaps extracted from Windows 7's winsrv.dll, embedded in a file that carries a GPL-3.0-only header. Worth confirming with the maintainer that shipping Microsoft resource data this way is acceptable for the catalog. The separator is a 50×3 horizontal alpha gradient and the buttons are 35×28 nine-slice skins — both are simple enough to generate procedurally at load time, which would also make the data reviewable (m417z's earlier question), which the external-DLL path already covers for users who want pixel-exact originals.

Optional improvements

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

  • #include <shellapi.h> doesn't appear to be used anywhere; <cstdlib> also looks unused (the C-library symbols actually used come from <cstring>, <climits> and <cstdint>).
  • thread_local bool g_isInitialThread = false; is declared but never read or written — leftover from an earlier approach.
  • Wh_ModSettingsChanged(BOOL* bReload) never assigns *bReload. The engine reads it to decide whether to reload the mod, so set it explicitly (*bReload = FALSE;) rather than relying on the caller's initialization. The BOOL variant is genuinely needed here since you use the FALSE return to request an unload.
  • Wh_ModSettingsChanged calls Wh_ModAfterInit() directly. Reusing a lifecycle callback as a helper is confusing; pull the body into e.g. ApplyResolverDisabledState() and call that from both.
  • The SYMBOL_HOOK entries use the legacy (void**)nullptr / (void*)Hook casts, which erase the prototype. Windhawk 1.7.1+ has a strong-typed overload that takes a bare nullptr, so { {L"..."}, nullptr, BlockedShutdownUXImpl_Start_hook, false } gets you compile-time signature checking for free. (The logonControllerDllHooks entry already benefits from this via &CLogonController__DoModal_orig.)
  • BlockedShutdownUXImpl_AddApplication_hook builds a std::wstring just to log it. Wh_Log takes printf-style arguments directly, so a single Wh_Log(L"AddApplication, AppId=%u, IsBlocking=%d, Caption=%s, BlockReason=%s", ...) is simpler and avoids the allocations.
  • IsLogonUiInjectionEnabled uses raw new wchar_t[] / delete[] with manual cleanup on four error paths; a std::wstring (or std::vector<wchar_t>) would make the early returns leak-proof.
  • GetProcAddress(kernelBase, "CreateEventW") result isn't null-checked before being passed to SetFunctionHook.
  • CreateDialogParamW(g_hResDll, ..., DlgProc, lParam) passes WM_CREATE's LPCREATESTRUCT as the dialog's dwInitParam. DlgProc never reads it, so passing 0 would be clearer.
  • hTitleFont / hDescFont are set to nullptr twice in the WM_CLOSE handler (right after DeleteObject, then again in the handle-reset block below).
  • LoadImageWithWIC and ConvertWICBitmapToHBITMAP copy the raw IWICImagingFactory* into a local ComPtr purely to null-check it (an extra AddRef/Release round-trip); a plain if (!pWICImagingFactory) return E_ABORT; does the same. Similarly GetBitmapFromRandomStream takes its stream by ComPtr value where a raw pointer would do.
  • Wh_Log(L"CoCreateInstance(CLSID_WICImagingFactory2) failed, HR=%d", hrIf) — HRESULTs read better as %08X.

Functionality notes

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

  • ExitProcess(0) in Cancel(). Terminating LogonUI.exe outright is a heavy hammer, and it skips all mod teardown. It's well commented and only reachable when the session end was forced onto the default desktop and the DoModal event capture failed, so there may be no better option — just flagging that if the CreateEventW capture ever regresses, this becomes the live path.
  • The DoModal exit event is captured heuristically. CreateEventW_hook grabs the first unnamed, auto-reset, non-signalled event created on the DoModal thread. If a future build creates a different such event first, the mod will SetEvent on an unrelated object. A tighter filter (e.g. only accepting the capture if it happens within the first N events, or additionally checking the return address against LogonController.dll's range) would fail more predictably.
  • Resolve() always sets g_wasClicked = true, including for the programmatic resolutions (dialog creation failure, thread start failure, Wh_ModUninit). get_WasClicked then tells LogonUI the user clicked when they didn't. Consider only setting it from the actual IDCANCEL/IDYES paths.
  • pendingApps isn't cleared between BSDR sessions. After WM_CLOSE nulls hDlg, any late AddApplication goes into pendingApps and stays there; a subsequent Start() in the same LogonUI process would show those stale entries in the new dialog. Clearing it in Start (or in the WM_CLOSE drain, next to the WM_ADD_APP drain) closes that.
  • IsLogonUiInjectionEnabled details. I see the approach was already discussed and settled, so just two small things: the check is a substring match over the whole pipe-separated value, so an unrelated entry containing logonui.exe passes; and it only looks at Include, not at the Exclude value or at the mod's own per-mod process inclusion/exclusion override — in either of those cases the logoff-sequence feature silently does nothing with no indication to the user. Since the mod already has a settings-level escape hatch (noSafetyChecks), a Wh_Log line naming the exact reason would at least make it diagnosable.
  • UpdateAppListLayout shifts siblings by walking GW_HWNDNEXT from hScrollBar. That's exact for the built-in template, but with a user-supplied resource DLL whose dialog has a different control order it will move the wrong controls. Matching by control ID instead would be robust against that.
  • The background screenshot isn't retaken on WM_DISPLAYCHANGE — you note this in the code and it matches Windows 7's behavior, so it's fine; just be aware that on a resolution increase the dimmed area will no longer cover the whole virtual screen.


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.

@Ingan121

Copy link
Copy Markdown
Contributor Author

/ai-review

@windhawk-reviewer

Copy link
Copy Markdown

Submission review

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

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

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


The mod is in good shape after the previous rounds — the settings block matches the code exactly, the window class is registered/unregistered per session, all the [[clang::no_destroy]] globals have matching explicit releases, both LoadLibraryExW calls use LOAD_LIBRARY_SEARCH_SYSTEM32, the user-supplied resource DLL is loaded as a datafile only, and the hand-built DLGTEMPLATEEX is correctly sized and aligned. Four things left:

1. The ~1,300 lines of hex bitmap data are still unreviewable (lines 326-1622). This is the maintainer's open inline question on this PR ("Is there a way to include this data in textual form to make these parts of the mod reviewable?"), which hasn't been answered yet. It's ~20 KB of opaque bytes in a mod that runs as SYSTEM in LogonUI.exe, and a human reviewer currently has no way to tell what's in it. Two concrete reductions that don't change what's rendered:

  • RES_BSDR_SEPARATOR (50x3, line 326) can be a 50-byte alpha table. Every pixel in it is ff ff ff <alpha> — pure white with a varying alpha — and DrawSeparator only ever samples row y = 1 (all three AlphaBlend calls pass source y = 1, height = 1, lines 2171-2180), so rows 0 and 2 are dead data. The whole 640-byte blob collapses to:

    // Win7 separator: pure white, horizontal alpha ramp (row 1 of the 50x3 original)
    static constexpr uint8_t kSeparatorAlpha[50] = { 0,0,0,0, 1,2,3,6,9,0x0d, ... };

    plus a few lines that build the DIB — reviewable at a glance.

  • The five 35x28 button bitmaps only need to be 9 px wide. Each row is [4 left-border pixels][one color repeated 27x][4 right-border pixels] — e.g. line 390: 00 00 00 76, a0 99 8b e2, f7 dc 9f c6, f1 c1 58 a3, then f0 bb 46 9c repeated, then the mirror. DrawButton already 9-slices with borderSize = 4 and stretches the middle column horizontally (lines 2280-2292), so cropping the source to 9 px wide (4 + 1 + 4) renders identically while cutting those five blobs from ~19.6 KB to ~5 KB.

    Between the two, that's roughly a 75% cut. If you'd rather keep byte-exact originals, please at least add a comment above each array stating the dimensions, pixel format and provenance, and mention where the data was extracted from — right now RES_BSDR_BTN_SELECTED_HOVER is indistinguishable from arbitrary bytes.

2. The LogonUI-readiness safety check can't detect the failure it exists for, and depends on Windhawk's internal registry layout (IsLogonUiInjectionEnabled, lines 3924-4005). The check parses HKLM\Software\Windhawk\Engine\Settings\Include for the string LOGONUI.EXE. That tells you the user edited the inclusion list — not that the mod actually initialized in LogonUI.exe. Concretely, if HookSymbols for Windows.UI.BlockedShutdown.dll fails (symbol server unreachable, a future build where BlockedShutdownUXImpl is renamed, etc.), Wh_ModInit returns FALSE at line 4088 and the LogonUI side is gone — but winlogon still sees LOGONUI.EXE in the registry, sets g_fShutdownResolverDisabled = 1, and the user gets exactly the hang your noSafetyChecks description warns about: the stock BSDR on the invisible secure desktop. The same hole applies to a mod-specific exclusion for LogonUI.exe, which the global-include check explicitly can't see (as your own comment at line 3922 notes).

Windhawk's per-mod storage is shared across processes and is a documented API, so it can carry the real signal instead:

// LogonUI.exe, at the end of Wh_ModInit once the hooks are in:
Wh_SetIntValue(L"logonUiHooksReady", 1);
// ...and Wh_SetIntValue(L"logonUiHooksReady", 0) on each LogonUI-side bail-out
// (keep it 1 for the AuthUX path at line 4050 — the logoff sequence works there).

// winlogon.exe, in IsLogonUiInjectionEnabled():
if (!Wh_GetIntValue(L"logonUiHooksReady", 0)) {
    return false;
}

This covers the inclusion list, mod-specific exclusions, symbol-resolution failures and AuthUX in one check, and drops the dependency on Windhawk's internal registry keys. mods/classic-min-max-animations.wh.cpp#L377 uses the same storage across processes. The portable-Windhawk service probe (lines 3973-4004) is orthogonal and can stay as-is.

3. The unload path resolves the shutdown after the mod's own hooks are already gone (Wh_ModUninit, lines 4154-4180). Windhawk removes the mod's hooks when Wh_ModBeforeUninit returns, so by the time Wh_ModUninit runs, Resolve(BlockedShutdownResolution_Cancel) at line 4172 invokes LogonUI's handler, and every follow-up call it makes — Hide, get_WasClicked, Stop — lands on the real BlockedShutdownUXImpl, whose Start was never called because you replaced it. Per your own comment at line 3672, the real get_WasClicked returning false makes winlogon lock the session after a cancel; and Hide/Stop run against an object that was never started. So disabling/updating the mod while a blocked-shutdown dialog is up can lock the session or destabilize LogonUI.

The fix is just to move the handshake one callback earlier, where the hooks are still installed:

void Wh_ModBeforeUninit() {
    if (g_isWinlogon) {
        return;
    }

    g_isExiting.store(true);

    using namespace CustomBSDR;

    HANDLE hThreadLocal = nullptr;
    {
        std::lock_guard lock(workerMutex);
        hThreadLocal = hThread;
        hThread = nullptr;
    }

    if (hThreadLocal) {
        bool needsResolve;
        {
            std::lock_guard lock(g_resolvedMutex);
            needsResolve = g_resolvedValue == BlockedShutdownResolution_None;
        }
        if (needsResolve) {
            Resolve(BlockedShutdownResolution_Cancel);
        }
        if (hStopEvent) {
            SetEvent(hStopEvent);
        }
        WaitForSingleObject(hThreadLocal, INFINITE);
        CloseHandle(hThreadLocal);
    }
}

Wh_ModUninit then keeps only the handle/library teardown (hDesktop, g_hDoModalExitEventDup, pendingApps.reset(), hStopEvent, g_Resolved.Reset(), the three FreeLibrary calls) — the thread is already joined, so the mod is still fully unloadable when it returns.

4. Uninitialized HSTRINGs in BlockedShutdownUXImpl_AddApplication_hook (lines 3682-3691). caption and blockReason are declared uninitialized and the HRESULTs are discarded, so if get_Caption or get_BlockReason fails without writing its out-param, WindowsGetStringRawBuffer and WindowsDeleteString run on a stack garbage value — an access violation in LogonUI.exe mid-shutdown. CreateAppTileControls (lines 2439-2457) already gets this right; the logging hook should match:

HSTRING caption = nullptr, blockReason = nullptr;
blockingApp->get_Id(&appId);
blockingApp->get_IsBlocking(&isBlocking);
HRESULT hrCaption = blockingApp->get_Caption(&caption);
HRESULT hrReason = blockingApp->get_BlockReason(&blockReason);
Wh_Log(L"BlockedShutdownUXImpl::AddApplication, AppId=%u, IsBlocking=%d, Caption=%s, BlockReason=%s",
       appId, isBlocking,
       SUCCEEDED(hrCaption) ? WindowsGetStringRawBuffer(caption, nullptr) : L"<failed>",
       SUCCEEDED(hrReason) ? WindowsGetStringRawBuffer(blockReason, nullptr) : L"<failed>");
WindowsDeleteString(blockReason);
WindowsDeleteString(caption);

Note that Wh_Log's arguments are only evaluated when logging is enabled, so moving the WindowsGetStringRawBuffer calls into the Wh_Log call also stops them from running on every AddApplication in normal use.

Optional improvements

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

  • resDllPath silently does nothing until the mod is toggled. The comment at line 4228 explains the reasoning, but a user who sets the path and sees no change has no way to know they need to disable/re-enable the mod. Since the LogonUI process is idle outside a session-end, you could set *bReload = TRUE in Wh_ModSettingsChanged when the path actually changed and no BSDR thread is running (hThread == nullptr under workerMutex), and fall back to the current behavior otherwise.

  • IsHighContrast() calls SystemParametersInfoW on every draw (line 1971) — it's invoked from DrawSeparator, DrawButton, WM_CTLCOLORDLG, WM_CTLCOLORSTATIC and AppListSubclassProc, so a single repaint makes a dozen SPI round-trips. Caching the value and refreshing it on WM_SETTINGCHANGE/WM_THEMECHANGED would be cheaper and no less correct.

  • WM_SETTINGCHANGE is swallowed unconditionally (lines 3148-3153): the handler returns TRUE for every wParam, telling the dialog manager the message was handled even when it wasn't. Move the return TRUE inside the if (wParam == SPI_SETWHEELSCROLLLINES) block and break otherwise.

  • Sleep(700) on the UI thread in the SetTimer failure fallback (line 3019) freezes the dialog for 700 ms. It's an unlikely path, but MsgWaitForMultipleObjectsEx with a timeout, or just calling Cancel() immediately, would avoid the freeze.

Functionality notes

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

  • The DoModal exit-event capture is a heuristic (CreateEventW_hook, lines 3857-3884): it grabs the first anonymous, auto-reset, initially-non-signaled event created on the thread while g_enteringDoModal is set. If a future build creates any other such event earlier inside CLogonController::DoModal, the mod would SetEvent on the wrong object. You've noted it works on LTSC 2021 and 25H2 and it's only used on the non-secure-desktop path, so there may be nothing better available — worth keeping in mind if reports of odd LogonUI behavior show up. If CLogonController::DoModal has a nearby symbol that exposes the event (a member field, a WaitForSingleObject call site), resolving it directly would be more robust than the loader-level heuristic.

  • ExitProcess(0) in Cancel() (line 2740) is a heavy hammer for a mod, even as a documented last-resort fallback. Now that the event capture is in place it should essentially never be reached; you may want to log loudly (rather than silently exiting) if it ever is, so users can report it.

  • The safety check is evaluated once, in Wh_ModAfterInit (line 4141). A user who follows the readme and adds LogonUI.exe to the inclusion list afterwards won't get the classic logoff sequence until winlogon's copy of the mod is reloaded. Worth a line in the readme's usage note ("after adding LogonUI.exe, disable and re-enable the mod"), since the mod will otherwise appear to silently ignore the setting. Point 2 above would make the check accurate but doesn't change when it runs.

  • Nice touch: replacing %d manually instead of passing a DLL-supplied string to swprintf (lines 2709-2714) correctly avoids a format-string bug with untrusted resource DLLs. Same for the bitmap header validation in LoadAlphaBitmap (lines 2039-2073) — the uint64_t arithmetic there keeps width * height * 4 from overflowing later.


Next steps:

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

See the review process for details.

@Ingan121

Copy link
Copy Markdown
Contributor Author
  1. The maintainer only mentioned the dialog resource which was previously also hardcoded as hex blob. This mod is GPL licensed because it includes codes from the GPLed upstream AuthUX (LogonController interfaces and WIC handler), which already contains proprietary Microsoft bitmaps. If the maintainer decides embedding MS images is inappropriate, I'd rather just make the high contrast rendering the default than using inaccurately generated resources. (I'll consider compressing them though.)
  2. LogonUI injection check method has changed over and over and over during the review process; the AI is now suggesting using the initial method again which was quickly abandoned due to not checking the live status and being one step behind. (One logoff after configuring properly will not have the old sequence restored, and one other logoff after improper config change will result in that undesired stuck in secure desktop behavior.) I also tried writing specific data when called with some args, and executing LogonUI with that arg during probe to check the live injection status, but it costs one process exec and took around 0.5 seconds. So I settled with just checking the global inclusion registry, as this mod isn't even compatible with portable. Also note: applying the global inclusion setting fully restarts Windhawk and reloads every mods; there's no need to inform to disable and reenable the mod.

Another note: LogonUI just exits when idle. It isn't even running most of the time; more likely to be not running when the user changes the mod settings from the WH UI.

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

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants