Closed Bug 1937798 Opened 2 months ago Closed 2 months ago

Crash in [@ GetForegroundWindow]

Categories

(External Software Affecting Firefox :: Other, defect)

defect

Tracking

(firefox133 wontfix, firefox134+ wontfix, firefox135 fixed)

RESOLVED FIXED
135 Branch
Tracking Status
firefox133 --- wontfix
firefox134 + wontfix
firefox135 --- fixed

People

(Reporter: yannis, Assigned: yannis)

Details

(Keywords: topcrash, topcrash-startup)

Crash Data

Attachments

(1 file)

This seems to be content/utility/socket crashes with Win11 24H2 (10.0.26100) when using winhook64.dll from "Shanghai EISOO Information Technology Corp." or "AISHU Technology Corp.". Example crash here.

Top frames in stack:

0 	user32!GetForegroundWindow
1 	winhook64
2 	winhook64
3 	ntdll!RtlDosPathNameToNtPathName_U_WithStatus
4 	KERNELBASE!GetFileAttributesW
5 	winhook64
6 	KERNELBASE!CreateFileInternal
7 	KERNELBASE!CreateFileW
8 	xul!OpenFile(nsTString<char16_t> const&, int, int, bool, PRFileDesc**)

Although it only triggers with a third-party DLL, the root cause might be a Microsoft bug. The code for GetForegroundWindow appears to have changed with 24H2, which makes me wonder if we might be able to repro locally just by calling into that function at startup (without a third-party DLL). While this function used to just call into NtUserGetForegroundWindow, it does more things now:

    user32!GetForegroundWindow:
7ffae19a44a0 mov     qword ptr [rsp+8], rbx
7ffae19a44a5 mov     qword ptr [rsp+10h], rsi
7ffae19a44aa push    rdi
7ffae19a44ab sub     rsp, 20h
7ffae19a44af call    user32!Feature_GetForegroundWindowInUserMode__private_IsEnabledDeviceUsageNoInline (7ffae19a445c)
7ffae19a44b4 xor     ebx, ebx
7ffae19a44b6 test    eax, eax
7ffae19a44b8 je      user32!GetForegroundWindow+0x7e (7ffae19a451e)
7ffae19a44ba mov     rcx, qword ptr [user32!gpsi (7ffae19e3478)]
7ffae19a44c1 call    qword ptr [user32!__imp_ABI_Get_ForegroundWindow (7ffae19bee38)]
7ffae19a44c8 nop     dword ptr [rax+rax]
7ffae19a44cd mov     rsi, rax
7ffae19a44d0 test    rax, rax
7ffae19a44d3 je      user32!GetForegroundWindow+0x7a (7ffae19a451a)
7ffae19a44d5 mov     rdi, qword ptr gs:[30h]
7ffae19a44de cmp     qword ptr [rdi+820h], rbx
7ffae19a44e5 jne     user32!GetForegroundWindow+0x56 (7ffae19a44f6)
7ffae19a44e7 lea     ecx, [rbx+0Eh]
7ffae19a44ea call    qword ptr [user32!__imp_NtUserGetThreadState (7ffae19bf620)]
7ffae19a44f1 nop     dword ptr [rax+rax]
7ffae19a44f6 movzx   ecx, si
7ffae19a44f9 call    user32!_HANDLEENTRY_FROM_INDEX (7ffae19455e0)
7ffae19a44fe test    rax, rax
7ffae19a4501 je      user32!GetForegroundWindow+0x7a (7ffae19a451a)
7ffae19a4503 mov     rcx, qword ptr [rax+10h]
7ffae19a4507 mov     rdx, qword ptr [rdi+820h]
7ffae19a450e cmp     qword ptr [rdx], rcx
7ffae19a4511 cmove   rbx, rsi
7ffae19a4515 mov     rax, rbx
7ffae19a4518 jmp     user32!GetForegroundWindow+0x8a (7ffae19a452a)
7ffae19a451a xor     eax, eax
7ffae19a451c jmp     user32!GetForegroundWindow+0x8a (7ffae19a452a)
7ffae19a451e call    qword ptr [user32!__imp_NtUserGetForegroundWindow (7ffae19bf468)]
7ffae19a4525 nop     dword ptr [rax+rax]
7ffae19a452a mov     rbx, qword ptr [rsp+30h]
7ffae19a452f mov     rsi, qword ptr [rsp+38h]
7ffae19a4534 add     rsp, 20h
7ffae19a4538 pop     rdi
7ffae19a4539 ret     

The code after 7ffae19a44de seems quite suspicious: we compare [rdi+820h] to NULL (since rbx is 0), but what we do after that in the NULL case is a bit weird:

  • at 7ffae19a44ea we call NtUserGetThreadState but don't use the result of this call;
  • then we reach into the same code path as when [rdi+820h] is not NULL -- a code path that derefs [rdi+820h] at 7ffae19a450e.

It almost feels as though there was an intention to use the result of NtUserGetThreadState in place of [rdi+820h], maybe.

Crash Signature: [@ GetForegroundWindow) → [@ GetForegroundWindow]

I am indeed able to reproduce this crash on Win11 24H2 by forcing a call to GetForegroundWindow from NS_InitXPCOM.

The bug is linked to a topcrash signature, which matches the following criteria:

  • Top 20 desktop browser crashes on release (startup)
  • Top 10 content process crashes on release (startup)
  • Top 5 socket and utility process crashes on release (startup)

For more information, please visit BugBot documentation.

(93.70% in signature vs 00.84% overall) Module "winhook64.dll" = true [93.85% vs 11.79% if platform_version = 10.0.26100]

Starting with Windows 11 24H2, calling GetForegroundWindow from
user32.dll in a win32k locked-down Firefox process causes crashes. We
have observed this as third-party injected DLLs call this function. We
can leverage the sandbox interceptors to nullify GetForegroundWindow
just in win32k locked-down processes, where user32.dll functionality is
obviously very limited anyway.

Assignee: nobody → yjuglaret
Status: NEW → ASSIGNED

Playing with prefs confirms that this only crashes in win32k locked down processes (no lockdown = no crash). The attached patch fixes the issue locally. Credits to :bobowen for the idea and pointers.

Pushed by yjuglaret@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e5bb64b4fb1c Nullify GetForegroundWindow in win32k locked-down processes. r=bobowen
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 135 Branch

[Tracking Requested - why for this release]:
If this isn't fixed by Microsoft it looks like it is likely to increase as more people upgrade.
Should we consider the fix for a dot release, if it continues/increases?

Tracking for 134, that's a big patch for a dot release but let's evaluate it mid-cycle for the planned dot release after we have confirmed that we are no longer crashing in 135 beta.

Although we still see the crash signature and the patch seems effective on 135 beta, the volume of crashes and affected installations since we shipped 134 and 134.0 dropped a lot compared to the 133 and 132 cycles which might indicate a fix being rolled out by MS. It. Let's play safe and have it ship with 135 in 2 weeks. Thanks.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: