1741452 - Crash in [@ mozilla::widget::UpdateOcclusionStateRunnable::Run]

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

4 crashes from 4 different installations but only for two different internet hosts. Oldest reported installation is 96.0a1 20211109190508, occlusion got enabled for Windows in bug 1732736 shortly before.

Crash report: https://crash-stats.mozilla.org/report/index/f3f83dbe-ca13-4ef5-8282-186b30211114

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll mozilla::widget::UpdateOcclusionStateRunnable::Run widget/windows/WinWindowOcclusionTracker.cpp:100
1 xul.dll mozilla::widget::SerializedRunnable::Run widget/windows/WinWindowOcclusionTracker.cpp:120
2 xul.dll mozilla::widget::SerializedTaskDispatcher::HandleTasks widget/windows/WinWindowOcclusionTracker.cpp:297
3 xul.dll mozilla::detail::runnable_args_base<mozilla::detail::NoResult>::Run dom/media/webrtc/transport/runnable_utils.h:41
4 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:771
5 xul.dll mozilla::TaskController::ProcessPendingMTTask xpcom/threads/TaskController.cpp:391
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1175
7 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:107
8 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:324
9 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:306

Comment 1

[Sotaro Ikeda [:sotaro]]

Bug 1741799 seemed to address the crash.

Comment 2

[Sotaro Ikeda [:sotaro]]

Hmm, a few crashes still exist. Reopen it.

Comment 3

[Sotaro Ikeda [:sotaro]]

Frequency of crash is very low. Then set to S3.

Comment 4

[Sotaro Ikeda [:sotaro]]

When the crash happened, register value was rax": "0xe5e5e5e5e5e5e5e5". Then UpdateOcclusionStateRunnable seemed like UAF.

Comment 5

[Sotaro Ikeda [:sotaro]]

deleted

Comment 6

[Sotaro Ikeda [:sotaro]]

The crash seems weird. From the reports, the crash seemed to happen by UAF of UpdateOcclusionStateRunnable. But UpdateOcclusionStateRunnable is always ref counted by RefPtr<> from its creation until calling UpdateOcclusionStateRunnable::Run().

Comment 7

[Sotaro Ikeda [:sotaro]]

:jrmuizel, nical, do you have any ideas about how the crash happened? If the crash actually happened by UAF, it should happen at frontTask->Run(), I think. Hmm.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Should we consider disabling occlusion on 96 before we ship this crash? The frequency looks pretty non-trivial to me.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

nvm, this is behind the early_beta flag

Comment 10

[Jeff Muizelaar [:jrmuizel]]

The crash isn't getting properly attributed, presumably because UpdateOcclusionState is being inlined. The crash is actually happening here:
https://hg.mozilla.org/mozilla-central/file/3762abb6ee0616e990d1b843cd98da68465dd8f6/widget/windows/WinWindowOcclusionTracker.cpp#l637

Comment 11

[Jeff Muizelaar [:jrmuizel]]

It looks like the nsBaseWidget* in mHwndRootWindowMap is probably going away. Sotaro, should we be using a RefPtr<nsBaseWidget> instead of a raw pointer?

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)

It looks like the nsBaseWidget* in mHwndRootWindowMap is probably going away. Sotaro, should we be using a RefPtr<nsBaseWidget> instead of a raw pointer?

Thank you for the advice! I am going to try it.

Comment 13

[Sotaro Ikeda [:sotaro]]

nsWindow::DestroyCompositor() tries to remove nsBaseWidget* from mHwndRootWindowMap. Then I used the raw pointer.

Comment 14

[Sotaro Ikeda [:sotaro]]

Bug 1746813 is going to update crash signature.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is mozilla::widget::WinWindowOcclusionTracker::UpdateOcclusionState the new signature for these? The timing fits from what I can see.

Comment 16

[Sotaro Ikeda [:sotaro]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)

Is mozilla::widget::WinWindowOcclusionTracker::UpdateOcclusionState the new signature for these? The timing fits from what I can see.

Yes, the new crash signature seemed to show a correct place of the crash.

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #16)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)

Is mozilla::widget::WinWindowOcclusionTracker::UpdateOcclusionState the new signature for these? The timing fits from what I can see.

Yes, the new crash signature seemed to show a correct place of the crash.

Somehow, nsBaseWidget* became an obsoleted pointer as in Comment 11.

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)

It looks like the nsBaseWidget* in mHwndRootWindowMap is probably going away. Sotaro, should we be using a RefPtr<nsBaseWidget> instead of a raw pointer?

RefPtr<nsBaseWidget> usage seemed to cause the problem. nsBaseWidget::~nsBaseWidget() and nsWindow::~nsWindow() seem to expect that a widget might be released without calling Destroy().

Then it might be better to use weak pointer of widget.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: Bug 1741452 - Use nsWeakPtr in mHwndRootWindowMap

Comment 20

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/jobs?repo=try&revision=40e56edfb96baef21e8fa6e07929921e04b5389c

Comment 22

[Sotaro Ikeda [:sotaro]]

Comment on attachment 9258334 [details]
Bug 1741452 - Use nsWeakPtr in mHwndRootWindowMap

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It is very hard. And this is behind the early_beta flag.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: 97
• If not all supported branches, which bug introduced the flaw?: Bug 1732736
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It is easy to create.
• How likely is this patch to cause regressions; how much testing does it need?: It is unlikely to cause the regression, since nsBaseWidget* in mHwndRootWindowMap is just changed to nsWeakPtr.

Comment 23

[Tom Ritter [:tjr]]

Comment on attachment 9258334 [details]
Bug 1741452 - Use nsWeakPtr in mHwndRootWindowMap

Approved to land and uplift

Comment 24

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

Use nsWeakPtr in mHwndRootWindowMap r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/a021f128d35a624a09749feb855c1f6c8ea9d5d9
https://hg.mozilla.org/mozilla-central/rev/a021f128d35a

Comment 25

[BugBot [:suhaib / :marco]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 26

[Donal Meehan [:dmeehan]]

Landed 97.0b3
https://hg.mozilla.org/releases/mozilla-beta/rev/036e6fc8e0b8