Closed Bug 792220 Opened 8 years ago Closed 7 years ago

IonMonkey: Assertion failure: !cx->compartment->activeAnalysis, at jsinterp.cpp:337 or Crash [@ js::types::CompilerOutput::isValid]

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox16 --- unaffected
firefox17 --- unaffected
firefox18 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: decoder, Assigned: nbp)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update][ion:p1:fx18][adv-main18-])

Crash Data

Attachments

(1 file, 1 obsolete file)

The following testcase asserts on mozilla-central revision e4757379b99a (run with --ion-eager):


var p = Proxy.create({
    has : function(id) {}
});
Object.prototype.__proto__ = p;
function f() {
        if (/a/.exec("a"))
            (null).default++;
}
delete RegExp.prototype.test;
f();
Valgrind shows a lot of fun stuff here, e.g.:

==16860== Invalid read of size 1
==16860==    at 0x80C405F: js::types::TypeCompartment::addPendingRecompile(JSContext*, js::types::RecompileInfo const&) (jsinfer.cpp:2455)
==16860==    by 0x80C4607: AddPendingRecompile(JSContext*, JSScript*, unsigned char*, RecompileKind) (jsinfer.cpp:2004)
==16860==    by 0x806CB59: js::types::TypeCompartment::resolvePending(JSContext*) (jsinferinlines.h:1010)
==16860==    by 0x807664D: js::types::TypeSet::addType(JSContext*, js::types::Type) (jsinferinlines.h:1329)
==16860==    by 0x80BE45F: js::types::TypeMonitorResult(JSContext*, JSScript*, unsigned char*, JS::Value const&) (jsinfer.cpp:5325)
==16860==    by 0x80D479F: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinferinlines.h:792)
==16860==    by 0x8C5B0F3: ???
==16860==  Address 0x8c8f984 is 4 bytes before a block of size 32 alloc'd
==16860==    at 0x7C01876: malloc (vg_replace_malloc.c:236)
==16860==    by 0x8229E24: js::VectorImpl<js::types::CompilerOutput, 0u, js::TempAllocPolicy, false>::growTo(js::Vector<js::types::CompilerOutput, 0u, js::TempAllocPolicy>&, unsigned int) (Utility.h:153)
==16860==    by 0x82A82F7: _ZN2js6VectorINS_5types14CompilerOutputELj0ENS_15TempAllocPolicyEE13growStorageByEj.clone.330 (Vector.h:612)
==16860==    by 0x82A8A7D: bool js::ion::IonCompile<&(js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (Vector
.h:772)
==16860==    by 0x82A8C96: js::ion::MethodStatus js::ion::Compile<&(js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&))>(JSContext*, JSScript*, JSFunction*, unsigned char*,
 bool) (Ion.cpp:1154)
==16860==    by 0x82A8DD6: js::ion::CanEnter(JSContext*, JSScript*, js::StackFrame*, bool) (Ion.cpp:1252)
==16860==    by 0x80E01BF: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:297)
==16860==    by 0x80E03B6: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jsinterp.cpp:378)
==16860==    by 0x80E09F1: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) (jsinterp.h:109)
==16860==    by 0x811A588: Trap1(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<int>, JS::Value*) (jsproxy.cpp:654)
==16860==    by 0x92126BF: ???


There was also some glibc abort within the output during minimization, so I'm assuming sec-critical here.
Blocks: IonFuzz
Keywords: crash, sec-critical
Whiteboard: [jsbugmon:update]
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
I also got a test with this crash trace instead, but it also involves a Proxy and triggers the same assertion in debug builds:


