IonMonkey: Assertion failure: !cx->compartment->activeAnalysis, at jsinterp.cpp:334 or Crash [@ js::ion::IonCompile]

VERIFIED FIXED

Status

()

--
major
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: decoder, Assigned: djvj)

Tracking

(Blocks: 2 bugs, 4 keywords)

Other Branch
x86
Linux
assertion, crash, testcase, valgrind
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr10 unaffected)

Details

(Whiteboard: [jsbugmon:update][ion:p1:fx18], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
The following testcase asserts on ionmonkey revision 15db586f9a12 (run with --ion -n -m --ion-eager -a):


p = Proxy.create({
  has: function() {}
})
Object.prototype.__proto__ = p
function testIn() {
    var obj = { "-1": 5, "1.7": 3, "foo": 5, "1": 7 };
}
testIn()
(Reporter)

Comment 1

6 years ago
Valgrind shows several errors here that look like this:


==18507== Invalid read of size 4
==18507==    at 0x80D9A12: js::types::TypeSet::getKnownTypeTag(JSContext*) (jsinfer.cpp:1470)
==18507==    by 0x8394E82: js::ion::TypeInferenceOracle::elementWriteIsDenseArray(JSScript*, unsigned char*) (TypeOracle.cpp:389)
==18507==    by 0x834A1FB: js::ion::IonBuilder::jsop_initelem() (IonBuilder.cpp:3902)
==18507==    by 0x8354508: js::ion::IonBuilder::inspectOpcode(JSOp) (IonBuilder.cpp:909)
==18507==    by 0x8354AAC: js::ion::IonBuilder::traverseBytecode() (IonBuilder.cpp:663)
==18507==    by 0x8356722: js::ion::IonBuilder::build() (IonBuilder.cpp:344)
==18507==    by 0x83306A5: js::ion::BuildMIR(js::ion::IonBuilder&, js::ion::MIRGraph&) (Ion.cpp:691)
==18507==    by 0x833558A: bool js::ion::IonCompile<&(js::ion::TestCompiler(js::ion::IonBuilder&, js::ion::MIRGraph&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (Ion.cpp:833)
==18507==    by 0x8335C0B: js::ion::CanEnter(JSContext*, JSScript*, js::StackFrame*, bool) (Ion.cpp:986)
==18507==    by 0x80F6EC2: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2491)
==18507==    by 0x8CFA770: ???
==18507==  Address 0x8d2cd70 is 16 bytes before a block of size 64 alloc'd
==18507==    at 0x779C82F: malloc (vg_replace_malloc.c:236)
==18507==    by 0x80CE1C6: js::Vector<js::types::CompilerOutput, 0u, js::TempAllocPolicy>::growStorageBy(unsigned int) (Utility.h:157)
==18507==    by 0x83356D4: bool js::ion::IonCompile<&(js::ion::TestCompiler(js::ion::IonBuilder&, js::ion::MIRGraph&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (Vector.h:772)
==18507==    by 0x8335C0B: js::ion::CanEnter(JSContext*, JSScript*, js::StackFrame*, bool) (Ion.cpp:986)
==18507==    by 0x80F6EC2: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2491)
==18507==    by 0x8CFA770: ???
==18507== 
==18507== Invalid read of size 4
==18507==    at 0x83355BC: bool js::ion::IonCompile<&(js::ion::TestCompiler(js::ion::IonBuilder&, js::ion::MIRGraph&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (jsscript.h:627)
==18507==    by 0x8335C0B: js::ion::CanEnter(JSContext*, JSScript*, js::StackFrame*, bool) (Ion.cpp:986)
==18507==    by 0x80F6EC2: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2491)
==18507==    by 0x8CFA770: ???
==18507==  Address 0x8d2cd70 is 16 bytes before a block of size 64 alloc'd
==18507==    at 0x779C82F: malloc (vg_replace_malloc.c:236)
==18507==    by 0x80CE1C6: js::Vector<js::types::CompilerOutput, 0u, js::TempAllocPolicy>::growStorageBy(unsigned int) (Utility.h:157)
==18507==    by 0x83356D4: bool js::ion::IonCompile<&(js::ion::TestCompiler(js::ion::IonBuilder&, js::ion::MIRGraph&))>(JSContext*, JSScript*, JSFunction*, unsigned char*, bool) (Vector.h:772)
==18507==    by 0x8335C0B: js::ion::CanEnter(JSContext*, JSScript*, js::StackFrame*, bool) (Ion.cpp:986)
==18507==    by 0x80F6EC2: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2491)
==18507==    by 0x8CFA770: ???


Memory corruption => S-s
Crash Signature: [@ js::ion::IonCompile]
Keywords: crash, valgrind
Summary: IonMonkey: Assertion failure: !cx->compartment->activeAnalysis, at jsinterp.cpp:334 → IonMonkey: Assertion failure: !cx->compartment->activeAnalysis, at jsinterp.cpp:334 or Crash [@ js::ion::IonCompile]
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1:fx18]
(Assignee)

Updated

6 years ago
Assignee: general → kvijayan
(Assignee)

Comment 2

6 years ago
Created attachment 650279 [details] [diff] [review]
Fix.

The issue here was that IonBuilder, when compiling jsop_initprop, called |LookupPropertyWithFlags| on the template object, which can call |lookupGeneric| to do its work.

In this test case, a proxy object is installed as the base __proto__ for all regularly constructed objects.  So the templateObject lookup hits a proxy with a |has| trap, and that runs into an ASSERT on InvokeKernel about not being called in the middle of analysis.

Other parts of the IonBuilder which call lookupGeneric ensure that the object they're calling it on is native and has no hooks lookup hooks installed (on either it or one of its prototypes).

Fix just refactors that check into a separate, cumbersomely named function, and adds the check to the jsop_initprop compiler.
Attachment #650279 - Flags: review?
(Assignee)

Comment 3

6 years ago
Created attachment 650280 [details] [diff] [review]
Fix with review.

See previous comment.  Same patch.
Attachment #650279 - Attachment is obsolete: true
Attachment #650279 - Flags: review?
Attachment #650280 - Flags: review?
Comment on attachment 650280 [details] [diff] [review]
Fix with review.

djvj meant to give this review to mjrosenb, which previously failed because mjrosenb was not on cc. Fixed.
Attachment #650280 - Flags: review? → review?(mrosenberg)
Comment on attachment 650280 [details] [diff] [review]
Fix with review.

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

::: js/src/ion/IonBuilder.cpp
@@ +3939,5 @@
>  
> +static bool
> +CanEffectlesslyCallLookupGenericOnObject(JSObject *obj)
> +{
> +    JSObject *pobj = obj;

minor nit, you don't need a temp here (what does the 'p' stand for?)

@@ -4277,1 @@
>  

ahh, it was copied verbatim from here :)
Attachment #650280 - Flags: review?(mrosenberg) → review+
(Assignee)

Comment 6

6 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/d142f2087917
https://hg.mozilla.org/projects/ionmonkey/rev/723b214b3512 (fix nit)
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

6 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 7

6 years ago
JSBugMon: This bug has been automatically verified fixed.
status-firefox-esr10: --- → unaffected
Group: core-security
You need to log in before you can comment on or make changes to this bug.