1198145 - IonAssemblerBuffer::getInst is not OOM-safe

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

If the incoming offs is unassigned then getInst() may step off the end of the buffer chain with an NPE.  This can happen when OOM prevented the instruction from being allocated.  I've seen this in my usage of getInst() for ARM32 disassembly.

A cursory reading of the code suggests that getInstructionAt() in the ARM64 assembler will have the same problem since it's just a shim around getInst() and is uncritically called on buffer offset values returned from instruction emitters, but I have to dig a little deeper to verify that.

(More information coming.)

Comment 1

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b0f08976635

Comment 2

[Lars T Hansen [:lth]]

Attachment: Introduce getInstOrNull, use that instead

I pushed this with r=me to deal with OOM test failures, but of course it needs proper review, and maybe there's a better solution?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4b0f08976635

Comment 4

[Lars T Hansen [:lth]]

Reopening, adding leave-open, still hoping for a review.

Comment 5

[Jakob Stoklund Olesen [:jolesen]]

I would add an assertion to getInst(), it is not obvious that it will always crash otherwise:

iff --git a/js/src/jit/shared/IonAssemblerBuffer.h b/js/src/jit/shared/IonAssemblerBuffer.h
index be1251e..b4552e6 100644
--- a/js/src/jit/shared/IonAssemblerBuffer.h
+++ b/js/src/jit/shared/IonAssemblerBuffer.h
@@ -286,6 +286,9 @@ class AssemblerBuffer
     }
 
     Inst* getInst(BufferOffset off) {
+        // Valid offset expected, see getInstOrNull() above.
+        MOZ_ASSERT(off.assigned());
+
         const int offset = off.getOffset();
 
         // Is the instruction in the last slice?

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

Some code patterns in the ARM64 assembler are easy to get wrong when dealing with OOMs:

 BufferOffset Assembler::b(Label* label) {
   // Flush the instruction buffer if necessary before getting an offset.
   BufferOffset branch = b(0);
+  // Bail early in case of OOM.
+  if (!branch.assigned())
+    return branch;
   Instruction* ins = getInstructionAt(branch);
   VIXL_ASSERT(ins->IsUncondBranchImm());
 
   // Encode the relative offset.
   b(ins, LinkAndGetInstructionOffsetTo(branch, label));
   return branch;
 }

The pattern here is: Insert a dummy instruction just to get the offset, then insert the real instruction at the same address.

A safer, less error-prone pattern would be:

 BufferOffset Assembler::b(Label* label) {
   BufferOffset branch = nextBufferOffset();
 
   // Encode the relative offset.
   return b(LinkAndGetInstructionOffsetTo(branch, label));
 }

The nextBufferOffset() method would not insert any instructions, it would simply return the BufferOffset that would be given to the next inserted instruction (possibly flushing the buffer first). The last b() call does the actual insertion, and only when there is no OOM condition.

Comment 7

[Sean Stangl [:sstangl]]

Comment on attachment 8652201 [details] [diff] [review]
Introduce getInstOrNull, use that instead

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

Whew, sorry for the long review! I stopped paying attention to those reminder emails ever since people starting needinfo?ing me on long-term things.

Fortunately, this patch seems fine. For annotating the spew, getInstOrNull() is reasonable. I think jolesen may be changing BufferOffsets to always point to valid memory (offset 0?), but if that's the case, he can update these callsites himself.