Closed
Bug 1240880
Opened 8 years ago
Closed 8 years ago
Assertion failure: consumer->isConsistentFloat32Use(use.use()), at js/src/jit/IonAnalysis.cpp:1791
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox45 | --- | wontfix |
firefox46 | --- | verified |
firefox-esr38 | --- | unaffected |
firefox-esr45 | 48+ | fixed |
People
(Reporter: gkw, Assigned: h4writer)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update][adv-main46+][adv-esr45.3+])
Attachments
(1 file, 1 obsolete file)
842 bytes,
patch
|
h4writer
:
review+
Sylvestre
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
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•8 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•8 years ago
|
OS: Mac OS X → All
Assignee | ||
Comment 2•8 years ago
|
||
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•8 years ago
|
||
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•8 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)
Comment 5•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/de993f2b8952
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Comment 6•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Comment 7•8 years ago
|
||
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+]
Updated•8 years ago
|
Whiteboard: [jsbugmon:update][adv-main45+] → [jsbugmon:update][adv-main46+]
Comment 8•7 years ago
|
||
(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?
Updated•7 years ago
|
Flags: needinfo?(bbouvier)
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
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.
Comment 13•7 years ago
|
||
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•7 years ago
|
||
[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 15•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(hv1989)
Updated•7 years ago
|
Whiteboard: [jsbugmon:update][adv-main46+] → [jsbugmon:update][adv-main46+][adv-esr45.3+]
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•