Closed Bug 592069 Opened 11 years ago Closed 11 years ago

handle deep bail from IteratorMore/method-write barrier

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: [sg:critical] fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Running this in the browser:

  function foo() {
    [1 for each (a in this)];
  }

deep bails and asserts (with the tighter assertions added by bug 539144).  The fix is to replace the hack in record_JSOP_MOREITER (and something analogous in propTail) with something that will take the deep bail path in LeaveTree.
Attached patch patch (obsolete) — Splinter Review
This fixes the failure and passes trace, ref, and browser ref tests.  The two identified problem sites were IteratoreMore and MethodWriteBarrier, so I applied the same fix to both, but, for MethodWriteBarrier, I wasn't sure that all calls were from idempotent ops; you can see the FIXME in the comment.
Attachment #470705 - Flags: review?(gal)
Attachment #470705 - Flags: review?(dvander)
Comment on attachment 470705 [details] [diff] [review]
patch

Can you get confirmation from brendan that this is true for the method write barrier? Doesn't seem so to me.
Can the MethodWriteBarrier even deep bail?  It seems like, if it could deep bail, then we are letting the user observe this optimization which I thought was transparent.  I see that deep down it is calling putProperty; I'm not sure what the semantics of "put" are.
It could before scope-removal, because it could trigger GC. That no longer appears to be the case.
Luke, what makes this a security bug? And if it is one, what's the nature/severity of the potential exploit here?
Attached patch patchSplinter Review
It looks like, as we discussed earlier, MethodWriteBarrier can only OOM from trace and js_ReportOutOfMemory already exits early if on trace, so we can avoid the deep bail call there without any trouble.  I just did guardedShapeTable.remove(obj_ins); I think that's sufficient and a guardedShapeTable.clear() isn't necessary, but please double check me.
Attachment #470705 - Attachment is obsolete: true
Attachment #470933 - Flags: review?(gal)
Attachment #470933 - Flags: review?(dvander)
Attachment #470705 - Flags: review?(gal)
Attachment #470705 - Flags: review?(dvander)
Attachment #470933 - Flags: review?(gal) → review+
Attachment #470933 - Flags: review?(dvander) → review+
http://hg.mozilla.org/tracemonkey/rev/714792d3bcb1
Whiteboard: fixed-in-tracemonkey
Marking sg:critical per discussion with Luke, this could lead to arbitrary code execution.
Whiteboard: fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/714792d3bcb1 merged Sat Sep 11 12:16:24 . Does it need to be on other branches?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to comment #9)
No, we lucked out: both the IteratoreMore and MethodWriteBarrier call were added post 1.9.2.
Duplicate of this bug: 562734
Depends on: 612523
Group: core-security
You need to log in before you can comment on or make changes to this bug.