Closed Bug 867482 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

All
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox20 --- wontfix
firefox21 + wontfix
firefox22 + fixed
firefox23 + verified
firefox-esr17 22+ fixed
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- affected

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main22+][adv-esr1707+])

Crash Data

Attachments

(3 files)

Attached file 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)
Not sure if it's really JM at play here - cc'ing folks.
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
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.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attachment #744108 - Flags: review?(bhackett1024)
Flags: needinfo?(jdemooij)
Attachment #744108 - Flags: review?(bhackett1024) → review+
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.
Attachment #744108 - Flags: sec-approval?
Comment on attachment 744108 [details] [diff] [review]
Patch

sec-approval+. Please nominate this for branches.
Attachment #744108 - Flags: sec-approval? → sec-approval+
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 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.
Attachment #744108 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/7c9e2aa0dd8a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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?
Attachment #744108 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::types::Type::ObjectType] [@ GetValueType] → [@ js::types::Type::ObjectType] [@ GetValueType]
JSBugMon: This bug has been automatically verified fixed.
NI? see comment 9's question for b2g18 and esr17
Crash Signature: [@ js::types::Type::ObjectType] [@ GetValueType] → [@ js::types::Type::ObjectType] [@ GetValueType]
Flags: needinfo?(jdemooij)
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)
Attachment #747079 - Flags: review+
Flags: needinfo?(jdemooij)
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.
Attachment #747079 - Flags: approval-mozilla-esr17?
Attachment #747079 - Flags: approval-mozilla-b2g18?
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.
Attachment #747079 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
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.
Flags: needinfo?(bbajaj)
Flags: needinfo?(akeybl)
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.
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.
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.
Keywords: sec-critical
I wonder if this does even need to be a hidden security bug, given what we know now. Jan, what do you think?
(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.
Flags: needinfo?(bbajaj)
Flags: needinfo?(akeybl)
Keywords: sec-moderate
Flags: needinfo?(jdemooij)
(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..
Flags: needinfo?(jdemooij)
(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.
(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!
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).
Attachment #747079 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Whiteboard: [adv-main22+][adv-esr1707+]
Marking status-firefox23:verified based on comment 12.
Group: core-security
You need to log in before you can comment on or make changes to this bug.