1616079 - Crash in [@ RtlAcquireSRWLockExclusive | mozilla::detail::MutexImpl::lock | mozilla::dom::WorkerRunnable::DispatchInternal]

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P1)

Description

[[:philipp]]

This bug is for crash report bp-6b42f74b-7ac0-4dee-995d-c2e540200217.

Top 10 frames of crashing thread:

0 ntdll.dll RtlAcquireSRWLockExclusive 
1 mozglue.dll mozilla::detail::MutexImpl::lock mozglue/misc/Mutex_windows.cpp:22
2 xul.dll mozilla::dom::WorkerRunnable::DispatchInternal dom/workers/WorkerRunnable.cpp
3 xul.dll mozilla::dom::WorkerRunnable::Dispatch dom/workers/WorkerRunnable.cpp:89
4 xul.dll mozilla::detail::RunnableFunction<`lambda at z:/task_1581648291/build/src/dom/workers/remoteworkers/RemoteWorkerChild.cpp:475:19'>::Run xpcom/threads/nsThreadUtils.h:562
5 xul.dll mozilla::MozPromise<nsTArray<bool>, nsresult, 0>::ThenValue<`lambda at z:/task_1581648291/build/src/extensions/permissions/nsPermissionManager.cpp:3300:29', `lambda at z:/task_1581648291/build/src/extensions/permissions/nsPermissionManager.cpp:3301:11'>::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:727
6 xul.dll mozilla::MozPromise<RefPtr<mozilla::AllocPolicy::Token>, bool, 1>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:403
7 xul.dll mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run xpcom/threads/TaskDispatcher.h:197
8 xul.dll mozilla::EventTargetWrapper::Runner::Run xpcom/threads/AbstractThread.cpp:113
9 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:282

these cross-platform content crash signatures are showing up in increased volume since firefox 73. in firefox 73.0b9 the sw-e10s work got disabled and those crashes stopped, so i assume they are related to this new feature.
the crashing addresses of a number of reports indicate that this is a UAF issue.

Comment 1

[Jens Stutte [:jstutte]]

It seems, that we fail accessing a WorkerPrivate's mutex, thus a freed WorkerPrivate.

However, as the DispatchInternal function is inlined, we cannot be sure, if it is the access to the WorkerRunnable's mWorkerPrivate or the access to its parent's WorkerPrivate.

The WorkerRunnable contains a raw pointer to WorkerPrivate though. Rather than to try to patch, the better approach might be to start work on: bug 1539508, bug 1350337 and get rid of all those raw pointers on WorkerPrivate.

Eden, can you please have a look?

Comment 2

[Eden Chuang[:edenchuang]]

Yes, I can take a look.
Maybe we can try to replace the raw pointer of WorkerRunnable with RefPtr first.
I guess this might a side-effect of bug 1575185. Will look it in detail.

Comment 3

[Jens Stutte [:jstutte]]

This could be a dupe of bug 1604847.

Comment 4

[Eden Chuang[:edenchuang]]

According to the stack, it seems that the WorkerPrivate was alive when creating the WorkerRunnable.

https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/workers/remoteworkers/RemoteWorkerChild.cpp#463

But got deleted before executing the WorkerRunnable. And when executing the runnable, we got UAF on WorkerPrivate at

https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/workers/remoteworkers/RemoteWorkerChild.cpp#477

Comment 5

[Daniel Veditz [:dveditz]]

This is

Comment 6

[Perry Jiang [:perry] [no longer employee, use ni?]]

(In reply to Eden Chuang[:edenchuang] from comment #4)

According to the stack, it seems that the WorkerPrivate was alive when creating the WorkerRunnable.

https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/workers/remoteworkers/RemoteWorkerChild.cpp#463

But got deleted before executing the WorkerRunnable. And when executing the runnable, we got UAF on WorkerPrivate at

https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/workers/remoteworkers/RemoteWorkerChild.cpp#477

That makes sense. Maybe InitializeWorkerRunnable can have a RefPtr<WorkerPrivate>? Then the dispatch will still fail but there will not be a UAF.

Comment 7

[Eden Chuang[:edenchuang]]

Workaround in comment 1 does not work. Using RefPtr on WorkerRunnable::mWorkerPrivate changes WorkerPrivate's life cycle. And it also makes WorkerPrivate be freed on the unexpected thread. So we meet lots of crashes in ~WorkerPrivate().

I am not sure if using RefPtr holding WorkerPrivate in InitializeWorkerRunnable is suitable or not, since the WorkerPrivate's life cycle still changed.

What my guess is RemoteWorkerChild::mState probably is switched to Terminated/PendingTerminate for some reasons, i.e. shutdown, before dispatching the InitializeWorkerRunnable. The state switching makes the WorkerPrivate strong reference on Pending state be released.

https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/dom/workers/remoteworkers/RemoteWorkerChild.cpp#446

But the following logic doesn't expect the state switching case. It seems to expect the RemoteWorkerChild::mState should be Pending

https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/dom/workers/remoteworkers/RemoteWorkerChild.cpp#452-459

Here is a workaround to check if RemoteWorkerChild::mState is Pending before dispatching InitializeWorkerRunnable. The difference from holding a RefPtr<WorkerPrivate> in InitializeWorkerRunnable is this workaround does not change any object's life cycle.

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Checking if RemoteWorkerChild::mState is Pending before dispatching IninializeWorkerRunnable

Comment 9

[Jens Stutte [:jstutte]]

Eden, AFAIU this can bite us with and without SW e10s enabled, right?

Comment 10

[Eden Chuang[:edenchuang]]

If I understand correctly it affects sw-e10s, since sw-e10s uses RemoteWorker mechanism.
But I am not sure without sw-e10s case, probably is not.

Comment 11

[Eden Chuang[:edenchuang]]

Comment on attachment 9128764 [details]
Checking if RemoteWorkerChild::mState is Pending before dispatching IninializeWorkerRunnable

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it is not easy to construct the case. It needs a special thread execution sequence to achieve, but the thread execution switching is controlled by OS.
• 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?: 73
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Essentially the patch should not cause regressions, it only adds a condition check before dispatching runnable.

Comment 12

[Perry Jiang [:perry] [no longer employee, use ni?]]

Comment on attachment 9128764 [details]
Checking if RemoteWorkerChild::mState is Pending before dispatching IninializeWorkerRunnable

Beta/Release Uplift Approval Request

• User impact if declined: Intermittent UAF
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adds a safety check, overall restricts the possible things that can happen (no new situations would need to be tested for).
• String changes made/needed:

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9128764 [details]
Checking if RemoteWorkerChild::mState is Pending before dispatching IninializeWorkerRunnable

Approved for our last beta.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/4fbd268ab7f0
https://hg.mozilla.org/releases/mozilla-beta/rev/c3c4e6f97dc3

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/4fbd268ab7f0