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

VERIFIED FIXED in Firefox -esr45



2 years ago
2 years ago


(Reporter: decoder, Assigned: jandem)


(Blocks: 1 bug, 5 keywords)

assertion, jsbugmon, regression, sec-high, testcase

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr4550+ fixed, firefox50+ verified, firefox51+ verified, firefox52+ verified)


(Whiteboard: [jsbugmon:update][adv-main50+][adv-esr45.5+])


(1 attachment, 1 obsolete attachment)



2 years ago
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);


 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.
I'll just mark this sec-high as a guess.

Jan, could you triage this? Thanks.
Component: JavaScript Engine → JavaScript Engine: JIT
Flags: needinfo?(jdemooij)
Keywords: sec-high

Comment 2

2 years ago
Created attachment 8794172 [details] [diff] [review]

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?
Assignee: nobody → jdemooij
Flags: needinfo?(jdemooij)
Attachment #8794172 - Flags: review?(bhackett1024)
Comment on attachment 8794172 [details] [diff] [review]

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.
Attachment #8794172 - Flags: review?(bhackett1024) → review+

Comment 4

2 years ago
Created attachment 8794217 [details] [diff] [review]

Different patch to limit the number of type sets.
Attachment #8794172 - Attachment is obsolete: true
Attachment #8794217 - Flags: review?(bhackett1024)
Attachment #8794217 - Flags: review?(bhackett1024) → review+
How far back does this go?
status-firefox50: --- → ?
status-firefox52: --- → affected
status-firefox-esr45: --- → ?
Flags: needinfo?(jdemooij)

Comment 6

2 years ago
(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.
status-firefox49: --- → affected
status-firefox50: ? → affected
status-firefox-esr45: ? → affected
tracking-firefox52: --- → ?
Flags: needinfo?(jdemooij)
status-firefox49: affected → wontfix
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox-esr45: --- → ?

Comment 7

2 years ago
Comment on attachment 8794217 [details] [diff] [review]

[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?

> Which older supported branches are affected by this flaw?

> 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.
Attachment #8794217 - Flags: sec-approval?
Comment on attachment 8794217 [details] [diff] [review]

sec-approval+ for trunk. Please make and nominate patches for Aurora, Beta, and ESR45. We'll want this everywhere, I think.
Attachment #8794217 - Flags: sec-approval? → sec-approval+
tracking-firefox50: ? → +
tracking-firefox51: ? → +
tracking-firefox52: ? → +
tracking-firefox-esr45: ? → 50+

Comment 10

2 years ago
Comment on attachment 8794217 [details] [diff] [review]

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.
Attachment #8794217 - Flags: approval-mozilla-esr45?
Attachment #8794217 - Flags: approval-mozilla-beta?
Attachment #8794217 - Flags: approval-mozilla-aurora?
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8794217 [details] [diff] [review]

Fix a sec-high, taking it on all branches
Attachment #8794217 - Flags: approval-mozilla-esr45?
Attachment #8794217 - Flags: approval-mozilla-esr45+
Attachment #8794217 - Flags: approval-mozilla-beta?
Attachment #8794217 - Flags: approval-mozilla-beta+
Attachment #8794217 - Flags: approval-mozilla-aurora?
Attachment #8794217 - Flags: approval-mozilla-aurora+
status-firefox51: affected → fixed
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
Flags: needinfo?(jdemooij)
Group: javascript-core-security → core-security-release


2 years ago
status-firefox50: fixed → verified
status-firefox51: fixed → verified
status-firefox52: fixed → verified

Comment 16

2 years ago
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
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main50+][adv-esr45.5+]
Whiteboard: [jsbugmon:update,bisect][adv-main50+][adv-esr45.5+] → [jsbugmon:update][adv-main50+][adv-esr45.5+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.