Crash report: https://crash-stats.mozilla.org/report/index/57164b49-9ced-4a88-8ba2-38c750201027

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 10 frames of crashing thread:

0 xul.dll js::GlobalHelperThreadState::finishParseTaskCommon js/src/vm/HelperThreads.cpp:1918
1 xul.dll js::GlobalHelperThreadState::finishSingleParseTask js/src/vm/HelperThreads.cpp:2039
2 xul.dll nsJSUtils::ExecutionContext::JoinDecode dom/base/nsJSUtils.cpp:299
3 xul.dll mozilla::dom::ScriptLoader::EvaluateScript dom/script/ScriptLoader.cpp:2937
4 xul.dll mozilla::dom::ScriptLoader::ProcessRequest dom/script/ScriptLoader.cpp:2535
5 xul.dll mozilla::dom::ScriptLoader::ProcessPendingRequests dom/script/ScriptLoader.cpp:3287
6 xul.dll mozilla::detail::RunnableMethodImpl< xpcom/threads/nsThreadUtils.h:1240
7 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:146
8 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:515
9 xul.dll mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:85:7'>::Run xpcom/threads/nsThreadUtils.h:577

This appears to be a use-after-free crash. 64-bit crash reports appear to be crashing on address 0xffffffffffffffff but that's a side-effect of bug 1493342. They're really crashing on the full poison pattern 0xe5e5e5e5e5e5e5e5, opening the raw dump for them will show at least two registers holding it.

There's a few reports in Firefox 82 and I'm not sure what to make of them because it appears to have started in nightly where there's significant volume. The first affected build has buildid 20201021213007.

sec-want is generally used for proposed security enhancements, and isn't appropriate for potential vulnerabilities like a UAF. I'll remove that.

The big spike that happened in nightly 84 at the end of October has subsided: the last nightly build with any appreciable number of crashes is 20201027212052. The earliest build with a lot of crashes is 20201014125134. Whatever caused the spike was probably fixed or backed out on 10/27 or 10/28. On that basis we could call this "worksforme" (fixed).

There is a moderate number of crashes on 82.0, 82.0.2 and 82.0.3. Something that got uplifted? or just coincidence? The crashes with the UAF marker in the registers seem to start with 82.0. There were crashes in earlier versions (68 and 78 ESRs, 80 and 81) but they don't have the UAF markers. There are also UAF-looking 83 beta crashes that we're about to ship. Would we be able to figure out what we could uplift to fix it there in a point release, or should we just wait for the 84 release to clear it up?

Gabriele: should we just call this "worksforme"?

For nightly I'd say yes. There's definitely some valid crashes on release and possibly beta too. One of the issues with this signatures it's that it's one of the "common" ones that tends to catch noise from machines with flaky hardware, you can tell because the remaining crashes on release have multiple different reasons and multiple different crashing addresses. However there remains a few valid UAFs on release that can be told apart because they are hitting the poison pattern and are read accesses (crashes caused by writes don't seem to involve the poison pattern so I think they're noise).

Denis, I think you might have worked on this code recently, any insight about these crashes?

Based on the time frames, it looks like the problem was first introduced by https://hg.mozilla.org/mozilla-central/rev/480a0b10c49a2d97d4332e843ccd62ba2a86449e on 10/21 and then subsequently fixed by https://hg.mozilla.org/mozilla-central/rev/68e6b05b17b7e32496b5519f0b0fd9e99e10b3a0 on 10/28. This seems to also line up with the crash stack since the first change could have accidentally canceled an off thread parse without ScriptLoader knowing about it, leading to the UAF when the script needs to be executed.

I would say this is probably resolved/duplicated by bug 1672760.

Outside of the spike introduced by Bug 1672760 (which was fixed before hitting beta/release), there are still UAFs from the dom::ScriptLoader.

In beta84 / nightly85, the crashes are all MOZ_RELEASE_ASSERT(task->runtime == cx->runtime()). I refactored code in this area which is probably why we now have a more predictable location.

It looks like this code mishandles raw pointers and smart pointers and under error conditions we can UAFs.

I'll put together a patch for these and update security assessment. At a glace, it seems that 84+ crash in a predictable way.

