Closed Bug 1562294 Opened 5 years ago Closed 5 years ago

Assertion failure: types->hasType(TypeSet::GetValueType(vp)), at js/src/jit/IonBuilder.cpp:8408

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: decoder, Assigned: iain)

References

Details

(4 keywords, Whiteboard: [jsbugmon:ignore][post-critsmash-triage][adv-main70-])

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 48f71175c999+ (build with --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --baseline-warmup-threshold=1 --ion-warmup-threshold=0):

let x = Object.getPrototypeOf(function*(){}).constructor;
var t = Object.getPrototypeOf;
monitorType(t, 1, t); t(t);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  js::jit::IonBuilder::jsop_intrinsic (this=this@entry=0x7ffff4eef238, name=<optimized out>) at js/src/jit/IonBuilder.cpp:8408
#1  0x000055555632467c in js::jit::IonBuilder::inspectOpcode (this=this@entry=0x7ffff4eef238, op=op@entry=JSOP_GETINTRINSIC) at js/src/jit/IonBuilder.cpp:2224
#2  0x00005555563259d4 in js::jit::IonBuilder::visitBlock (this=this@entry=0x7ffff4eef238, cfgblock=cfgblock@entry=0x7ffff4ef7098, mblock=<optimized out>) at js/src/jit/IonBuilder.cpp:1685
#3  0x0000555556326328 in js::jit::IonBuilder::traverseBytecode (this=this@entry=0x7ffff4eef238) at js/src/jit/IonBuilder.cpp:1600
#4  0x000055555632728e in js::jit::IonBuilder::build (this=this@entry=0x7ffff4eef238) at js/src/jit/IonBuilder.cpp:926
#5  0x0000555556335bb2 in js::jit::IonCompile (cx=<optimized out>, cx@entry=0x7ffff5519000, script=<optimized out>, baselineFrame=baselineFrame@entry=0x0, osrPc=osrPc@entry=0x0, recompile=<optimized out>, optimizationLevel=<optimized out>) at js/src/jit/Ion.cpp:1996
#6  0x0000555556336a62 in js::jit::Compile (cx=cx@entry=0x7ffff5519000, script=script@entry=..., osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2294
#7  0x0000555556336bdd in js::jit::CanEnterIon (cx=cx@entry=0x7ffff5519000, state=...) at js/src/jit/Ion.cpp:2384
#8  0x000055555635fc66 in js::jit::MaybeEnterJit (cx=0x7ffff5519000, state=...) at js/src/jit/Jit.cpp:157
#9  0x00005555558fb571 in Interpret (cx=0x7ffff5519000, state=...) at js/src/vm/Interpreter.cpp:3135
#10 0x0000555555901ab6 in js::RunScript (cx=0x7ffff5519000, state=...) at js/src/vm/Interpreter.cpp:425
[...]
#19 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11367
rax	0x555557dd9160	93825034719584
rbx	0x7ffff4eef238	140737302688312
rcx	0x555556cebba8	93825016970152
rdx	0x0	0
rsi	0x7ffff6e528b0	140737335601328
rdi	0x7ffff6e51680	140737335596672
rbp	0x7fffffffc660	140737488340576
rsp	0x7fffffffc620	140737488340512
r8	0x7ffff6e528b0	140737335601328
r9	0x7ffff7fd2d00	140737353952512
r10	0x9	9
r11	0x7ffff6ae37e0	140737332000736
r12	0x7fffffffc628	140737488340520
r13	0x7ffff4eef568	140737302689128
r14	0x21a3236289a0	36984557046176
r15	0x7fffffffc730	140737488340784
rip	0x5555562ea809 <js::jit::IonBuilder::jsop_intrinsic(js::PropertyName*)+969>
=> 0x5555562ea809 <js::jit::IonBuilder::jsop_intrinsic(js::PropertyName*)+969>:	movl   $0x0,0x0
   0x5555562ea814 <js::jit::IonBuilder::jsop_intrinsic(js::PropertyName*)+980>:	ud2

This test requires the new testing function "monitorType" added by the patch in bug 1560070. Marking s-s because tests found with this function can indicate pre-existing problems in JIT type inference.

Needinfo for :jandem since he added the added the patch.

Flags: needinfo?(jdemooij)
Type: task → defect
Priority: -- → P1

Iain, since you are working on IonBuilder TI a lot recently. Can you take a look? It looks like it should be pretty solvable.

We have a 'monitorType' shell function we'd like to add as a stress test, but it is tripping up IonBuilder::jsop_intrinsic(). The current code (https://searchfox.org/mozilla-central/rev/da855d65d1fbdd714190cab2c46130f7422f3699/js/src/jit/IonBuilder.cpp#8401-8408) is a making assumptions about typeset empty/not-empty indicating certain information.

Flags: needinfo?(iireland)

Yeah I wanted to get back to this soon... I think the simplest fix is to just change that types->empty() to !types->hasType(..).

I'll take a look.

Flags: needinfo?(jdemooij)
Flags: needinfo?(iireland)

In the first version of this code, we assumed that a non-empty typeset implied that we had already executed this opcode, so the intrinsic must already exist and its type must have already been added to the typeset. We added an assertion to justify the assumption.

In bug 1000780, we improved jsop_intrinsic to check for the intrinsic's existence, because it might have been created elsewhere. The assertion remained, even though we were no longer depending on it for correctness.

The new monitorType testing function gives us another way to add to type sets, making it possible for the typeset in jsop_intrinsic to contain something other than the type of the intrinsic. That doesn't cause any correctness issues, so the right fix is just to remove the assertion and adjust the surrounding logic accordingly.

This isn't actually a sec bug, but we haven't unhidden the monitorType patches yet. Landing this fix on it's own is fine, and we are likely to unhide the monitorType bug very soon anyways.

Keywords: sec-other
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Assignee: nobody → iireland

Cristian, could you please provide a standalone testcase, if there is a need to verify this bug?

Flags: qe-verify?
Flags: needinfo?(choller)

(In reply to Brindusa Tot[:brindusat] from comment #8)

Cristian, could you please provide a standalone testcase, if there is a need to verify this bug?

That's not possible because this only showed up when testing a patch that landed after this was fixed :)

Flags: needinfo?(choller)
Flags: qe-verify? → qe-verify-
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][post-critsmash-triage]
Whiteboard: [jsbugmon:ignore][post-critsmash-triage] → [jsbugmon:ignore][post-critsmash-triage][adv-main70-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: