1907700 - null dereference in js::jit::Sink()

Status: RESOLVED DUPLICATE of bug 1109195

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Xiaochuan Yu [:xia0o0o0o]]

Description

In bool js::jit::Sink(js::jit::MIRGenerator *mir, js::jit::MIRGraph &graph)
When all uses of a instruction are resume point, the usesDominator will never be set [1]

      bool hasLiveUses = false;
      bool hasUses = false;
      MBasicBlock* usesDominator = nullptr;
      for (MUseIterator i(ins->usesBegin()), e(ins->usesEnd()); i != e; i++) {
        hasUses = true;
        MNode* consumerNode = (*i)->consumer();
        if (consumerNode->isResumePoint()) {
          if (!consumerNode->toResumePoint()->isRecoverableOperand(*i)) {
            hasLiveUses = true;   // [2] slot is not recoverable, set hasLiveUses
          }
          continue;               // [1] skip
        }
        // .........
      }

And if the slot is not recoverable during a bailout, hasLiveUses will be set to true [2].

      // not reachable since hasLiveUses == true
      if (!hasLiveUses) {
        MOZ_ASSERT(!usesDominator);
        ins->setRecoveredOnBailout();
        JitSpewDef(JitSpew_Sink,
                   "  No live uses, recover the instruction on bailout\n", ins);
        continue;
      }

There will be a null dereference in

while (block->loopDepth() < usesDominator->loopDepth()) {
    // .........
}

On commit 01aaa47e62a2015e7641f26ab0bc2bb00ab579b8 the following proof-of-concept crashes js shell with obj-x86_64-pc-linux-gnu/dist/bin/js --fast-warmup --ion-sink=on poc.js (or add --ion-offthread-compile=off for better reliability)

class Class1  { }
const f = (a) => { a + 1; }

function foo() {
    class Class2 extends Class1 {
        constructor() {
            super();
            // resume
            f(0);
            f({});
            for (let i = 0; i < 0x30; i++) { }
        }
    }
    for (let i = 0; i < 0x30; i++) { 
        new Class2();
    }
}
foo();
foo();
foo();

It crashes at cmp ebx, DWORD PTR [r13+0xe4] when r13 == 0x0 with the following stack trace.

[ Legend: Modified register | Code | Heap | Stack | String ]
───── registers ────
$rax   : 0x00007ffff3e94b70 
... 
$r10   : 0xffffffff
$r11   : 0x1d000
$r12   : 0x00007ffff3e959c8  →  0x00005555591a9b90  →  0x0000555558a54d90  →  <js::jit::MResumePoint::getOperand(unsigned long) const+0> push rbp
$r13   : 0x0
$r14   : 0x00007ffff3e95938  →  0x00007ffff3e96cb8  →  0x00007ffff3e966a0  →  0x00007ffff3e964c8  →  0x00007ffff3e95d88  →  0x00007ffff3e95b00  →  0x00007ffff3e95938  →  [loop detected]
$r15   : 0x00005555592bedfc  →  <__start___sancov_guards+789420> in al, dx
──── code:x86:64 ────
   0x555558283d28 <js::jit::Sink(js::jit::MIRGenerator*, js::jit::MIRGraph&)+1080> js     0x555558283e21 <_ZN2js3jit4SinkEPNS0_12MIRGeneratorERNS0_8MIRGraphE+1329>
   0x555558283d2e <js::jit::Sink(js::jit::MIRGenerator*, js::jit::MIRGraph&)+1086> mov    rax, QWORD PTR [rbp-0xa8]
   0x555558283d35 <js::jit::Sink(js::jit::MIRGenerator*, js::jit::MIRGraph&)+1093> mov    ebx, DWORD PTR [rax+0xe4]
 → 0x555558283d3b <js::jit::Sink(js::jit::MIRGenerator*, js::jit::MIRGraph&)+1099> cmp    ebx, DWORD PTR [r13+0xe4]
   0x555558283d42 <js::jit::Sink(js::jit::MIRGenerator*, js::jit::MIRGraph&)+1106> jae    0x555558283d74 <_ZN2js3jit4SinkEPNS0_12MIRGeneratorERNS0_8MIRGraphE+1156>
   0x555558283d44 <js::jit::Sink(js::jit::MIRGenerator*, js::jit::MIRGraph&)+1108> data16 data16 cs nop WORD PTR [rax+rax*1+0x0]
   0x555558283d50 <js::jit::Sink(js::jit::MIRGenerator*, js::jit::MIRGraph&)+1120> mov    r14, QWORD PTR [r13+0x128]
   0x555558283d57 <js::jit::Sink(js::jit::MIRGenerator*, js::jit::MIRGraph&)+1127> cmp    r13, r14
   0x555558283d5a <js::jit::Sink(js::jit::MIRGenerator*, js::jit::MIRGraph&)+1130> je     0x55555828445d <_ZN2js3jit4SinkEPNS0_12MIRGeneratorERNS0_8MIRGraphE+2925>
