1308048 - Crash [@ js::CompartmentChecker::fail] or Assertion failure: arena()->allocated(), at js/src/gc/Heap.h:1211

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision 42c95d88aaaa (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

// jsfunfuzz-generated
m = 'x';
for (var i = 0; i < 99; i++) {
    try {
        m += m;
    } catch (e) {}
}
offThreadCompileScript("", ({
    elementAttributeName: m
}));
// Adapted from randomly chosen test: js/src/jit-test/tests/debug/Debugger-findScripts-06.js
var n = newGlobal();
// jsfunfuzz-generated
n.runOffThreadScript();


Backtrace:

0   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x00000001012ca031 js::gc::TenuredCell::isMarked(unsigned int) const + 241 (Heap.h:1211)
1   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x000000010128c6b0 js::GlobalHelperThreadState::finishParseTask(JSContext*, js::ParseTaskKind, void*) + 368 (jscntxtinlines.h:122)
2   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x000000010128d241 js::GlobalHelperThreadState::finishScriptParseTask(JSContext*, void*) + 17 (HelperThreads.cpp:1278)
3   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x0000000100b16497 runOffThreadScript(JSContext*, unsigned int, JS::Value*) + 167 (RootingAPI.h:783)
4   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x00000001012a38cd js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 93 (jscntxtinlines.h:240)
5   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x00000001012a3648 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 696 (Interpreter.cpp:446)
6   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x00000001012a3e7e js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) + 46 (Interpreter.cpp:522)
/snip

For detailed crash information, see attachment.

This also crashes js opt builds [@ js::CompartmentChecker::fail]. Setting s-s because GC is on the stack and that js opt builds show "Compartment mismatch":

*** Compartment mismatch 0x7fb0d05086d0 vs. 0x4b4b4b4b4b4b4b4b
Segmentation fault: 11

Seems to go back prior to m-c rev dc4b163f7db7 (early Nov 2014) so unable to get a bisection window.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jon, do you know what might be the issue here, since GC is on the stack?

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1308048-root-parsed-script

The problem is that ParseTask::finish can GC, and the parse task is no longer traced after it's removed from finished parse task list.  I think just rooting the script is sufficient.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8798339 [details] [diff] [review]
bug1308048-root-parsed-script

[Security approval request comment]

How easily could an exploit be constructed based on the patch? Certainly difficult.  I'm not sure this is even possible.

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? Everything back to FF33.

If not all supported branches, which bug introduced the flaw? Bug 1031636 seems to have introduce the possibility for GC here.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be trivial.

How likely is this patch to cause regressions; how much testing does it need? Unlikely.

Comment 5

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. I think we should take this on at least Aurora and Beta as well if the patch applies.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb5040568cfb

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8798339 [details] [diff] [review]
bug1308048-root-parsed-script

Approval Request Comment
[Feature/regressing bug #]: Bug 1031636.
[User impact if declined]: Possible crash / security vulnerability.
[Describe test coverage new/current, TreeHerder]: On m-c since 8th October.
[Risks and why]: Low.
[String/UUID change made/needed]: None.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798339 [details] [diff] [review]
bug1308048-root-parsed-script

Sec-high, Aurora51+, Beta50+

Hi Jon, we will also need a patch for uplift to ESR45.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8b247ac9ab1
https://hg.mozilla.org/releases/mozilla-beta/rev/3d2ba22eee02

Comment 10

[Jon Coppeard (:jonco)]

Attachment: bug1308048-esr45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug.
User impact if declined: Possible crash / security vulnerability.
Fix Landed on Version: FF33.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8799676 [details] [diff] [review]
bug1308048-esr45

sec-high, taking it on esr 45

Comment 12

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/bb10104dc89ef873c508f737cead98bf354dd6d1

Comment 13

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx50
JSBugMon: This bug has been automatically verified fixed on Fx51