==18606== Invalid read of size 1
==18606==    at 0x64C73F: bool js::ion::IonCompile<&(js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (jsinferinlines.h:389)
==18606==    by 0x64C9FC: js::ion::MethodStatus js::ion::Compile<&(js::ion::TestCompiler(js::ion::IonBuilder*, js::ion::MIRGraph*, js::ion::AutoDestroyAllocator&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (Ion.cpp:1154)
==18606==    by 0x64CB74: js::ion::CanEnter(JSContext*, JSScript*, js::StackFrame*, bool) (Ion.cpp:1252)
==18606==    by 0x492672: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2479)
==18606==    by 0x49659C: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:324)
==18606==    by 0x497531: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (jsinterp.cpp:509)
==18606==    by 0x41910A: JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) (jsapi.cpp:5726)
==18606==    by 0x419652: JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, char const*, unsigned long, JS::Value*) (jsapi.cpp:5741)
==18606==    by 0x420660: JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, char const*, JS::Value*) (jsapi.cpp:5758)
==18606==    by 0x40835D: Load(JSContext*, unsigned int, JS::Value*) (js.cpp:725)
==18606==    by 0x4966F1: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:372)
==18606==    by 0x48B769: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2454)
==18606==  Address 0x100616f3d8 is not stack'd, malloc'd or (recently) free'd
Crash Signature: [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile]
This patch use the native inlining method to substitute regexp_exec by regexp_test instead of looking up for the "test" property on the regexp class.

This prevent the execution of the proxy "has" function of the reported test case.
Attachment #665241 - Flags: review?(jdemooij)
Comment on attachment 665241 [details] [diff] [review]
Remove lookupProperty to prevent interpreter reentrance.

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

This is much nicer, thanks!

::: js/src/builtin/RegExp.cpp
@@ +562,5 @@
>  }
>  
> +bool
> +js::ExecuteRegExp(JSContext *cx, RegExpExecType execType, HandleObject regexp,
> +              HandleString string, MutableHandleValue rval)

Nit: fix indentation of the last line.

