1258320 - Wrong branch in CodeGenerator::visitGetNextMapEntryForIterator.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Tooru Fujisawa [:arai]]

testcase:
  let a = new Map();
  for (let i = 0; i < 100; i++) {
    a.set(i, i);
  }
  function f() {
    let iter = a.entries();
    while (true) {
      let v = iter.next();
      if (v.done) {
        break;
      }
    }
    iter.next();
  }
  for (let i = 0; i < 1000; i++)
    f();

result:

* thread #1: tid = 0x169495f, 0x000000010442d886, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x18)
    frame #0: 0x000000010442d886
->  0x10442d886: movq   0x18(%rbp), %rsi

>     masm.branchTestPtr(Assembler::Zero, range, range, &iterDone);
> 
>     masm.load32(Address(range, ValueMap::Range::offsetOfI()), temp);
>     masm.loadPtr(Address(range, ValueMap::Range::offsetOfHashTable()), dataLength);
>     masm.load32(Address(dataLength, ValueMap::offsetOfImplDataLength()), dataLength);
>     masm.branch32(Assembler::AboveOrEqual, temp, dataLength, &iterDone);
> ...
>         masm.bind(&iterDone);
> 
>         ValueMapRangeDestruct(masm, range, temp, dataLength);
> 
>         masm.storeValue(PrivateValue(nullptr),
>                         Address(iter, NativeObject::getFixedSlotOffset(MapIteratorObject::RangeSlot)));
> 
>         masm.move32(Imm32(1), output);

iterDone is jumped from 2 cases, but 1st case means range==nullptr, and we should jump to just before |masm.move32(Imm32(1), output);| in that case.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Fix jump target in CodeGenerator::visitGetNextMapEntryForIterator.

So, the crash happens if next() is called when loop is already done, so the ValueMap::Range is already destructed and the private slot contains nullptr.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8732797 [details] [diff] [review]
Fix jump target in CodeGenerator::visitGetNextMapEntryForIterator.

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

Nice!

Comment 3

[Jan de Mooij [:jandem]]

(Not a security bug btw - near-null crashes are not exploitable. I can't open this up because it's in the core-security group instead of the JS-specific group.)

Comment 4

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

looks like I also cannot open this.

anyway, just for clarification, this is a recent m-c only regression from bug 1248289.
will land this.

Comment 5

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f835f8fde27da0129710d264584a2530c1118abf
Bug 1258320 - Fix jump target in CodeGenerator::visitGetNextMapEntryForIterator. r=jandem

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f835f8fde27d