This is a pretty traditional UAF caused by an unlucky OOM. While the crashes in the wild now seem to be MOZ_RELEASE_ASSERT, with care one could do worse. The fix is straightforward though, and the related code should probably be refactored further to avoid these accidents in future.

Comment on attachment 9190922 [details]
Bug 1673567 - Cleanup initialization of JS::OffThreadToken. r?jonco!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Moderately. Patch suggests that the out-param still matters in failure paths, but this still requires a well placed OOM and then exploiting the UAF itself.

Uplifting to late beta-84 seems to most sensible. I don't believe it is worth getting into 83.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: 82+
• If not all supported branches, which bug introduced the flaw?: Bug 1652126
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Applies cleanly to 84, 85. There is a trivial conflict when rebasing on 83.
• How likely is this patch to cause regressions; how much testing does it need?: Patch is straightforward and only has impact on OOM edge cases. Risk is low and new code is simpler than old.

Comment on attachment 9190922 [details]
Bug 1673567 - Cleanup initialization of JS::OffThreadToken. r?jonco!

sec-approved to land today if we can uplift it too.

[Tracking Requested - why for this release]:

https://hg.mozilla.org/integration/autoland/rev/336e9d01c80d9b88e3c15f59ef18f8dfb812a84d

Backed out for causing assertion failures on HelperThreads.cpp

backout: https://hg.mozilla.org/integration/autoland/rev/00126f905ad388041537780532b0040b5b4e6177

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=336e9d01c80d9b88e3c15f59ef18f8dfb812a84d&searchStr=spidermonkey&selectedTaskRun=HobFKyNOSamJkTa5YAI04g.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=323592669&repo=autoland&lineNumber=12898

[task 2020-12-04T16:44:47.386Z] TEST-PASS | testThreadingThreadSeTesting with principals 'IsSystem'
[task 2020-12-04T16:44:47.388Z] Testing with principals 'IsNotSystem'
[task 2020-12-04T16:44:47.390Z] Testing with principals 'testPrincipals'
[task 2020-12-04T16:44:47.392Z] Testing with principals 'nullptr principals'
[task 2020-12-04T16:44:48.202Z] Assertion failure: *tokenOut == nullptr, at /builds/worker/checkouts/gecko/js/src/vm/HelperThreads.cpp:1091
[task 2020-12-04T16:44:48.213Z] in directory /builds/worker/workspace/obj-spider, running ['setarch', 'x86_64', '-R', 'make', 'check-jstests']
[task 2020-12-04T16:44:48.219Z] make -C js/src check-jstests
[task 2020-12-04T16:44:48.223Z] make[1]: Entering directory '/builds/worker/workspace/obj-spider/js/src'
[task 2020-12-04T16:44:48.223Z] ../../dist/bin/run-mozilla.sh /builds/worker/workspace/obj-spider/_virtualenvs/init_py3/bin/python -u /builds/worker/checkouts/gecko/js/src/tests/jstests.py
[task 2020-12-04T16:44:48.223Z] --no-progress --format=automation --timeout 300
[task 2020-12-04T16:44:48.223Z] --args='--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so' --exclude-file=/builds/worker/checkouts/gecko/js/src/devtools/automation/arm64-jstests-slow.txt
[task 2020-12-04T16:44:48.223Z] ../../dist/bin/js
[task 2020-12-04T16:45:08.412Z] TEST-PASS | test262/harness/verifyProperty-restore.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so") [0.1 s]

Thanks. Issue was with a C++ test-case itself. Fixed and re-pushed.

https://hg.mozilla.org/integration/autoland/rev/825d07a758626f0b1ed2da76592edb23b2def267

https://hg.mozilla.org/mozilla-central/rev/825d07a75862

Comment on attachment 9190922 [details]
Bug 1673567 - Cleanup initialization of JS::OffThreadToken. r?jonco!

Beta/Release Uplift Approval Request

• User impact if declined: Potentially exploitable UAF in content process. Also leading to small number of crashes in the wild for normal users.
• Is this code covered by automated tests?: Yes
• 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): The changed code is in an uncommon error path only. New code is more straightforward than old.
• String changes made/needed:

Comment on attachment 9190922 [details]
Bug 1673567 - Cleanup initialization of JS::OffThreadToken. r?jonco!

Approved for 84.0rc1.

https://hg.mozilla.org/releases/mozilla-beta/rev/b73daebb5397

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.