867482 - Crash [@ js::types::Type::ObjectType] or [@ GetValueType] or Assertion failure: !val.isMagic(), at jsobj.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: debug and opt stacks

try {
    x = []
    x.length = 7
    Object.defineProperty(this, "y", {
        get: function() {
            return ([]).concat(x)
        }
    })
    Array.prototype.unshift.call(x, x)
} catch (e) {}
y[8]
Array.prototype.forEach.call(y, Array.indexOf)

crashes js opt shell on m-c changeset 1150403342b2 with --no-baseline -a --no-ion at GetValueType and crashes js debug shell at js::types::Type::ObjectType

s-s because scary memory address 0xfffb7fffffffffff is being accessed.

This requires --no-baseline, and occurs when --no-baseline was first added as a shell flag, so autoBisect is unable to get an accurate bisection.

The following variant does not crash js opt shell but asserts js debug shell at Assertion failure: !val.isMagic(), at jsobj.cpp :

x = [];
x.length = 7
Object.defineProperty(this, "y", {
    get: function() {
        return ([]).concat(x)
    }
});
Array.prototype.unshift.call(x, x)
y[1] = undefined
Array.prototype.forEach.call(y, Array.indexOf)

Comment 1

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

Not sure if it's really JM at play here - cc'ing folks.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

JM bug but IonMonkey has the same problem. When we inline arr1.concat(arr2), the result array will have arr1's type. The problem is that if arr1 is known to be packed, and arr2 is not packed, the result may not be packed but its TypeObject will say it is.

I can reproduce this crash on mozilla-beta too, so it's likely an old bug.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 744108 [details] [diff] [review]
Patch

I *think* this is a (near-)NULL-dereference worst-case, and therefore not exploitable, but probably best to treat as critical just to be sure.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not trivial, but shouldn't be too hard, especially with some fuzzing help.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? There are comments but somebody reading the code would basically get the same info.

Which older supported branches are affected by this flaw? Release, aurora, beta.

If not all supported branches, which bug introduced the flaw? Unknown, pretty sure it's an old bug.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should apply cleanly, or else very easy to backport.

How likely is this patch to cause regressions; how much testing does it need? Unlikely.

Comment 4

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 744108 [details] [diff] [review]
Patch

sec-approval+. Please nominate this for branches.

Comment 5

[bhavana bajaj [:bajaj]]

Jandem,just wanted to make sure the patch is extremely safe & low risk enough to take into our final beta ? or should be reconsider the landing here based on comment# 3 which informs it may not be exploitable and this is already out there is Release.

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c9e2aa0dd8a

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 744108 [details] [diff] [review]
Patch

