Closed Bug 1251922 Opened 8 years ago Closed 8 years ago

Assertion failure: isNurseryAllocAllowed(), at js/src/gc/Allocator.cpp:153

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 + verified
firefox-esr38 --- wontfix
firefox-esr45 46+ fixed

People

(Reporter: gkw, Assigned: terrence)

References

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.1+])

Attachments

(3 files, 1 obsolete file)

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

// jsfunfuzz-generated
enableShellObjectMetadataCallback();
// Adapted from randomly chosen test: js/src/jit-test/tests/profiler/bug1211962.js
var lfGlobal = newGlobal();
for (lfLocal in this) {
    lfGlobal[lfLocal] = this[lfLocal];
}
const script = 'oomTest(() => getBacktrace({}));';
lfGlobal.offThreadCompileScript(script);

Backtrace:

0   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010096f868 JSObject* js::gc::GCRuntime::tryNewNurseryObject<(js::AllowGC)0>(JSContext*, unsigned long, unsigned long, js::Class const*) + 232 (Allocator.cpp:153)
1   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001005e87df js::NewObjectCache::newObjectFromHit(JSContext*, int, js::gc::InitialHeap) + 383 (Runtime-inl.h:63)
2   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001005ca5c7 js::NewObjectWithClassProtoCommon(js::ExclusiveContext*, js::Class const*, JS::Handle<JSObject*>, js::gc::AllocKind, js::NewObjectKind) + 311 (jsobj.cpp:773)
3   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010075e878 ShellObjectMetadataCallback(JSContext*, JS::Handle<JSObject*>) + 200 (jsobjinlines.h:746)
4   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010053dc0a JSCompartment::setNewObjectMetadata(JSContext*, JS::Handle<JSObject*>) + 170 (jscompartment.cpp:893)
5   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001005e3370 js::SetNewObjectMetadata(js::ExclusiveContext*, JSObject*) + 384 (RootingAPI.h:666)
6   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001005ca2b3 NewObject(js::ExclusiveContext*, JS::Handle<js::ObjectGroup*>, js::gc::AllocKind, js::NewObjectKind, unsigned int) + 387 (jsobj.cpp:668)
7   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001005c9d81 js::NewObjectWithGivenTaggedProto(js::ExclusiveContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) + 177 (jsobj.cpp:729)
8   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001006ddf7f CreateBlankProto(JSContext*, js::Class const*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) + 95 (NativeObject.h:1453)
9   js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001006ddea9 js::GlobalObject::createBlankPrototype(JSContext*, js::Class const*) + 105 (RootingAPI.h:666)
10  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010058ef3c js::GlobalObject::initIteratorProto(JSContext*, JS::Handle<js::GlobalObject*>) + 156 (GlobalObject.h:337)
11  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010054809e js::GlobalObject::getOrCreateObject(JSContext*, unsigned int, bool (*)(JSContext*, JS::Handle<js::GlobalObject*>)) + 126 (GlobalObject.h:529)
12  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001006dae86 js::GlobalObject::initStarGenerators(JSContext*, JS::Handle<js::GlobalObject*>) + 166 (GlobalObject.h:537)
13  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001006e55ac EnsureParserCreatedClasses(JSContext*, js::ParseTaskKind) + 124 (HelperThreads.cpp:386)
14  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001006e2293 CreateGlobalForOffThreadParse(JSContext*, js::ParseTaskKind, js::gc::AutoSuppressGC const&) + 227 (HelperThreads.cpp:422)
15  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001006e1eab js::StartOffThreadParseScript(JSContext*, JS::ReadOnlyCompileOptions const&, char16_t const*, unsigned long, void (*)(void*, void*), void*) + 75 (HelperThreads.cpp:466)
16  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100013af2 OffThreadCompileScript(JSContext*, unsigned int, JS::Value*) + 1330 (js.cpp:3766)
17  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x0000000100798022 js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 738 (jscntxtinlines.h:236)
18  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x000000010079876b js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) + 555 (Interpreter.cpp:530)
19  js-dbg-64-dm-clang-darwin-5e0140b6d118	0x00000001001b7d31 js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 3105 (BaselineIC.cpp:6136)
20  ???                           	0x0000000101ebb45b 0 + 4327191643
21  ???                           	0x000000010200ced0 0 + 4328574672

Setting s-s as a start since this seems to involve a gc assertion.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/17bfd6a2a529
user:        Jon Coppeard
date:        Thu Feb 25 09:43:06 2016 +0000
summary:     Bug 1232229 - Add assertions to prevent nursery allocation when setting up OMT parse tasks r=terrence

Jon, is bug 1232229 a likely regressor?
Blocks: 1232229
Flags: needinfo?(jcoppeard)
That patch just looks like it just adds an assertion, so the actual underlying regression would be earlier.
Jon is away. Maybe Terrence can take a look.
Flags: needinfo?(jcoppeard) → needinfo?(terrence)
Yup, looks like Jon's diagnostic picked up a signal! I'll try and figure it out.
For some reason we create the global and all associated state that we're going to use when parsing off-main-thread using a normal context. This allows these allocations to be made in the nursery, then moved off-main-thread. This is, obviously, highly catastrophic.

In general, the global allocation paths are very careful to avoid allocating in the nursery, for exactly this reason. Unfortunately, the metadata system is allocated in a generic fashion, automatically, below this layer. This /really/ should have all be converted over to ExclusiveContext when OMT parsing landed. I'm sure we can hack around it somehow in the short term, but we really do need to make all this use ExclusiveContext at some point soon.
Flags: needinfo?(terrence)
Steve suggested a second assertion we can add to guard even harder: when tenuring, check that the target Zone is not an OMT zone.
Attached patch fuzz-1251922-v0.diff (obsolete) — Splinter Review
As we discussed today, this is a hack to work around the crash. It should be easy to backport to branches as well, as we'll need something much more involved to solve the problem at a more core level.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8725457 - Flags: review?(nfitzgerald)
Comment on attachment 8725457 [details] [diff] [review]
fuzz-1251922-v0.diff

