Value overwriting during VM stack scanning can overwrite copied values

RESOLVED FIXED in Firefox 16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla17
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14 unaffected, firefox15 affected, firefox16 fixed, firefox17 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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();
(Assignee)

Comment 1

5 years ago
Created attachment 650688 [details] [diff] [review]
patch

This is the simpler fix we discussed.
Attachment #650688 - Flags: review?(bhackett1024)
Attachment #650688 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 2

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5bda3083952
(Assignee)

Comment 3

5 years ago
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
Attachment #650688 - Flags: approval-mozilla-beta?
Attachment #650688 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a5bda3083952
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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.
(Assignee)

Comment 6

5 years ago
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 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.
Attachment #650688 - Flags: approval-mozilla-beta?
Attachment #650688 - Flags: approval-mozilla-beta-
Attachment #650688 - Flags: approval-mozilla-aurora?
Attachment #650688 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/63c85d3cb319
status-firefox-esr10: --- → unaffected
status-firefox14: --- → unaffected
status-firefox15: --- → affected
status-firefox16: --- → fixed
status-firefox17: --- → fixed
(Assignee)

Comment 9

5 years ago
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
Depends on: 782782
Keywords: verifyme

Comment 10

5 years ago
(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?
Keywords: verifyme
Whiteboard: [qa?]
(Assignee)

Comment 11

5 years ago
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.
(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+?
Flags: in-testsuite?
Whiteboard: [qa?] → [qa-]
(Assignee)

Comment 13

5 years ago
Yes. I always forget to set that flag.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.