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

VERIFIED FIXED in Firefox 46

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: gkw, Assigned: h4writer)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla46
x86_64
All
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox45 wontfix, firefox46 verified, firefox-esr38 unaffected, firefox-esr4548+ fixed)

Details

(Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.3+])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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?
Blocks: 1193112
Flags: needinfo?(hv1989)
(Reporter)

Updated

2 years ago
OS: Mac OS X → All
(Assignee)

Comment 2

2 years ago
Created attachment 8710362 [details] [diff] [review]
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.
Assignee: nobody → hv1989
Flags: needinfo?(hv1989)
Attachment #8710362 - Flags: review?(bbouvier)
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());
Attachment #8710362 - Flags: review?(bbouvier) → review+
(Assignee)

Comment 4

2 years ago
(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.
Flags: needinfo?(hv1989)
https://hg.mozilla.org/mozilla-central/rev/de993f2b8952
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified

Comment 6

2 years ago
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
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?
status-firefox45: --- → wontfix
status-firefox-esr38: --- → unaffected
status-firefox-esr45: --- → affected
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main45+]
Whiteboard: [jsbugmon:update][adv-main45+] → [jsbugmon:update][adv-main46+]
(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?
tracking-firefox-esr45: --- → 47+
Flags: needinfo?(philringnalda)
Flags: in-testsuite?
Flags: needinfo?(bbouvier)
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""
Flags: needinfo?(philringnalda)
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.
Flags: needinfo?(bbouvier)
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.
Flags: needinfo?(efaustbmo)
This will have to wait for ESR 45.3.0 since we are going to build on ESR 45.2.0 tonight.
tracking-firefox-esr45: 47+ → 48+
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.
(Assignee)

Comment 14

2 years ago
Created attachment 8759612 [details] [diff] [review]
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.
Attachment #8710362 - Attachment is obsolete: true
Flags: needinfo?(efaustbmo)
Attachment #8759612 - Flags: review+
Attachment #8759612 - Flags: approval-mozilla-esr45?
Comment on attachment 8759612 [details] [diff] [review]
Updated patch (works also on ESR45)

let's take it for esr 45.3.0
Attachment #8759612 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
https://hg.mozilla.org/releases/mozilla-esr45/rev/7ddd4faf5f7c
status-firefox-esr45: affected → fixed
(Assignee)

Updated

2 years ago
Flags: needinfo?(hv1989)
Whiteboard: [jsbugmon:update][adv-main46+] → [jsbugmon:update][adv-main46+][adv-esr45.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.