@@ +601,5 @@
>  
>      /* Step 9a. */
>      if (i < 0 || i > length) {
>          reobj->zeroLastIndex();
> +        rval.address()->setNull();

Nit: rval.setNull();

@@ +613,5 @@
>          return false;
>      }
>  
>      /* Step 11 (with sticky extension). */
> +    if (re->global() || (!rval.address()->isNull() && re->sticky())) {

Nit: rval.isNull()

@@ +614,5 @@
>      }
>  
>      /* Step 11 (with sticky extension). */
> +    if (re->global() || (!rval.address()->isNull() && re->sticky())) {
> +        if (rval.address()->isNull())

Same here.

@@ +640,5 @@
> +    JSString *input = ToString(cx, (args.length() > 0) ? args[0] : UndefinedValue());
> +    if (!input)
> +        return false;
> +
> +    RootedString string(cx, input);

Nit: you can use RootedString directly:

RootedString input(cx, ToString(...));
if (!input)
    return false;

::: js/src/ion/CodeGenerator.cpp
@@ +232,5 @@
> +
> +    Register output = ToRegister(lir->output());
> +    Label notBool, end;
> +    Assembler::Condition isFalse = masm.testBoolean(Assembler::NotEqual, JSReturnOperand);
> +    masm.j(isFalse, &notBool);

Nit: masm.branchTestBoolean(Assembler::NotEqual, JSReturnOperand, &notBool);

::: js/src/ion/LIR-Common.h
@@ +1712,5 @@
> +
> +    LRegExpTest(const LAllocation &regexp, const LAllocation &string)
> +    {
> +        setOperand(0, regexp);
> +        setOperand(0, string);

s/0/1

::: js/src/ion/Lowering.cpp
@@ +1114,5 @@
> +    JS_ASSERT(ins->string()->type() == MIRType_String);
> +
> +    LRegExpTest *lir = new LRegExpTest(useRegisterAtStart(ins->regexp()),
> +                                       useRegisterAtStart(ins->string()));
> +    return define(lir, ins) && assignSafepoint(lir, ins);

s/define/defineVMReturn

::: js/src/ion/MCallOptimize.cpp
@@ +652,5 @@
>  
> +IonBuilder::InliningStatus
> +IonBuilder::inlineRegExpTest(uint32 argc, bool constructing)
> +{
> +    if (argc != 2 || constructing)

Looking at the other natives, this should be argc != 1.

@@ +658,5 @@
> +
> +    if (getInlineArgType(argc, 0) != MIRType_Object)
> +        return InliningStatus_NotInlined;
> +    if (getInlineArgType(argc, 1) != MIRType_String)
> +        return InliningStatus_NotInlined;

We should also check the return type == MIRType_Boolean, or for regexp.exec probably if the type set *contains* MIRType_Boolean since exec will return an object in the interpreter. Can you make sure the micro-benchmark in bug 764743 comment 0 does not regress?

::: js/src/ion/MIR.h
@@ +3001,5 @@
> +    }
> +
> +    AliasSet getAliasSet() const {
> +        // Changing slots of the regexp can change the behaviour of it.
> +        return AliasSet::Load(AliasSet::Slot);

It's probably best to use the default alias set. It shouldn't matter much and RegExp.test can have side-effects:

/abc(def)/.test("abcdef");
assertEq(RegExp.$1, "def");
Attachment #665241 - Flags: review?(jdemooij) → review+
From some of the signatures and the files the patch touches we're assuming this is ion-only.
(In reply to Jan de Mooij [:jandem] from comment #4)
> @@ +658,5 @@
> > +
> > +    if (getInlineArgType(argc, 0) != MIRType_Object)
> > +        return InliningStatus_NotInlined;
> > +    if (getInlineArgType(argc, 1) != MIRType_String)
> > +        return InliningStatus_NotInlined;
> 
> We should also check the return type == MIRType_Boolean, or for regexp.exec
> probably if the type set *contains* MIRType_Boolean since exec will return
> an object in the interpreter.

Ok for checking for regexp.test, but I don't understand how the returned type set of regexp.exec might contain the boolean type.  The return type of regexp.exec is either Null or an Object, even if the result of the call does not escape.

> Can you make sure the micro-benchmark in bug
> 764743 comment 0 does not regress?

./js -b _build/bug764743.js
586
runtime = 586.669 ms
This Bug can safely be open once landed because it only affect fx18 and if there is any issue it can be fix easily or disabled.

https://hg.mozilla.org/integration/mozilla-inbound/rev/564d554c4318
Crash Signature: [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile] → [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile]
CC efaust to help figuring out the Lowering issue of DOM method calls.
Crash Signature: [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile] → [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile]
- Rebase the patch, still failing on M2.
Attachment #665241 - Attachment is obsolete: true
Attachment #667180 - Flags: review+
Crash Signature: [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile] → [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile]
Comment on attachment 665241 [details] [diff] [review]
Remove lookupProperty to prevent interpreter reentrance.

Oops, not so obsolete yet.
Attachment #665241 - Attachment is obsolete: false
Crash Signature: [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile] → [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile]
Summary: Assertion failure: !cx->compartment->activeAnalysis, at jsinterp.cpp:337 or Crash [@ js::types::CompilerOutput::isValid] → IonMonkey: Assertion failure: !cx->compartment->activeAnalysis, at jsinterp.cpp:337 or Crash [@ js::types::CompilerOutput::isValid]
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1:fx18]
Comment on attachment 667180 [details] [diff] [review]
Remove lookupProperty to prevent interpreter reentrance.

Oops, bad patch.
Attachment #667180 - Attachment is obsolete: true
The problem causing the M2 failures was improper initialization of targets in jsop_call()

The line 

RootedFunction target(cx, NULL);

should be changed to:

RootedFunction target(cx, numTargets == 1 ? targets[0]->toFunction() : NULL);

This seems to solve at least this part of the problem.
Crash Signature: [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile] → [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile]
https://tbpl.mozilla.org/?tree=Try&rev=32a06e4a3e8c (try without test case)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d41ca12d2527 (with test case)
Crash Signature: [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile] → [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile]
https://hg.mozilla.org/mozilla-central/rev/d41ca12d2527
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile] → [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile]
JSBugMon: This bug has been automatically verified fixed.
Crash Signature: [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile] → [@ js::types::CompilerOutput::isValid] [@ js::ion::IonCompile]
Whiteboard: [jsbugmon:update][ion:p1:fx18] → [jsbugmon:update][ion:p1:fx18][adv-main18-]
Depends on: 849014
Group: core-security
You need to log in before you can comment on or make changes to this bug.