(In reply to bhavana bajaj [:bajaj] from comment #5)
> or should be reconsider the landing
> here based on comment# 3 which informs it may not be exploitable and this is
> already out there is Release.

Yeah, I'm fine with taking this on aurora and not beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown.
User impact if declined: Crashes, correctness issues.
Testing completed (on m-c, etc.): On mozilla-inbound
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7c9e2aa0dd8a

Comment 9

[Daniel Veditz [:dveditz]]

ESR doesn't have IonMonkey and b2g18 has it turned off, but do we need the methodjit part of this patch on those older branches?

Comment 10

[bhavana bajaj [:bajaj]]

(In reply to Jan de Mooij [:jandem] from comment #7)
> Comment on attachment 744108 [details] [diff] [review]
> Patch
> 
> (In reply to bhavana bajaj [:bajaj] from comment #5)
> > or should be reconsider the landing
> > here based on comment# 3 which informs it may not be exploitable and this is
> > already out there is Release.
> 
> Yeah, I'm fine with taking this on aurora and not beta.

wontfixing for Fx21 based on above.

> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Unknown.
> User impact if declined: Crashes, correctness issues.
> Testing completed (on m-c, etc.): On mozilla-inbound
> Risk to taking this patch (and alternatives if risky): Low.
> String or IDL/UUID changes made by this patch: None.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/029defe81302

Comment 12

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

NI? see comment 9's question for b2g18 and esr17

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Patch for esr17 and b2g18

I couldn't reproduce the crash on esr17/b2g18 with the tests in comment 0, because forEach is not self-hosted there.

The test below crashes esr17/b2g18 though with -m -n -a and I verified this patch fixes the crash.

function ArrayForEach(callbackfn ) {
    var O = this;
    var len = (O.length >>> 0);
    for (var k = 0; k < len; k++) {
        if (k in O)
	    O[k].toString();
    }
}
try {
    x = []
    x.length = 7
    Object.defineProperty(this, "y", {
        get: function() {
            return ([]).concat(x)
        }
    })
    Array.prototype.unshift.call(x, x)
} catch (e) {}
y[8]
ArrayForEach.call(y, Array.indexOf)

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 747079 [details] [diff] [review]
Patch for esr17 and b2g18

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): JM
User impact if declined: Crashes.
Testing completed: On m-c for almost a week.
Risk to taking this patch (and alternatives if risky): Very low.
String or UUID changes made by this patch: None.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: near-NULL crashes, as far as we know not exploitable.
User impact if declined: crashes
Fix Landed on Version: 23 (last week)
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 747079 [details] [diff] [review]
Patch for esr17 and b2g18

Approved for b2g18 but we'll wait until after 5/14 to approve on esr17 that will ship with FF22.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8eac2429297c

Comment 18

[Jan de Mooij [:jandem]]

It turns out this bug is responsible for a topcrasher on Outlook.com with certain email accounts (bug 851934). Whether the browser crashes or not depends on your contact list; Firefox becomes unusable if you happen to have a "bad" Outlook.com account (it will crash every time you reply to an email etc).

So considering this bug affects real-world websites and causes top crashes, do we want to revisit our decision to not land this on beta? The patch is small and pretty safe (it disables an optimization in some cases), has been on m-c for more than a week and has been confirmed to fix the bug (bug 851934 comment 59).

Requesting needinfo? from release management.

Comment 19

[Robert Kaiser]

Well, we already have built the final 21.0 beta and release builds (modulo any extremely-high issues we'd do a last-minute respin for).
A week ago, there would have been some wiggle room for a high-value fix (as this is on the condition of fixing bug 851934), but now we're probably past that.
Though this is also a sec-crit, so if the worst case happens, it's exploitable, the aurora patch enables people to paint a bulls-eye on the problem and notice it via outlook.com crashing, we could end up with a 0-day and chemspill - so that eventuality (next to the high-volume crasher) might still give it some leverage for taking in a respin.

Comment 20

[Robert Kaiser]

Oh, on the sec-crit status - I just looked into a few crashes from bug 851934 and on all of them the breakpad tool gives them an exploitability rating of "none" so the automatic analysis says those (probably) can't be exploited.

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Robert Kaiser (:[e-mail]) from comment #19)
> Though this is also a sec-crit, so if the worst case happens, it's
> exploitable, the aurora patch enables people to paint a bulls-eye on the
> problem and notice it via outlook.com crashing

No it's not sec-critical. I wasn't sure about it when I wrote comment 3, but I can't think of any way to exploit this and that's also why we didn't take it on beta.

Comment 22

[Robert Kaiser]

I wonder if this does even need to be a hidden security bug, given what we know now. Jan, what do you think?

Comment 23

[bhavana bajaj [:bajaj]]

(In reply to Jan de Mooij [:jandem] from comment #18)
> It turns out this bug is responsible for a topcrasher on Outlook.com with
> certain email accounts (bug 851934). Whether the browser crashes or not
> depends on your contact list; Firefox becomes unusable if you happen to have
> a "bad" Outlook.com account (it will crash every time you reply to an email
> etc).
> 
> So considering this bug affects real-world websites and causes top crashes,
> do we want to revisit our decision to not land this on beta? The patch is
> small and pretty safe (it disables an optimization in some cases), has been
> on m-c for more than a week and has been confirmed to fix the bug (bug
> 851934 comment 59).

Hrm, given we may not have any more beta's for Fx21, is this safe enough to land on release-branch directly ? This means we will not get any beta testing and will be landing our fix directly for release audience.How confident are we that we will see no fallout's once this lands on Fx21 code ? 

For everyone's context ,the reason that we did not take this in last beta was because we did not know that it had any relation to the top-crasher & based on discussion in comment #7 .
> 
> Requesting needinfo? from release management.

Comment 24

[Jan de Mooij [:jandem]]

(In reply to bhavana bajaj [:bajaj] from comment #23)
> Hrm, given we may not have any more beta's for Fx21, is this safe enough to
> land on release-branch directly ? This means we will not get any beta
> testing and will be landing our fix directly for release audience.How
> confident are we that we will see no fallout's once this lands on Fx21 code
> ?

I'm very confident, also because this has been on m-c for over a week without any problems. As with any code change it's hard to be 100% sure though, but considering it fixes the crash it's probably worth it..

Comment 25

[bhavana bajaj [:bajaj]]

(In reply to Jan de Mooij [:jandem] from comment #24)
> (In reply to bhavana bajaj [:bajaj] from comment #23)
> > Hrm, given we may not have any more beta's for Fx21, is this safe enough to
> > land on release-branch directly ? This means we will not get any beta
> > testing and will be landing our fix directly for release audience.How
> > confident are we that we will see no fallout's once this lands on Fx21 code
> > ?
> 
> I'm very confident, also because this has been on m-c for over a week
> without any problems. As with any code change it's hard to be 100% sure
> though, but considering it fixes the crash it's probably worth it..

Thanks Jandem. I discussed this with Alex and both us believe that this fix needs to be on a beta and not something we can take on release directly.So although this is really worthy, it is just too late for Fx21.0 keeping in mind the general risk of regression with code change and to avoid surprises directly in our release build.

Comment 26

[Jan de Mooij [:jandem]]

(In reply to bhavana bajaj [:bajaj] from comment #25)
> Thanks Jandem. I discussed this with Alex and both us believe that this fix
> needs to be on a beta and not something we can take on release directly.So
> although this is really worthy, it is just too late for Fx21.0 keeping in
> mind the general risk of regression with code change and to avoid surprises
> directly in our release build.

Sure, that makes sense. Thanks!

Comment 27

[Robert Kaiser]

I'm setting this back to affected for 21 in case we do a point release and might want to have this as a ride-along to fix the high-ranking outlook.com crash (bug 851934).

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/468f2568342c

Comment 29

[u279076]

Marking status-firefox23:verified based on comment 12.