──── trace ────
[#0] 0x555558283d3b → js::jit::Sink(mir=0x7ffff3e94180, graph=@0x7ffff3e94068)
[#1] 0x5555586f9a7e → js::jit::OptimizeMIR(mir=0x7ffff3e94180)
[#2] 0x5555586fb1e1 → js::jit::CompileBackEnd(mir=0x7ffff3e94180, snapshot=0x7ffff3e94aa8)
[#3] 0x5555586fd4ab → js::jit::IonCompile(cx=0x7ffff5f3d200, script={
  <js::HandleOperations<JSScript*, JS::Handle<JSScript*> >> = {
    <js::WrappedPtrOperations<JSScript*, JS::Handle<JSScript*>, void>> = {<No data fields>}, <No data fields>},
  members of JS::Handle<JSScript*>:
  ptr = 0x7fffffffc930
}, osrPc=<optimized out>)
[#4] 0x5555586fd4ab → js::jit::Compile(cx=0x7ffff5f3d200, script={
  <js::HandleOperations<JSScript*, JS::Handle<JSScript*> >> = {
    <js::WrappedPtrOperations<JSScript*, JS::Handle<JSScript*>, void>> = {<No data fields>}, <No data fields>},
  members of JS::Handle<JSScript*>:
  ptr = 0x7fffffffc930
}, osrFrame=0x7fffffffca48, osrPc=0x7ffff5f058e8 "\227\005")
[#5] 0x5555586fe7e2 → BaselineCanEnterAtBranch(cx=0x7ffff5f3d200, osrFrame=0x7fffffffca48, pc=0x7ffff5f058e8 "\227\005", script={
  <js::HandleOperations<JSScript*, JS::Handle<JSScript*> >> = {
    <js::WrappedPtrOperations<JSScript*, JS::Handle<JSScript*>, void>> = {<No data fields>}, <No data fields>},
  members of JS::Handle<JSScript*>:
  ptr = <optimized out>
})

Possible variants

In other routines that need to traverse the uses chain, similar problem may occur, leading to variables outside the loop not being updated or simply uninitialized.

Proposed fix

Skip the loop when usesDominator is nullptr

diff --git a/js/src/jit/Sink.cpp b/js/src/jit/Sink.cpp
index b7ebeaa211f2..58fcb811cdea 100644
--- a/js/src/jit/Sink.cpp
+++ b/js/src/jit/Sink.cpp
@@ -113,6 +113,10 @@ bool Sink(MIRGenerator* mir, MIRGraph& graph) {
         continue;
       }
 
+      if (!usesDominator) {
+        continue;
+      }
+
       // This guard is temporarly moved here as the above code deals with
       // Dead Code elimination, which got moved into this Sink phase, as
       // the Dead Code elimination used to move instructions with no-live

Or update the usesDominator when hasLiveUses is set.

diff --git a/js/src/jit/Sink.cpp b/js/src/jit/Sink.cpp
index b7ebeaa211f2..d77a18f08402 100644
--- a/js/src/jit/Sink.cpp
+++ b/js/src/jit/Sink.cpp
@@ -74,6 +74,8 @@ bool Sink(MIRGenerator* mir, MIRGraph& graph) {
         if (consumerNode->isResumePoint()) {
           if (!consumerNode->toResumePoint()->isRecoverableOperand(*i)) {
             hasLiveUses = true;
+            MBasicBlock *consumerBlock = consumerNode->block();
+            usesDominator = CommonDominator(usesDominator, consumerBlock);
           }
           continue;
         }

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

Sink is only partially enabled, and turning it on has unfortunately known pit-falls. Which is why Bug 1109195 has been opened to fix these issues.

Thanks for trying the various flags, but --ion-sink=on is known to fail as of today.

Comment 2

[Iain Ireland [:iain]]

It's worth noting that --ion-sink=on doesn't actually fail on any of the tests in our test suite, so a failing testcase is valuable. (We know it's broken because we had to back it out in the past.)

The actual mechanism of the bug is somewhat interesting. Because of trial inlining, OSR, and branch pruning, we end up with a CFG where we inline the super call, inline one call to f, and immediately bail out from inside f. In this case, the outer resume point just before the call to f captures the MNewPlainObject that we allocate to pass in as this for the super call. In bug 1645835, we added code to check for non-recoverable operands in Sink. However, in this case, because of pruning, that's the only live use of the MNewPlainObject, leading to the case described in the original comment, where we don't have a usesDominator block.

If we wanted to support sinking the allocation of this, I think the second proposed fix ("Or update the usesDominator when hasLiveUses is set") would be a step in the right direction. In practice we have a variety of other restrictions on when Sink kicks in that would prevent this from having any effect. In general, it seems unlikely that sinking this would be beneficial in any non-pathological case. So we could also consider just skipping instructions with non-recoverable uses in the sink=on case. (The patch from bug 1645835 already has that effect in the sink=off case.)

Given that the non-recoverable operand code was added well after we disabled the full version of Sink, it doesn't seem possible for this to be the main problem that was causing crashes with Sink. So fixing this probably isn't a high priority.

Either way, I'm going to open this up, because it's not security-sensitive.

Thanks for the detailed report!