Closed Bug 1258320 Opened 6 years ago Closed 6 years ago

Wrong branch in CodeGenerator::visitGetNextMapEntryForIterator.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file)

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.
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.
Assignee: nobody → arai.unmht
Attachment #8732797 - Flags: review?(jdemooij)
Comment on attachment 8732797 [details] [diff] [review]
Fix jump target in CodeGenerator::visitGetNextMapEntryForIterator.

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

Nice!
Attachment #8732797 - Flags: review?(jdemooij) → review+
(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.)
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f835f8fde27da0129710d264584a2530c1118abf
Bug 1258320 - Fix jump target in CodeGenerator::visitGetNextMapEntryForIterator. r=jandem
Group: core-security
Blocks: 1258432
https://hg.mozilla.org/mozilla-central/rev/f835f8fde27d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.