564937 - fast iterators shouldn't touch regs.sp[0]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: fix

Stack code shouldn't read/write regs.sp[0] (or anything higher).  With bug 540706, these slots may be used by a more-recently-pushed js::CallStack, JSStackFrame, or arg slots.  In particular, using regs.sp[0] as the rval of a js_InternalInvoke is bad.  The fact that regs.sp[0] is not rooted (and IteratorMore assumes it is) suggests that this is a bug even without the contiguous stack patch.

Comment 1

[Andreas Gal :gal]

Comment on attachment 444507 [details] [diff] [review]
fix

Tryserver carefully, lots of interpreter and tracer dependencies here, but looks good.

Comment 2

[Jeff Walden [:Waldo]]

Marking sensitive out of an excess of caution; treating unrooted locations as rooted is GC-hazardous.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 444507 [details] [diff] [review]
fix

Sorry I missed this, should have caught it.

/be

Comment 4

[Luke Wagner [:luke]]

So I did find this:

  cx->regs->sp -= fused ? (*orig.pc == JSOP_MOREITER ? 0 : 2) : 1;

in TraceRecorder::checkTraceEnd.  Does that have any interaction with the change:

-    TRY_BRANCH_AFTER_COND(cond, 0);
+    TRY_BRANCH_AFTER_COND(cond, 1);

in my patch?

Comment 5

[Luke Wagner [:luke]]

Andreas and I looked over the above code and the patch seems fine as is, since the checkTraceEnd call happens before JSOP_MOREITER.

But also CC'ing Gary, perchance to fuzz.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #5)
> Andreas and I looked over the above code and the patch seems fine as is, since
> the checkTraceEnd call happens before JSOP_MOREITER.
> 
> But also CC'ing Gary, perchance to fuzz.

Patch looks good on 32-bit shells after 12 hours of dedicated fuzz.

Comment 7

[Luke Wagner [:luke]]

Thank you Gary!

http://hg.mozilla.org/tracemonkey/rev/c9138b6a4af4

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c9138b6a4af4

Comment 9

[Christian Holler (:decoder)]

Luke, did this affect 1.9.2? I assume not as bug 540706 did not affect 1.9.2 if I read correctly. Is there any reason to keep this locked?

Comment 10

[Luke Wagner [:luke]]

Nope.