Closed Bug 996536 Opened 6 years ago Closed 6 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, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox29 --- wontfix
firefox30 --- verified
firefox31 --- verified
firefox-esr24 --- unaffected
b2g-v1.3 --- wontfix
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
seamonkey2.26 --- fixed

People

(Reporter: decoder, Assigned: till)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main30+])

Crash Data

Attachments

(2 files)

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) {}
}
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]
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
Needinfo from bhackett based on comment 3 :)
Flags: needinfo?(bhackett1024)
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)
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
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
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: nobody → till
Status: NEW → ASSIGNED
Attachment #8411339 - Flags: review?(bhackett1024) → review+
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)
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.
Flags: needinfo?(continuation)
Keywords: sec-highsec-low
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.
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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Status: RESOLVED → VERIFIED
Crash Signature: [@ JSFunction::lazyScript] [@ js::ExclusiveContext::getNewType] → [@ JSFunction::lazyScript] [@ js::ExclusiveContext::getNewType]
JSBugMon: This bug has been automatically verified fixed.
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?
Attachment #8411339 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3debec91f196
Crash Signature: [@ JSFunction::lazyScript] [@ js::ExclusiveContext::getNewType] → [@ JSFunction::lazyScript] [@ js::ExclusiveContext::getNewType]
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main30+]
Crash Signature: [@ JSFunction::lazyScript] [@ js::ExclusiveContext::getNewType] → [@ JSFunction::lazyScript] [@ js::ExclusiveContext::getNewType]
JSBugMon: This bug has been automatically verified fixed on Fx30
JSBugMon: This bug has been automatically verified fixed on Fx31
Group: core-security
You need to log in before you can comment on or make changes to this bug.