592069 - handle deep bail from IteratorMore/method-write barrier

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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.

Comment 1

[Luke Wagner [:luke]]

Attachment: patch

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.

Comment 2

[Andreas Gal :gal]

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.

Comment 3

[Luke Wagner [:luke]]

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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

It could before scope-removal, because it could trigger GC. That no longer appears to be the case.

Comment 5

[Johnny Stenback (:jst)]

Luke, what makes this a security bug? And if it is one, what's the nature/severity of the potential exploit here?

Comment 6

[Luke Wagner [:luke]]

Attachment: patch

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.

Comment 7

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/714792d3bcb1

Comment 8

[Johnny Stenback (:jst)]

Marking sg:critical per discussion with Luke, this could lead to arbitrary code execution.

Comment 9

[Mike Shaver (:shaver emeritus)]

http://hg.mozilla.org/mozilla-central/rev/714792d3bcb1 merged Sat Sep 11 12:16:24 . Does it need to be on other branches?

Comment 10

[Luke Wagner [:luke]]

(In reply to comment #9)
No, we lucked out: both the IteratoreMore and MethodWriteBarrier call were added post 1.9.2.