1809542 - Crash in [@ RtlpWaitOnCriticalSection | RtlpEnterCriticalSectionContended | RtlEnterCriticalSection | PR_Lock | PR_EnterMonitor | mozilla::ReentrantMonitor::Enter]

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P3)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/a3a49d7b-cee1-4521-af41-0041d0230110

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

0  ntdll.dll  RtlpWaitOnCriticalSection  
1  ntdll.dll  RtlpEnterCriticalSectionContended  
2  ntdll.dll  RtlEnterCriticalSection  
3  nss3.dll  PR_Lock  nsprpub/pr/src/threads/combined/prulock.c:215
3  nss3.dll  PR_EnterMonitor  nsprpub/pr/src/threads/prmon.c:139
4  xul.dll  mozilla::ReentrantMonitor::Enter  xpcom/threads/ReentrantMonitor.h:74
4  xul.dll  mozilla::ReentrantMonitorAutoEnter::ReentrantMonitorAutoEnter  xpcom/threads/ReentrantMonitor.h:176
4  xul.dll  mozilla::layers::AutoCompleteTask::AutoCompleteTask  gfx/layers/ipc/SynchronousTask.h:62
4  xul.dll  mozilla::widget::WinCompositorWindowThread::ShutDownTask  widget/windows/WinCompositorWindowThread.cpp:113
5  xul.dll  mozilla::detail::runnable_args_base<0>::Run  dom/media/webrtc/transport/runnable_utils.h:41

Looks like we're doing something wrong during shutdown, possibly trying to grab a lock that's uninitialized (but not dead, I don't see evidence of an UAF).

Comment 1

[Ray Kraesig [:rkraesig]]

Ouch! No, this is indeed a UAF. When this timeout falls through, we proceed blithely on our way out of the function -- the associated comment describes "leaking memory", but the task on whose completion we're waiting is constructed on the stack, so this leaves a pointer-to-stack-garbage in the compositor's task queue.

Out of an abundance of caution, I'm elevating this to a sec bug.

Comment 2

[Ray Kraesig [:rkraesig]]

(ni?ing :bradwerth as author of the regressing bug, since I'm not sure whether that gets done automatically for sec bugs.)

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1798652

Comment 4

[Ray Kraesig [:rkraesig]]

(Addendum: there's now a "Regresses:" link on 1798652 pointing here. I may have opened the metaphorical barn door while trying to secure it.)

Comment 5

[Andrew McCreight [:mccr8]]

It looks like this crash is in the main process during shutdown, so there shouldn't be any content-controlled code running at this point, so I don't think it is particularly exploitable.

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1809542: Remove stack-allocated param to runnable.

This ensures the param is still alive when the task runs, even if the
thread object has been deallocated.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1798652

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Remove stack-allocated param to runnable. r=rkraesig
https://hg.mozilla.org/integration/autoland/rev/0b661055a25ef73ff81c812b2fe671935aebd5f7
https://hg.mozilla.org/mozilla-central/rev/0b661055a25e

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:bradwerth, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox110 to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Brad Werth [:bradwerth]]

This should be uplifted, but not until we solve the expected intermittent test failures (from the memory leaks) in Bug 1811634. Uplifting now would just push the intermittents into beta and create a lot of confusion for build sherrifs.

Comment 11

[Brad Werth [:bradwerth]]

The fixups in Bug 1811634 and Bug 1423833 are complex enough that it would be somewhat risky to uplift all pieces. Since this is a UAF on shutdown and therefore very difficult to exploit, I think this fix can wait.