Might help if I qref first.
Attachment #8725457 - Flags: review?(nfitzgerald)
Now with more than 0 lines.
Attachment #8725457 - Attachment is obsolete: true
Attachment #8725458 - Flags: review?(nfitzgerald)
Attachment #8725458 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8725458 [details] [diff] [review]
fuzz-1251922-v0.diff

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I think it would be quite difficult. You'd have to trick the user into enabling the debugger to turn on metadata collection, then take advantage of a race with the GC to corrupt the free lists. That would, I think, allow you to finalize a live object, which could lead to UAF. It would be a same-zone write however, so I'm not sure how useful it would actually be.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, although I think it's unlikely that anyone is looking at GC flaws yet, considering how hard they are to exploit.

Which older supported branches are affected by this flaw?
All currently shipping branches.

If not all supported branches, which bug introduced the flaw?
--

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I'm not sure when metadata suppression was added, but I think it was early enough that we should be able to backport easily.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely. When we merge back into the main thread, it looks like we unset the debuggee status, which means we can't make use of the metadata object. Thus, we won't be losing any functionality by not creating it in the first place.
Attachment #8725458 - Flags: sec-approval?
I'm going to give sec-approval+ now but this needs to wait until 3/22, two weeks into the next cycle. We're shipping 45 this week so we don't want to expose this right now.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][checkin on 3/22]
Attachment #8725458 - Flags: sec-approval? → sec-approval+
Oh, and we will want branch patches at that time, to go in after trunk.
Hi Terrence. Based on comment 7, it appears that there could be more work here. Once this bug is fixed, can you make another bug to track that?
Terrence, a friendly reminder that your sec-approval is go for this bug, as of today.
Flags: needinfo?(terrence)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d860e70ee337ba7a648d7f8de234ea075c83f3f8
Bug 1251922 - Do not create metadata objects for temporary parse globals; r=fitzgen
Whiteboard: [jsbugmon:update][checkin on 3/22] → [jsbugmon:update]
https://hg.mozilla.org/mozilla-central/rev/d860e70ee337
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Terence can you request uplift? Thanks.
Sylvestre do you want to consider this for 45 dot release?
Flags: needinfo?(sledru)
> How easily could an exploit be constructed based on the patch?
> I think it would be quite difficult.

Based on this statement, no, I am not planning to take it for 45.
However, happy to take it for ESR 45
Flags: needinfo?(sledru)
Comment on attachment 8725458 [details] [diff] [review]
fuzz-1251922-v0.diff

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Unnecessary crashes.
Fix Landed on Version: 48
Risk to taking this patch (and alternatives if risky): Very low. 
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Flags: needinfo?(terrence)
Attachment #8725458 - Flags: checkin+
Attachment #8725458 - Flags: approval-mozilla-esr45?
Attachment #8725458 - Flags: approval-mozilla-beta?
Attachment #8725458 - Flags: approval-mozilla-aurora?
Comment on attachment 8725458 [details] [diff] [review]
fuzz-1251922-v0.diff

Fix for sec-critical issue, let's uplift to aurora and for the beta 9 build.
Attachment #8725458 - Flags: approval-mozilla-beta?
Attachment #8725458 - Flags: approval-mozilla-beta+
Attachment #8725458 - Flags: approval-mozilla-aurora?
Attachment #8725458 - Flags: approval-mozilla-aurora+
This doesn't apply cleanly to beta:

$ hg graft -er bceb0dcb8897
grafting 336556:bceb0dcb8897 "Bug 1251922 - Do not create metadata objects for temporary parse globals; r=fitzgen a=lizzard"
merging js/src/vm/HelperThreads.cpp
warning: conflicts while merging js/src/vm/HelperThreads.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
Flags: needinfo?(terrence)
The AutoSuppress API does not exist on beta, so let's just test for a flag that we know is set in the bad path.
Flags: needinfo?(terrence)
Attachment #8739084 - Flags: review?(nfitzgerald)
Comment on attachment 8739084 [details] [diff] [review]
backport_beta_1251922-v0.diff

Review of attachment 8739084 [details] [diff] [review]:
-----------------------------------------------------------------

Blunt, but effective.
Attachment #8739084 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8725458 [details] [diff] [review]
fuzz-1251922-v0.diff

sec-critical, taking it.
Should be in 45.1.0
Attachment #8725458 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
ESR38 is affected, is there a reason we didn't take this there?
Flags: needinfo?(lhenry)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main46+][adv-esr45.1+]
Flags: needinfo?(sledru)
Comment on attachment 8725458 [details] [diff] [review]
fuzz-1251922-v0.diff

Nope, just missed it, sorry!
Flags: needinfo?(sledru)
Flags: needinfo?(lhenry)
Attachment #8725458 - Flags: approval-mozilla-esr38+
This (the patch that landed in esr45, at least) doesn't apply cleanly to esr38, can we get a rebased patch?
Flags: needinfo?(terrence)
Sorry, was out on PTO yesterday afternoon and this morning. I just finished backporting, but it looks like the tree is already closed for the version bump.
Flags: needinfo?(terrence)
Attachment #8743555 - Flags: review+
Sylvestre: what do you want to do with this ESR 38 patch now?
Flags: needinfo?(sledru)
This has missed ESR38.8 now. Since ESR38 is EOL after this release, I'm assuming this will never be fixed on ESR38 at this point.
I agree with Al
Flags: needinfo?(sledru)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.