781393 - Value overwriting during VM stack scanning can overwrite copied values

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Here's a test case. With the patch in bug 781390, it passes with no options and fails with -m -n -a.

gczeal(4,1);
function check(o)
{
    print(o);
    assertEq(o.b, 3);
}
function f()
{
    for (var i=0; i<3; i++) {
	var o = {b: 3};
	check(o);
    }
}
f();

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This is the simpler fix we discussed.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5bda3083952

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 650688 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 750834
User impact if declined: JavaScript code could generate incorrect results.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 650688 [details] [diff] [review]
patch

What's the reach of this to users?  What kind of behaviour will be experienced on the web.  This was never nominated for tracking and it's late in FF15's beta cycle so I'm not likely to take this there at this point, but will consider uplift to Aurora if there's a serious user-facing problem here otherwise this can ride the trains.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It's hard to quantify how much this could affect people. On rare occasions, when running JavaScript code, we will overwrite a variable with an incorrect value. This can happen either in JS code from web pages or in our own chrome JS code. I see an argument for not landing it on beta, but I really think this should land on Aurora.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 650688 [details] [diff] [review]
patch

Works for me, we have more time to shake out any regressions if this goes to Aurora now and if all goes well it will be out to our GA in 7 weeks.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63c85d3cb319

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I forgot that this bug kinda depends on bug 782782 in the sense that the patch here exposes that bug on tryserver. Anyway, I landed that change here:

https://hg.mozilla.org/releases/mozilla-aurora/rev/95a9ef9dfc3d

Comment 10

[Ioana (away)]

(In reply to Bill McCloskey (:billm) from comment #6)
> It's hard to quantify how much this could affect people. On rare occasions,
> when running JavaScript code, we will overwrite a variable with an incorrect
> value. This can happen either in JS code from web pages or in our own chrome
> JS code. I see an argument for not landing it on beta, but I really think
> this should land on Aurora.

Bill, is there a way QA can trigger one of those rare occasions and verify this bug?

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Well, the test case attached in the patch will trigger it. That runs on tinderbox, so I don't think there's anything for QA to do here.

Comment 12

[u279076]

(In reply to Bill McCloskey (:billm) from comment #11)
> Well, the test case attached in the patch will trigger it. That runs on
> tinderbox, so I don't think there's anything for QA to do here.

Does that qualify for in-testsuite+?

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes. I always forget to set that flag.