Closed Bug 781393 Opened 8 years ago Closed 8 years ago

Value overwriting during VM stack scanning can overwrite copied values

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox14 --- unaffected
firefox15 --- affected
firefox16 --- fixed
firefox17 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: billm, Assigned: billm)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

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();
Attached patch patchSplinter Review
This is the simpler fix we discussed.
Attachment #650688 - Flags: review?(bhackett1024)
Attachment #650688 - Flags: review?(bhackett1024) → review+
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
Closed: 8 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.
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+
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
(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?]
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-]
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.