1833503 - Crash in [@ mozilla::dom::ThreadSafeRequestHandle::IsEmpty] via ScriptExecutorRunnable::PreRun on poison value

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Crash report: https://crash-stats.mozilla.org/report/index/ceb855bd-ece6-4493-8e18-d3fff0230514

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  xul.dll  RefPtr<JS::loader::ScriptLoadRequest>::operator! const  mfbt/RefPtr.h:350
0  xul.dll  mozilla::dom::ThreadSafeRequestHandle::IsEmpty  dom/workers/loader/WorkerLoadContext.h:180
0  xul.dll  mozilla::dom::workerinternals::loader::ScriptExecutorRunnable::PreRun  dom/workers/ScriptLoader.cpp:1564
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:479
4  xul.dll  mozilla::dom::WorkerPrivate::RunCurrentSyncLoop  dom/workers/WorkerPrivate.cpp:4374
5  xul.dll  mozilla::dom::AutoSyncLoopHolder::Run  dom/workers/WorkerPrivate.h:1608
5  xul.dll  mozilla::dom::workerinternals::  dom/workers/ScriptLoader.cpp:260
6  xul.dll  mozilla::dom::workerinternals::Load  dom/workers/ScriptLoader.cpp:1810

This crash looks similar to bug 1824803, but it appears to be showing up in builds that have that fix. The signature is different for some reason. Crashes in 113 and 114.

Comment 1

[Eden Chuang[:edenchuang]]

It looks like ScriptLoader::mLoadedRequests could be empty in the situation, such that requestHandle becomes null. But I am not sure if this is reasonable or not.

Since Yulia is on PTO and back in October, Yoshi, could you help or find someone to take a look

Thanks

Comment 2

[Jens Stutte [:jstutte]]

Hi :yoshi, are you able to dedicate some time to this? Thanks!

Comment 3

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

I'll check it this week, but this crash is from the classic worker, if the worker team can handle this feel free to take it.

Comment 4

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

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:allstars.chh, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 5

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

forward the ni? of comment 4 to asuth.

Comment 7

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

The problem is when we call importScripts("a', "b", ...), if the script "a" cannot be loaded, it will create and launch a ScriptExecutorRunnable for the script "a", and the ThreadSafeRequestHandle->ReleaseRequest() is called
https://searchfox.org/mozilla-central/rev/986024d59bff59819a3ed2f7c1d0f5254cdc3f3d/dom/workers/ScriptLoader.cpp#1659
it will dereference the ScriptLoaderRunnable, which in turn will release ScriptLoaderRunnable and the mLoadingRequests in ScriptLoaderRunnable.
Later when the ScriptExecutoreRunnable for "b" is launched, the ThreadSafeRequestHandle is invalid and causes the crash.

Comment 8

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

Attachment: Bug 1833503 - Add ref to ScriptLoaderRunnable.

Comment 9

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

Attachment: Bug 1833503 - Test for importScripts.

Comment 10

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

Comment on attachment 9339504 [details]
Bug 1833503 - Add ref to ScriptLoaderRunnable.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easy, this patch just adds a reference to the ScriptLoaderRunnable to prevent it from being released.
• 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?: 109
• If not all supported branches, which bug introduced the flaw?: Bug 1800496
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This patch can be applied to Firefox 109.
  https://hg.mozilla.org/mozilla-unified/rev/FIREFOX_109_0b9_RELEASE
  (d7ace0e5f22b0ae00db264be498b9ee0dc4baa98)
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely, this patch just adds a reference to the ScriptLoaderRunnable.
• Is Android affected?: Yes

Comment 11

[Tom Ritter [:tjr]]

Comment on attachment 9339504 [details]
Bug 1833503 - Add ref to ScriptLoaderRunnable.

Approved to request uplift and land

Comment 12

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

Add ref to ScriptLoaderRunnable. r=dom-worker-reviewers,smaug
https://hg.mozilla.org/integration/autoland/rev/1096905e1a6abf5085d4ae9197b139c4f6004e83
https://hg.mozilla.org/mozilla-central/rev/1096905e1a6a

Comment 13

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

The test case can be found in https://bugzilla.mozilla.org/attachment.cgi?id=9339115

Comment 14

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

https://firefox-source-docs.mozilla.org/bug-mgmt/processes/fixing-security-bugs.html#landing-your-patch
Tests should only be checked in later, after an official Firefox release that contains the fix has been live for at least four weeks.

The fix is laned in Firefox 116, which will be released on Aug. 1. 2023,
So the test https://phabricator.services.mozilla.com/D181214 should be landed after Aug. 29. 2023

Comment 15

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

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 16

[Donal Meehan [:dmeehan]]

:yoshi can you please attach a beta uplift request on this?
We are in the final week of beta for Fx115
https://wiki.mozilla.org/Release_Management/Uplift_rules#Requesting_Uplift_via_Bugzilla

Comment 17

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

(In reply to Donal Meehan [:dmeehan] from comment #16)

:yoshi can you please attach a beta uplift request on this?
We are in the final week of beta for Fx115
https://wiki.mozilla.org/Release_Management/Uplift_rules#Requesting_Uplift_via_Bugzilla

Yes, I plan to do that, but I am still waiting for QA to verify it in nightly.
As one of fields is asking Has the fix been verified in Nightly?

Comment 18

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

Comment on attachment 9339504 [details]
Bug 1833503 - Add ref to ScriptLoaderRunnable.

Beta/Release Uplift Approval Request

• User impact if declined: heap-use-after-free crash
• 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): Only adds a reference to a ScriptLoaderRunnable class.
• String changes made/needed: no
• Is Android affected?: Yes

Comment 19

[Donal Meehan [:dmeehan]]

Comment on attachment 9339504 [details]
Bug 1833503 - Add ref to ScriptLoaderRunnable.

Approved for 115.0b9.

Comment 20

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2aea2505b3e2

Comment 21

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

The crash is caused by Bug 1800496, which was landed in Firefox 109
Should this patch be uplifted to previous branches up to Firefox 109 as well ?

Comment 22

[Donal Meehan [:dmeehan]]

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

The crash is caused by Bug 1800496, which was landed in Firefox 109
Should this patch be uplifted to previous branches up to Firefox 109 as well ?

No, 109 is end of life. Uplifting to the next release going out (i.e. Fx115) is all that's needed.

Comment 23

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

a month ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2023-08-15] .

allstars.chh, please refer to the original comment to better understand the reason for the reminder.

Comment 24

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

Hi Tom, can you check comment 23 for the tag [reminder-test 2023-08-15] ?
I don't know what it means.
Thanks

Comment 25

[Kagami Rosylight [:saschanaz] (they/them)]

I believe that means https://phabricator.services.mozilla.com/D181215 can be pushed now.

Comment 26

[Tom Ritter [:tjr]]

Yes - that's a reminder notification to land the test now.

Comment 27

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

According to my comment 14, this test should be landed after Aug 29,
Why can it be landed now?

Comment 28

[Tom Ritter [:tjr]]

Hah; fair enough. I will get the documentation updated. I switched it from 4 weeks to 2 weeks (half-a-cycle) so we weren't reminding people to land things during the code freeze.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1577716f1bf6
Test for importScripts. r=dom-worker-reviewers,smaug

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/1577716f1bf6