1240880 - Assertion failure: consumer->isConsistentFloat32Use(use.use()), at js/src/jit/IonAnalysis.cpp:1791

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision b67316254602 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

function f(x) {
    "use strict";
    Math.fround(this === x && x);
}
f();
f();
(function() {
    f(Math.fround(0));
})();

Backtrace:

0   js-dbg-64-dm-darwin-b67316254602	0x0000000100232e64 js::jit::ApplyTypeInformation(js::jit::MIRGenerator*, js::jit::MIRGraph&) + 6068 (IonAnalysis.cpp:1791)
1   js-dbg-64-dm-darwin-b67316254602	0x000000010022a50b js::jit::OptimizeMIR(js::jit::MIRGenerator*) + 1835 (Ion.cpp:1634)
2   js-dbg-64-dm-darwin-b67316254602	0x0000000100235f53 js::jit::CompileBackEnd(js::jit::MIRGenerator*) + 67 (Ion.cpp:2011)
3   js-dbg-64-dm-darwin-b67316254602	0x000000010023804b js::jit::Compile(JSContext*, JS::Handle<JSScript*>, js::jit::BaselineFrame*, unsigned char*, bool, bool) + 4587 (Ion.cpp:2282)
4   js-dbg-64-dm-darwin-b67316254602	0x00000001002389b2 js::jit::CanEnter(JSContext*, js::RunState&) + 370 (Ion.cpp:2614)
5   js-dbg-64-dm-darwin-b67316254602	0x0000000100703f61 js::RunScript(JSContext*, js::RunState&) + 289 (Interpreter.cpp:405)
6   js-dbg-64-dm-darwin-b67316254602	0x00000001006f21b9 js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 841 (Interpreter.cpp:499)
7   js-dbg-64-dm-darwin-b67316254602	0x000000010071b4db js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) + 555 (Interpreter.cpp:533)
8   js-dbg-64-dm-darwin-b67316254602	0x0000000100197ddf js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 2655 (BaselineIC.cpp:6186)
9   ???                           	0x0000000102a213ab 0 + 4339143595
10  ???                           	0x0000000103e54d58 0 + 4360326488


Setting s-s as a start as this seems to involve MIR.

Comment 1

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2db399cd414f
parent:      257780:0760af2a400f
user:        Hannes Verschore
date:        Fri Aug 14 11:46:28 2015 +0200
summary:     Bug 1193112: IonMonkey - Let the float32 optimization work with Float32, r=bbouvier

Hannes, is bug 1193112 a likely regressor?

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Patch

Only do these conversions to float32, if the input is typed MIRType_Double; Else this can be problematic like in this example. We decide the MFilterTypeSet should return a "Float32", while it typeset is empty and should be MIRType_Value.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

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

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

If the checks in canProduceFlaot32/canConsumeFloat32 are necessary, please let me know. That seems overly conservative at first glance, but I might be proven wrong.

Also, test case please? :-)

::: js/src/jit/MIR.cpp
@@ +2443,5 @@
>  
>  void
>  MFilterTypeSet::trySpecializeFloat32(TempAllocator& alloc)
>  {
> +    if (!IsFloatingPointType(type()))

This shouldn't be needed: trySpecializeFloat32 is called only if isFloatCommutative returns true. So there's no way we can end up being here if !IsFloatingPointType(type()) as isFloatCommutative would have returned false.

@@ +2456,5 @@
>  
>  bool
>  MFilterTypeSet::canProduceFloat32() const
>  {
> +    if (!IsFloatingPointType(type()))

I don't think this is necessary.

@@ +2469,5 @@
>  
>  bool
>  MFilterTypeSet::canConsumeFloat32(MUse* operand) const
>  {
> +    if (!IsFloatingPointType(type()))

And this isn't as well.

::: js/src/jit/MIR.h
@@ +12597,5 @@
>  
> +    bool isFloat32Commutative() const override {
> +        if (!IsFloatingPointType(type()))
> +            return false;
> +        return true;

Can simply be:

 return IsFloatingPointType(type());

Comment 4

[Hannes Verschore [:h4writer]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Comment on attachment 8710362 [details] [diff] [review]
> Patch
> 
> Review of attachment 8710362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If the checks in canProduceFlaot32/canConsumeFloat32 are necessary, please
> let me know. That seems overly conservative at first glance, but I might be
> proven wrong.
> 
> Also, test case please? :-)

needinfo myself to upload testcase later.

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/de993f2b8952

Comment 6

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 7

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

Why was this checked in without sec-approval?

From the date in comment 1, this probably affects ESR45 (and affected Firefox 45) and should have gone through getting a security rating and sec-approval before checkin.

Can you please set the sec-approval? flag and answer the template questions?

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to Hannes Verschore [:h4writer] from comment #4)
> needinfo myself to upload testcase later.

You can use the "in-testsuite?" flag and then have a query/whine that shows bugs assigned to you in the in-testsuite? state.

(In reply to Phil Ringnalda (:philor) from comment #5)
> https://hg.mozilla.org/mozilla-central/rev/de993f2b8952

This seems to be only part of the patch attached to this bug -- what happened to the MIR.cpp bits?

Comment 9

[Phil Ringnalda (:philor)]

No idea, the full extent of my involvement with it was typing "hg pull m-i && hg merge && hg commit -m "Merge m-i to m-c, a=merge""

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Not sure why I have been needinfo'd, did you have any specific question in mind that I could help answering?

For what it's worth, the review in comment 3 seems to indicate that the MIR.cpp bits were not needed after all. I'd try pinging h4writer on irc about this bug, as I can see he still has a needinfo on this particular bug.

Comment 11

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Any update here? Does this need to be on ESR? And is there something still missing? 
Eric, can you help resolve this? We need sec-approval request and also if this is needed in ESR, an uplift request.

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

This will have to wait for ESR 45.3.0 since we are going to build on ESR 45.2.0 tonight.

Comment 13

[Eric Faust [:efaust]]

Looks like Hannes is already ni? here. I will ask him to nominate for sec-approval and uplifts all around. Leaving the ni? for reminder.

Comment 14

[Hannes Verschore [:h4writer]]

Attachment: Updated patch (works also on ESR45)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:
Wrong JS values, maybe crashes and I wouldn't rule out exploitable (but I have no POC and haven't tried to come up with one) with easy fix.

Fix Landed on Version:
FF46

Risk to taking this patch (and alternatives if risky): 
Very low. Patch is very small and has been working in FF46. No additional fixes were needed on top of this bug.

String or UUID changes made by this patch: 
/

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

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8759612 [details] [diff] [review]
Updated patch (works also on ESR45)

let's take it for esr 45.3.0

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/7ddd4faf5f7c