1824803 - Use-after-free crash in [@ mozilla::dom::workerinternals::loader::ScriptExecutorRunnable::PreRun]

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/b9766ac7-fae6-4d3c-866e-065730230322

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  RefPtr<JS::loader::ScriptLoadRequest>::get const  mfbt/RefPtr.h:325
0  xul.dll  RefPtr<JS::loader::ScriptLoadRequest>::operator-> const  mfbt/RefPtr.h:355
0  xul.dll  mozilla::dom::ThreadSafeRequestHandle::GetContext  dom/workers/loader/WorkerLoadContext.h:178
0  xul.dll  mozilla::dom::workerinternals::loader::ScriptExecutorRunnable::PreRun  dom/workers/ScriptLoader.cpp:1520
1  xul.dll  mozilla::dom::WorkerRunnable::Run  dom/workers/WorkerRunnable.cpp:261
2  xul.dll  nsThread::ProcessNextEvent  xpcom/threads/nsThread.cpp:1233
3  xul.dll  NS_ProcessNextEvent  xpcom/threads/nsThreadUtils.cpp:477
4  xul.dll  mozilla::dom::WorkerPrivate::RunCurrentSyncLoop  dom/workers/WorkerPrivate.cpp:4372
5  xul.dll  mozilla::dom::AutoSyncLoopHolder::Run  dom/workers/WorkerPrivate.h:1608
6  xul.dll  mozilla::dom::workerinternals::  dom/workers/ScriptLoader.cpp:257

Use-after-free crash in worker script load request code. I don't know if this is a dupe of something. 18 crashes in the last 2 weeks on recent versions. 1 on 113 on the 20230321213816 build (linked here) and the rest on 111.

Comment 1

[Yulia Startsev [:yulia]]

The bug only occurs with importScripts, which also happens to be the only case where we send a list of imports.

A couple of observations:

• There are only two places where we release the request: https://searchfox.org/mozilla-central/search?q=requestHandle-%3EReleaseRequest&path=&case=false&regexp=false
• We assert that the request has not been released before we transfer to the script executor: https://searchfox.org/mozilla-central/rev/82828dba9e290914eddd294a0871533875b3a0b5/dom/workers/ScriptLoader.cpp#1466
• We can end up dispatching a request in two cases: If we cancel, or if we successfully resolve: https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom15workerinternals6loader20ScriptLoaderRunnable30DispatchProcessPendingRequestsEv&redirect=false

There is a possibility that I can't rule out: we may cover one request twice. This would mean that the request handler is already empty. I am not sure how this would happen, as it doesn't appear to be due to cancellation in any of the crash reports. I am also not sure how it is happening given that we assert that the request was not released before queuing the task on the script executor. As this also happens on nightly, that means we are passing the debug check.

I've made a patch that protects this code from being accessed if the thread has been shut down, as well as doing a check for an empty request Handle -- if the requestHandle is empty, then this will be addressed by ProcessClassicScript and ProcessModuleScript. Hopefully if there are issues we can more accurately detect them from there.

Comment 2

[Yulia Startsev [:yulia]]

Attachment: Bug 1824803;

Comment 3

[Yulia Startsev [:yulia]]

Comment on attachment 9325967 [details]
Bug 1824803;

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It looks like this is very hard to trigger and happens rarely. I am having a hard time figuring out how we end up in a situation where all of the asserts do not catch this issue.
• 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?:
• If not all supported branches, which bug introduced the flaw?: Bug 1247687
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It should just be this patch
• How likely is this patch to cause regressions; how much testing does it need?: It is unlikely that this causes regressions, though it may move the error to ProcessModuleScript or ProcessClassicScript. If we end up with failures there we should look again.
• Is Android affected?: Yes

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

waiting for the sec-approval?

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] from comment #4)

waiting for the sec-approval?

I think we are very late in beta, so nothing can land on beta until next release, so sec-approvals are paused. They'll resume maybe next week or something.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Andrew McCreight [:mccr8] from comment #5)

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] from comment #4)

waiting for the sec-approval?

I think we are very late in beta, so nothing can land on beta until next release, so sec-approvals are paused. They'll resume maybe next week or something.

oh, I see, thanks for explaining.
I kept getting emails from release-mgmt team about this bug and was wondering why the sec-approval? still not granted.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Suhaib, I wonder if we can tell autonag not to include bugs with patches that have a pending sec-approval nomination in the nags that go out?

Comment 8

[Suhaib Mujahid [:suhaib]]

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

Suhaib, I wonder if we can tell autonag not to include bugs with patches that have a pending sec-approval nomination in the nags that go out?

Yes, it is doable. I filed an issue to follow up on this: https://github.com/mozilla/relman-auto-nag/issues/1966.

Comment 9

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

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

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9325967 [details]
Bug 1824803;

Approved to land and uplift

Comment 11

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

r=allstarschh
https://hg.mozilla.org/integration/autoland/rev/46fc4caba7a5751f2d2c2c7869ecc45a419fc156
https://hg.mozilla.org/mozilla-central/rev/46fc4caba7a5

Comment 12

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

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

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

For more information, please visit auto_nag documentation.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 9325967 [details]
Bug 1824803;

Beta/Release Uplift Approval Request

• User impact if declined: UAF crash
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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): This adds a check before the request is actually used, so it makes the usage of the request safer.
• String changes made/needed: no
• Is Android affected?: Yes

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9325967 [details]
Bug 1824803;

Approved for 113.0b5.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/56322cb5cada