Closed Bug 825705 Opened 7 years ago Closed 7 years ago

IonMonkey: Crash [@ JSScript::ensureRanAnalysis] or [@ AnalyzeNewScriptProperties] or [@ js_CreateThisForFunctionWithProto] or "Assertion failure: JS_ObjectIsFunction(0, this)," or "Assertion failure: JS_ObjectIsFunction(__null, this),"

Categories

(Core :: JavaScript Engine, defect, critical)

x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox17 --- unaffected
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [adv-main20-])

Crash Data

Attachments

(3 files, 1 obsolete file)

Attached file stack from Windows 7
evalcx("\
    var x = newGlobal().Object;\
    function f() { return new x; }\
    f();\
    f();\
", newGlobal());

asserts js debug shell on m-c changeset 0bb4773db082 with --ion-eager at Assertion failure: JS_ObjectIsFunction(0, this),

Jesse helped reduce this.

s-s because I had a lot of testcases that show this assertion, but which are difficult to reproduce.
The testcase in comment 0 is 32-bit only. I may have a 64-bit testcase soon.

(gdb) x/i $pc
=> 0x80cd8f7 <JSScript::ensureRanAnalysis(JSContext*, JS::Handle<JSScript*>)+23>:	mov    0x24(%eax),%ecx
(gdb) x/b $eax
0xffffff87:	Cannot access memory at address 0xffffff87
(gdb) x/b $ecx
0xffffbd0c:	0x87
(gdb)

Inspection shows weird memory address 0xffffff87 being accessed. Assuming sec-critical.

autoBisect is now running.
Keywords: sec-critical
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116169:7e44aec095e3
user:        Till Schneidereit
date:        Wed Oct 10 22:53:51 2012 +0200
summary:     Bug 784293 - Support creating and lazily cloning arbitrary objects in self-hosted code. r=jwalden

Bug 784293 only landed in Firefox 20, so this does not seem to affect branches.
Crash Signature: [@ JSScript::ensureRanAnalysis] → [@ JSScript::ensureRanAnalysis] [@ AnalyzeNewScriptProperties]
Summary: IonMonkey: Crash [@ JSScript::ensureRanAnalysis] or "Assertion failure: JS_ObjectIsFunction(0, this)," → IonMonkey: Crash [@ JSScript::ensureRanAnalysis] or [@ AnalyzeNewScriptProperties] or "Assertion failure: JS_ObjectIsFunction(0, this),"
Summary: IonMonkey: Crash [@ JSScript::ensureRanAnalysis] or [@ AnalyzeNewScriptProperties] or "Assertion failure: JS_ObjectIsFunction(0, this)," → IonMonkey: Crash [@ JSScript::ensureRanAnalysis] or [@ AnalyzeNewScriptProperties] or "Assertion failure: JS_ObjectIsFunction(0, this)," or "Assertion failure: JS_ObjectIsFunction(__null, this),"
Crash Signature: [@ JSScript::ensureRanAnalysis] [@ AnalyzeNewScriptProperties] → [@ JSScript::ensureRanAnalysis] [@ AnalyzeNewScriptProperties] [@ js_CreateThisForFunctionWithProto]
Summary: IonMonkey: Crash [@ JSScript::ensureRanAnalysis] or [@ AnalyzeNewScriptProperties] or "Assertion failure: JS_ObjectIsFunction(0, this)," or "Assertion failure: JS_ObjectIsFunction(__null, this)," → IonMonkey: Crash [@ JSScript::ensureRanAnalysis] or [@ AnalyzeNewScriptProperties] or [@ js_CreateThisForFunctionWithProto] or "Assertion failure: JS_ObjectIsFunction(0, this)," or "Assertion failure: JS_ObjectIsFunction(__null, this),"
Please check that the patches fix this testcase as well.
I think autobisect is wrong and this is a regression from bug 813773. CreateThis assumes the callee is an object, but here it's a proxy.
Blocks: 813773
No longer blocks: 784293
(In reply to Jan de Mooij [:jandem] from comment #4)
> CreateThis assumes the callee is an object, but here it's a proxy.

Er, it assumes the callee is a *function*.
There's another problem with proxies, they can intercept the "get obj.prototype" we use. The following testcase throws an exception with --no-jm but shouldn't throw:

function f() {
    for (var i=0; i<100; i++)
	new O();
}
var O = new Proxy(function() {}, {
    get: function() {
	throw "get trap";
    }
});
f();
Assignee: general → hv1989
CreateThis can now get constructed with and without "prototype". For unknown callee we use the one without "prototype". Therefore we are now only getting the callee.prototype when needed (and allowed)
Attachment #697451 - Flags: review?(jdemooij)
Updated as requested on IRC
Attachment #697451 - Attachment is obsolete: true
Attachment #697451 - Flags: review?(jdemooij)
Attachment #697496 - Flags: review?(jdemooij)
Comment on attachment 697496 [details] [diff] [review]
Don't get prototype when callee is unknown

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

Looks good, r=me with comments below addressed.

::: js/src/ion/Lowering.cpp
@@ +243,5 @@
>  
>  bool
> +LIRGenerator::visitCreateThisWithProto(MCreateThisWithProto *ins)
> +{
> +    LCreateThisO *lir = new LCreateThisO(useRegisterOrConstantAtStart(ins->getCallee()),

Nit: you can now rename LCreateThisO to LCreateThisWithProto and LCreateThisV to LCreateThis

::: js/src/ion/TypePolicy.h
@@ +175,5 @@
>          return staticAdjustInputs(ins);
>      }
>  };
>  
> +// Combine multiple policies.

You can remove this class since it's unused now.
Attachment #697496 - Flags: review?(jdemooij) → review+
Comment on attachment 697496 [details] [diff] [review]
Don't get prototype when callee is unknown

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

::: js/src/ion/CodeGenerator.cpp
@@ -2044,5 @@
>  
>  bool
> -CodeGenerator::emitCreateThisVM(LInstruction *lir,
> -                                const LAllocation *proto,
> -                                const LAllocation *callee)

Also remove the class definition of this method in CodeGenerator.h
https://hg.mozilla.org/mozilla-central/rev/67e44e98555c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 827082
Status: RESOLVED → VERIFIED
Crash Signature: [@ JSScript::ensureRanAnalysis] [@ AnalyzeNewScriptProperties] [@ js_CreateThisForFunctionWithProto] → [@ JSScript::ensureRanAnalysis] [@ AnalyzeNewScriptProperties] [@ js_CreateThisForFunctionWithProto]
JSBugMon: This bug has been automatically verified fixed.
Depends on: 827882
Crash Signature: [@ JSScript::ensureRanAnalysis] [@ AnalyzeNewScriptProperties] [@ js_CreateThisForFunctionWithProto] → [@ JSScript::ensureRanAnalysis] [@ AnalyzeNewScriptProperties] [@ js_CreateThisForFunctionWithProto]
Whiteboard: [adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.