Closed Bug 953114 Opened 6 years ago Closed 6 years ago

Assertion failure: MIR instruction returned value with unexpected type, at jit/IonMacroAssembler.cpp:1223

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
firefox26 --- wontfix
firefox27 + fixed
firefox28 + fixed
firefox29 + fixed
firefox-esr24 27+ fixed
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-high, testcase, Whiteboard: [jsbugmon:update,ignore][adv-main27+][adv-esr24.3+])

Attachments

(4 files)

The following testcase asserts on mozilla-central revision cd3e9359fd64 (run with --fuzzing-safe --ion-eager):


function test() {
    var arr = new Uint32Array(100);
    arr[50] = 0xffffee00;
    for (var i=0; i<200; i++)
      res = arr[i || this];
}
test();
Whiteboard: [jsbugmon:update,bisect]
Good catch. This is another TI problem with the Ion GetElement typed array stub (see also bug 929261). The result typeset is int32-or-undefined, but then we do this:

    if (output.hasValue())
        masm.loadFromTypedArray(arrayType, source, output.valueReg(), true,
                                elementReg, &popAndFail);

Passing true for allowDouble, so we'll happily read the double value (0xffffee00) out of the Uint32Array. The IC should probably have an allowDouble flag or something.
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/1b91cf5c8407
user:        Jan de Mooij
date:        Sat Dec 14 14:32:35 2013 +0100
summary:     Bug 949475 - Add some debug-only sanity checks. r=bhackett

This iteration took 1.054 seconds to run.
This doesn't sound great, so I'm setting it to sec-high.  Feel free to adjust as needed.
Keywords: sec-high
Attached patch PatchSplinter Review
Set an allowDoubleResult flag on the IC if it's okay to return a double and use that for Uint32Array.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8355508 - Flags: review?(efaustbmo)
Flags: needinfo?(jdemooij)
Comment on attachment 8355508 [details] [diff] [review]
Patch

Review of attachment 8355508 [details] [diff] [review]:
-----------------------------------------------------------------

OK, r=me with question below answered.

::: js/src/jit/MIR.cpp
@@ +2663,5 @@
>      return typedArray_->viewData();
>  }
>  
> +bool
> +MGetElementCache::allowDoubleResult() const

OK, this will work.

I was briefly considering a scheme in which we checked the state of TI at stub generation time, rather than cache creation time, and then generated sutbs based on that information, but this is simpler, if less responsive to change over time.

The only difference that I can see is that in this scheme, if we choose to go to the interpreter from the fallback path, and monitor a double result, we can then use that information to generate better stubs in the future for other typed arrays.

Is there some specific reason, other than simplicity, that we chose to check here, rather than in canAttachTypedArrayElement()?
Attachment #8355508 - Flags: review?(efaustbmo) → review+
Comment on attachment 8355508 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily, but it may be possible for somebody with some work.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
Firefox 22+.

> If not all supported branches, which bug introduced the flaw?
Bug 840696.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Shouldn't be hard.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #8355508 - Flags: sec-approval?
(As far as we know, the fuzzers never found a testcase that breaks/crashes due to this bug. That means it may not be exploitable, but in general breaking TI invariants (producing Values with unexpected types) is scary and should be considered sec-high/sec-critical just to be safe, I think.)
Comment on attachment 8355508 [details] [diff] [review]
Patch

sec-approval+ for trunk. We should get Aurora, Beta, and ESR24 patches made and nominated once it is in.
Attachment #8355508 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/91a585e70f28

(In reply to Eric Faust [:efaust] from comment #5)
> The only difference that I can see is that in this scheme, if we choose to
> go to the interpreter from the fallback path, and monitor a double result,
> we can then use that information to generate better stubs in the future for
> other typed arrays.
> 
> Is there some specific reason, other than simplicity, that we chose to check
> here, rather than in canAttachTypedArrayElement()?

I replied on IRC, but for posterity: stack/monitored TypeSets are frozen, so when we see a double the script will be invalidated anyway. The next time we compile it, we'll be able to attach the stub.

Setting needinfo for approval requests.
Flags: needinfo?(jdemooij)
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 840696.
User impact if declined: Security bugs.
Testing completed (on m-c, etc.): On m-i.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.
Attachment #8356074 - Flags: review+
Attachment #8356074 - Flags: approval-mozilla-beta?
Attachment #8356074 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jdemooij)
Attached patch Patch for ESR24Splinter Review
[Approval Request Comment]
User impact if declined: Security issues.
Fix Landed on Version: 29, will be backported to 27 and 28.
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 #8356083 - Flags: review+
Attachment #8356083 - Flags: approval-mozilla-esr24?
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 93fe56269382).
Attachment #8356074 - Flags: approval-mozilla-beta?
Attachment #8356074 - Flags: approval-mozilla-beta+
Attachment #8356074 - Flags: approval-mozilla-aurora?
Attachment #8356074 - Flags: approval-mozilla-aurora+
Attachment #8356083 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Attached patch Patch for b2g26Splinter Review
Attachment #8356187 - Flags: review+
Flags: needinfo?(jdemooij)
Flags: needinfo?(ryanvm)
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main27+][adv-esr24.3+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.