Closed
Bug 996536
Opened 10 years ago
Closed 10 years ago
Assertion failure: isInterpretedLazy() && u.i.s.lazy_, at jsfun.h:328 or Crash [@ JSFunction::lazyScript] or Crash [@ js::ExclusiveContext::getNewType] with gcPreserveCode
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla31
People
(Reporter: decoder, Assigned: till)
Details
(4 keywords, Whiteboard: [jsbugmon:update][adv-main30+])
Crash Data
Attachments
(2 files)
1.05 KB,
text/plain
|
Details | |
1.15 KB,
patch
|
bhackett1024
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The following testcase asserts on mozilla-central revision 5b6e82e7bbbf (run with --fuzzing-safe --ion-eager): gcPreserveCode(); var lfcode = new Array(); lfcode.push = loadFile; lfcode.push("\ function f() {}\ res = new f();\ var otherGlobalDifferentCompartment = newGlobal();\ eval = otherGlobalDifferentCompartment.eval;\ assertEq(res[(27)], otherGlobalDifferentCompartment);\ "); lfcode.push("setObjectMetadataCallback(function(obj) {});"); lfcode.push("gczeal(2,1);"); lfcode.push("var obj = new Intl.DateTimeFormat();"); function loadFile(lfVarx) { try { eval(lfVarx); } catch (lfVare) {} }
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
Crash trace: Program received signal SIGSEGV, Segmentation fault. js::ExclusiveContext::getNewType (this=0x15b84d0, clasp=<optimized out>, proto=..., fun=0x7ffff4f845e0) at js/src/jsinfer.cpp:3802 3802 fun = fun->lazyScript()->functionNonDelazifying(); #0 js::ExclusiveContext::getNewType (this=0x15b84d0, clasp=<optimized out>, proto=..., fun=0x7ffff4f845e0) at js/src/jsinfer.cpp:3802 #1 0x0000000000760d76 in js::CreateThisForFunctionWithProto (cx=<optimized out>, callee=..., proto=<optimized out>, newKind=js::GenericObject) at js/src/jsobj.cpp:1566 [...] #5 0x000000000159a2e0 in js::jit::CreateThisInfoCodeGen () rax 0x0 0 => 0x72abd5 <js::ExclusiveContext::getNewType(js::Class const*, js::TaggedProto, JSFunction*)+389>: mov 0x8(%rax),%r13 The crash is near null but the test involves preserving code across GCs, so assuming this could be a GC issue and therefore s-s.
Crash Signature: [@ JSFunction::lazyScript]
status-firefox31:
--- → affected
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter | ||
Comment 3•10 years ago
|
||
JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/bec71542c055 user: Brian Hackett date: Sat Dec 14 16:29:43 2013 -0800 summary: Bug 950118 - Don't allow the object metadata hook to reenter JS, r=jimb. This iteration took 178.912 seconds to run.
Reporter | ||
Comment 4•10 years ago
|
||
Needinfo from bhackett based on comment 3 :)
Flags: needinfo?(bhackett1024)
Comment 5•10 years ago
|
||
Slightly smaller testcase (just so we can land one more testcase): s = newGlobal() evalcx("\ [...(function(){ yield })()];\ gc();\ for(v of Iterator(1)) {}\ for(v of Iterator(1)) {}\ ", s) gcPreserveCode(); gc(); evalcx("for(x of (function() { yield })()) {}", s)
Updated•10 years ago
|
Crash Signature: [@ JSFunction::lazyScript] → [@ JSFunction::lazyScript]
[@ js::ExclusiveContext::getNewType]
Summary: Assertion failure: isInterpretedLazy() && u.i.s.lazy_, at jsfun.h:328 or Crash [@ JSFunction::lazyScript] with gcPreserveCode → Assertion failure: isInterpretedLazy() && u.i.s.lazy_, at jsfun.h:328 or Crash [@ JSFunction::lazyScript] or Crash [@ js::ExclusiveContext::getNewType] with gcPreserveCode
Comment 6•10 years ago
|
||
Sounds sketchy, so I'm marking this sec-high. Feel free to clear the rating or whatever if it isn't a sec problem.
Keywords: sec-high
Assignee | ||
Comment 7•10 years ago
|
||
This ended up being quite trivial: fun->isInterpretedLazy() returning true doesn't guarantee that the function has a lazyScript: self-hosted builtins are interpretedLazy, but don't have one. I hope setting fun to null is ok for self-hosted builtins. It seems to me like it should be: for one, we don't (yet) use many self-hosted constructors. Also, a self-hosted function being lazy should mean that we don't have type information for it anyway, right? Finally, I doubt that this is sec-high, as a release build should just crash when trying to dereference lazyScript, which should always be null in this scenario.
Attachment #8411339 -
Flags: review?(bhackett1024)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → till
Status: NEW → ASSIGNED
Updated•10 years ago
|
Attachment #8411339 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Hmm, so this has been in the tree since Dec 11 2013[1]. The changes in bug 886193 have probably made it much easier to trigger, but even that is mid-January. Given that, should we uplift to Aurora and Beta? I don't think it's that much of a security issue, but maybe uplifting to at least Aurora makes sense. Especially considering that it's a trivial patch. [1]: http://hg.mozilla.org/mozilla-central/rev/52021335eb42
Flags: needinfo?(bhackett1024) → needinfo?(continuation)
Comment 9•10 years ago
|
||
Yeah, if it is going to always be a null deref, I wouldn't worry about beta. We're super late in beta anyways. I guess it could avoid some crashes, so it might be nice for Aurora, though if that particular crash isn't showing up on crash-stats, maybe that's not worth the hassle either.
Assignee | ||
Comment 10•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4441b7c2b10b I didn't find any crashes with this signature at all, so I'm going to assume that this doesn't really happen in the wild for now. Should we find any, we could still uplift, as the fix is unlikely to need any changes whatsoever in the foreseeable future.
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4441b7c2b10b If the patch is low-risk, I'd vote for uplifting to Fx30, FWIW.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v2.0:
--- → fixed
status-firefox29:
--- → wontfix
status-firefox30:
--- → affected
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Reporter | ||
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Crash Signature: [@ JSFunction::lazyScript]
[@ js::ExclusiveContext::getNewType] → [@ JSFunction::lazyScript]
[@ js::ExclusiveContext::getNewType]
Reporter | ||
Comment 12•10 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8411339 [details] [diff] [review] Don't assume that all interpretedLazy functions have a lazyScript ># HG changeset patch ># User Till Schneidereit <till@tillschneidereit.net> ># Date 1398287217 -7200 ># Wed Apr 23 23:06:57 2014 +0200 ># Node ID 356711ada1c76b3d97cd66d08d771209c2e42bb3 ># Parent ddcccb78254943e7836dde2d31339baffa2d1c1c >Bug 996536 - Don't assume that all interpretedLazy functions have a lazyScript. r=bhackett > >diff --git a/js/src/jsinfer.cpp b/js/src/jsinfer.cpp >--- a/js/src/jsinfer.cpp >+++ b/js/src/jsinfer.cpp >@@ -3848,17 +3848,17 @@ ExclusiveContext::getNewType(const Class > > if (!newTypeObjects.initialized() && !newTypeObjects.init()) > return nullptr; > > // Canonicalize new functions to use the original one associated with its script. > if (fun) { > if (fun->hasScript()) > fun = fun->nonLazyScript()->functionNonDelazifying(); >- else if (fun->isInterpretedLazy()) >+ else if (fun->isInterpretedLazy() && !fun->isSelfHostedBuiltin()) > fun = fun->lazyScript()->functionNonDelazifying(); > else > fun = nullptr; > } > > TypeObjectWithNewScriptSet::AddPtr p = > newTypeObjects.lookupForAdd(TypeObjectWithNewScriptSet::Lookup(clasp, proto, fun)); > if (p) {
Attachment #8411339 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8411339 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/3debec91f196
Crash Signature: [@ JSFunction::lazyScript]
[@ js::ExclusiveContext::getNewType] → [@ JSFunction::lazyScript]
[@ js::ExclusiveContext::getNewType]
status-b2g-v1.3:
--- → wontfix
status-b2g-v1.4:
--- → fixed
Updated•10 years ago
|
status-firefox-esr24:
--- → unaffected
Updated•10 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main30+]
Comment 15•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-release/rev/28a81d11f8db
status-seamonkey2.26:
--- → fixed
Reporter | ||
Updated•10 years ago
|
Crash Signature: [@ JSFunction::lazyScript]
[@ js::ExclusiveContext::getNewType] → [@ JSFunction::lazyScript]
[@ js::ExclusiveContext::getNewType]
Reporter | ||
Comment 16•10 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx30
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 17•10 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx31
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•