1303710 - Assertion failure: !unknownObject(), at js/src/vm/TypeInference-inl.h:939

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision eaf5eb6f8fa0 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-eager):

try {
    var X = Object.create(V);
} catch (exc) {}
var localstr = "";
for (var i = 0; i < 65535; ++i) localstr += X + i + "; ";
var arr = [
function() { return 0 }
, function() {}
, function() { return 2 }
];
var arg = "x";
var body = localstr
  + "arr[3] = (new Function(arg, body));"
  + "for (var i = 0; i < 4; ++i) arr[i](x-1);";
(new Function(arg, body))(1000);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x00000000006b013c in js::TypeSet::getObjectCount (this=<optimized out>) at js/src/vm/TypeInference-inl.h:939
#0  0x00000000006b013c in js::TypeSet::getObjectCount (this=<optimized out>) at js/src/vm/TypeInference-inl.h:939
#1  0x0000000000730072 in js::jit::AddObjectsForPropertyRead (obj=obj@entry=0x7fffeaff9c68, name=name@entry=0x0, observed=observed@entry=0x7fffeb700000) at js/src/jit/MIR.cpp:6052
#2  0x000000000066d70d in js::jit::IonBuilder::jsop_getelem_dense (this=0x7fffed3b21c0, obj=0x7fffeaff9c68, index=0x7fffeaff8ee0, unboxedType=<optimized out>) at js/src/jit/IonBuilder.cpp:9771
#3  0x000000000066d9c2 in js::jit::IonBuilder::getElemTryDense (this=this@entry=0x7fffed3b21c0, emitted=emitted@entry=0x7fffffffa407, obj=obj@entry=0x7fffeaff9c68, index=index@entry=0x7fffeaff8ee0) at js/src/jit/IonBuilder.cpp:9410
#4  0x000000000068fc73 in js::jit::IonBuilder::jsop_getelem (this=this@entry=0x7fffed3b21c0) at js/src/jit/IonBuilder.cpp:8955
#5  0x000000000069bf9c in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7fffed3b21c0, op=op@entry=JSOP_CALLELEM) at js/src/jit/IonBuilder.cpp:2021
#6  0x000000000069467e in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7fffed3b21c0) at js/src/jit/IonBuilder.cpp:1535
#7  0x0000000000695276 in js::jit::IonBuilder::build (this=0x7fffed3b21c0) at js/src/jit/IonBuilder.cpp:921
#8  0x00000000006a8faf in js::jit::IonCompile (cx=cx@entry=0x7ffff695f000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=<optimized out>, constructing=<optimized out>, recompile=<optimized out>, optimizationLevel=js::jit::OptimizationLevel::Normal) at js/src/jit/Ion.cpp:2238
#9  0x00000000006a98a9 in js::jit::Compile (cx=cx@entry=0x7ffff695f000, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2479
#10 0x00000000006a9abe in js::jit::CanEnter (cx=cx@entry=0x7ffff695f000, state=...) at js/src/jit/Ion.cpp:2571
#11 0x0000000000ae26eb in js::RunScript (cx=cx@entry=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:380
#12 0x0000000000ae29c5 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff695f000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:476
#13 0x0000000000ae2bf6 in InternalCall (cx=cx@entry=0x7ffff695f000, args=...) at js/src/vm/Interpreter.cpp:503
#14 0x0000000000ae2d1a in js::CallFromStack (cx=cx@entry=0x7ffff695f000, args=...) at js/src/vm/Interpreter.cpp:509
#15 0x0000000000e63e11 in js::jit::DoCallFallback (cx=0x7ffff695f000, frame=0x7fffffffae78, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffae20, res=...) at js/src/jit/BaselineIC.cpp:5998
#16 0x00007ffff7e3d08a in ?? ()
#17 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7fffeb700000	140737143373824
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffa2b0	140737488331440
rsp	0x7fffffffa2b0	140737488331440
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x1	1
r13	0x0	0
r14	0x7ffff06767f0	140737226696688
r15	0x7fffee502140	140737191616832
rip	0x6b013c <js::TypeSet::getObjectCount() const+92>
=> 0x6b013c <js::TypeSet::getObjectCount() const+92>:	movl   $0x0,0x0
   0x6b0147 <js::TypeSet::getObjectCount() const+103>:	ud2    


Marking s-s because this TI assertion was previously associated with sec-high/critical issues.

Comment 1

[Andrew McCreight [:mccr8]]

I'll just mark this sec-high as a guess.

Jan, could you triage this? Thanks.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

When there are more than 2^16 typesets in the script, multiple bytecode ops can share the last one. That triggered this assertion failure because we called AddObjectsForPropertyRead with obj->types() == observed.

Brian, do you think it would make sense to abort Ion compilation for these monstrous scripts, to avoid these edge cases?

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 8794172 [details] [diff] [review]
Patch

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

This looks fine but it would make sense, yeah, to avoid ion compiling these scripts, there are likely to be other similar cases to this one.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Different patch to limit the number of type sets.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

How far back does this go?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)
> How far back does this go?

Forever, I'm afraid. It's hard to reason about this bug, but the patch is safe, so we should just backport this everywhere.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8794217 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Hitting this assertion failure shouldn't be too hard based on the patch, especially with some fuzzing help. Not sure how hard it is to go from that to an exploit.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
All.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply or be easy to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely. We abort Ion compilation for certain very large scripts, hitting this in the wild is pretty unlikely.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8794217 [details] [diff] [review]
Patch

sec-approval+ for trunk. Please make and nominate patches for Aurora, Beta, and ESR45. We'll want this everywhere, I think.

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd845d2e110e2d928233ebd442c1e0d4fb679c3

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8794217 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Old bug.
[User impact if declined]: Crashes, security issues.
[Describe test coverage new/current, TreeHerder]: Fixes the fuzz test.
[Risks and why]: Low risk. This should only affect some very large scripts that are rare on normal websites.
[String/UUID change made/needed]: None.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6cd845d2e110

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8794217 [details] [diff] [review]
Patch

Fix a sec-high, taking it on all branches

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e289e3f73f46

Comment 14

[Carsten Book [:Tomcat]]

needs rebasing for beta (and esr i guess)

merging js/src/jit/IonBuilder.cpp
warning: conflicts while merging js/src/jit/Ion.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue

Comment 15

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dacddb224d32cd5f979fa4b53e702c7dc69be46c
https://hg.mozilla.org/releases/mozilla-esr45/rev/6f845c23565be38d7c7b11ba7a33cb9975e6a0ea

Comment 16

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx50
JSBugMon: This bug has been automatically verified fixed on Fx51