Closed
Bug 959119
Opened 10 years ago
Closed 10 years ago
Assertion failure: store->index()->type() == MIRType_Int32, at jit/TypePolicy.cpp:783 or Crash on Heap near [@ EnterIon] with StructType
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
VERIFIED
FIXED
mozilla30
Tracking | Status | |
---|---|---|
firefox27 | --- | unaffected |
firefox28 | --- | fixed |
firefox29 | --- | fixed |
firefox30 | --- | fixed |
firefox-esr24 | --- | unaffected |
b2g18 | --- | unaffected |
b2g-v1.1hd | --- | unaffected |
b2g-v1.2 | --- | unaffected |
b2g-v1.3 | --- | fixed |
b2g-v1.3T | --- | fixed |
b2g-v1.4 | --- | fixed |
People
(Reporter: decoder, Assigned: bbouvier)
Details
(4 keywords, Whiteboard: [jsbugmon:update,origRev=7133bb431eba,testComment=0])
Attachments
(3 files, 2 obsolete files)
651 bytes,
text/plain
|
Details | |
1.87 KB,
patch
|
Details | Diff | Splinter Review | |
14.30 KB,
patch
|
bbouvier
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release-
|
Details | Diff | Splinter Review |
The following testcase asserts on mozilla-central revision 12d3ba62a599 (run with --fuzzing-safe --ion-eager): var StructType = TypedObject.StructType; var uint8 = TypedObject.uint8; var float32 = TypedObject.float32; var RgbColor1 = new StructType({r: uint8, g: uint8, b: uint8}); var RgbColor2 = new StructType({r: uint8, g: float32, b: uint8}); var Fade1 = new StructType({from: RgbColor1, to: RgbColor1}); var Fade2 = new StructType({from: RgbColor2, to: RgbColor2}); RgbColor2.prototype.add = function(c) { this.g += c; this.b += c; }; var black = new RgbColor1({r: 0, g: 0, b: 0}); var gray = new RgbColor2({r: 129, g: 128, b: 127}); gray.add(1); var fade1 = new Fade1({from: black, to: gray}); var fade2 = new Fade2(fade1); fade2.from.add(1);
Reporter | ||
Comment 1•10 years ago
|
||
Opt-crash trace: Program received signal SIGSEGV, Segmentation fault. 0xf799dd2e in ?? () #0 0xf799dd2e in ?? () #1 0x081772c6 in EnterIon (data=..., cx=0x9015950) at js/src/jit/Ion.cpp:2259 #2 js::jit::IonCannon (cx=0x9015950, state=...) at js/src/jit/Ion.cpp:2339 #3 0x083ba417 in js::RunScript (cx=0x9015950, state=...) at js/src/vm/Interpreter.cpp:399 #4 0x083bc22c in RunScript (state=..., cx=0x9015950) at js/src/vm/Interpreter.cpp:389 #5 js::Invoke (cx=0x9015950, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:482 #6 0x083bcc2d in js::Invoke (cx=0x9015950, thisv=..., fval=..., argc=1, argv=0xffffc81c, rval=...) at js/src/vm/Interpreter.cpp:519 #7 0x081868b6 in js::jit::DoCallFallback (cx=0x9015950, frame=0xffffc84c, stub=0x9029690, argc=1, vp=0xffffc80c, res=...) at js/src/jit/BaselineIC.cpp:8097 edx 0x902aa70 151169648 => 0xf799dd2e: movss %xmm1,(%edx,%edx,4) Marking s-s based on opt-crash.
Reporter | ||
Comment 2•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
Reporter | ||
Comment 3•10 years ago
|
||
JSBugMon: Cannot process bug: Error: Failed to compile specified revision 12d3ba62a599 (maybe try another?)
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Reporter | ||
Comment 4•10 years ago
|
||
JSBugMon: Bisection requested, failed due to error: Error: Failed to compile specified revision 12d3ba62a599 (maybe try another?)
Comment 5•10 years ago
|
||
I started investigating this. I have not yet reproduced the problem. I have some suspicions as to its cause but I wasn't able to confirm them yet. I'll leave the needinfo flag for the moment.
Updated•10 years ago
|
Group: javascript-core-security
Updated•10 years ago
|
status-firefox27:
--- → unaffected
status-firefox28:
--- → disabled
status-firefox29:
--- → affected
status-firefox-esr24:
--- → unaffected
Keywords: sec-high
Updated•10 years ago
|
Flags: needinfo?(nmatsakis)
Comment 6•10 years ago
|
||
I cannot replicate with my local build of 12d3ba62a599 when run with --fuzzing-safe --ion-eager on my OS X machine. decoder: can you tell me the configure flags that were used for the build? (I can try to replicate atop a Linux VM, but that may introduce its own difficulties...) If there is a reference build with associated debug info I should be downloading instead of trying to rebuild locally, feel free to clue me into where I should find that.
Flags: needinfo?(choller)
Reporter | ||
Comment 7•10 years ago
|
||
The flags should be --enable-debug --enable-optimize --enable-valgrind --disable-threadsafe and it's a 32 bit Linux build.
Flags: needinfo?(choller)
Comment 8•10 years ago
|
||
I'll try again to reproduce given those flags, I have a 32 bit linux system :)
Comment 9•10 years ago
|
||
I haven't been able to reproduce.
Comment 10•10 years ago
|
||
However, given the assertion, I have suspicions that this may be related to bug 943852
Comment 11•10 years ago
|
||
The assertion seems to be that somehow the index was not an int32. By inspection, I cannot find a current code path where this could be the case.
Reporter | ||
Comment 12•10 years ago
|
||
Let's see if JSBugMon can reproduce this on recent m-c.
Whiteboard: [jsbugmon:] → [jsbugmon:update,bisect,origRev=7133bb431eba,testComment=0]
Reporter | ||
Comment 13•10 years ago
|
||
This still reproduces for me, tested on m-c rev ecf20a2484b6, linux 32-bit build (--enable-debug --enable-optimize --enable-valgrind --disable-threadsafe), with runtime options --ion-eager.
Updated•10 years ago
|
status-firefox30:
--- → affected
Comment 14•10 years ago
|
||
Huh! OK, I'll try a clean build.
Comment 15•10 years ago
|
||
Incidentally, I cannot even *build* with --disable-threadsafe (I get unexplained build errors). Perhaps that is the difference between our setups.
Updated•10 years ago
|
Assignee: nobody → nmatsakis
Comment 16•10 years ago
|
||
OK, I was able to reproduce! Horray.
Comment 17•10 years ago
|
||
Not sure if this is security sensitive; probably not? The problem is that divisions which are already specialized to int can be "re-specialized" to float32 by the float32 process.
Comment 18•10 years ago
|
||
Avoid specialized to float32 if already specialized to int32. The typed object code generates special purpose MDiv instructions for address arithmetic and so on that later get converted from int32 to float32 by TypeAnalysis, foiling various assertions.
Attachment #8373962 -
Flags: review?(jdemooij)
Comment 19•10 years ago
|
||
(I decided it may be security sensitive after all -- without the assertion, who knows what the generated code would do)
Comment 20•10 years ago
|
||
Comment on attachment 8373962 [details] [diff] [review] Bug959119.diff Review of attachment 8373962 [details] [diff] [review]: ----------------------------------------------------------------- This patch only contains the test?
Attachment #8373962 -
Flags: review?(jdemooij)
Comment 21•10 years ago
|
||
This time with code changes (I think...)
Attachment #8373962 -
Attachment is obsolete: true
Attachment #8374155 -
Flags: review?(jdemooij)
Comment 22•10 years ago
|
||
Comment on attachment 8374155 [details] [diff] [review] Bug959119.diff Review of attachment 8374155 [details] [diff] [review]: ----------------------------------------------------------------- Isn't this also a problem for normal int32 divs in JS? Forwarding to bbouvier as he's more familiar with the float32 optimization. ::: js/src/jit/MIR.cpp @@ +1386,5 @@ > MDefinition *left = lhs(); > MDefinition *right = rhs(); > > + if (specialization() == MIRType_Int32 > + || !left->canProduceFloat32() Style nit: || goes at the end of the previous line.
Attachment #8374155 -
Flags: review?(jdemooij) → review?(benj)
Assignee | ||
Comment 23•10 years ago
|
||
Comment on attachment 8374155 [details] [diff] [review] Bug959119.diff Review of attachment 8374155 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/TypedObject/bug959119.js @@ +1,3 @@ > +// This test exposed a bug in float32 optimization. > +// The (inlined and optimized) code for `add()` created > +// MDiv instructions specialized to integers, which was I see the assertion indeed occurs in a MDiv node, but this happens in some internal hosted function, if I understand correctly. Could you add a note for that, please? ::: js/src/jit/MIR.cpp @@ +1385,5 @@ > { > MDefinition *left = lhs(); > MDefinition *right = rhs(); > > + if (specialization() == MIRType_Int32 Actually, I think that's not the right fix. For instance, adding two constant integers and storing them into a float32 array would imply a conversion with this fix, whereas we could just convert the two constants into floats at compile time and then store the result directly. This is just an example but there is a lot of potential other cases. Here, CheckUsesAreFloat32Consumers should fail: StoreTypedArrayElement is a Float32 consumer as the typed array has the Float32 type. The problem is that in this case, the *index* is being optimized, not the value we want to store. canConsumeFloat32 should take the input as an argument and check which use is made in the first place. That's a separate issue I will take care of. I was wondering why it wouldn't happen with regular stores (i.e. not on typed objects). The reason is that in IonBuilder ( http://mxr.mozilla.org/mozilla-central/source/js/src/jit/IonBuilder.cpp#7800 precisely ), we ensure that the id (key) is an integer, so there's no chance this bad case happens. In StoreScalarTypedObjectValue, there is no such coercion, hence the result. As a matter of fact, you can either add the ToInt32 in this function, make StoreTypedArrayElement a MixPolicy< currentPolicy, NoFloatPolicy<0> >, or wait for me to change the consumer checking rule, as you prefer.
Attachment #8374155 -
Flags: review?(benj)
Comment 24•10 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #23) > Comment on attachment 8374155 [details] [diff] [review] > Bug959119.diff > > Review of attachment 8374155 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jit-test/tests/TypedObject/bug959119.js > @@ +1,3 @@ > > +// This test exposed a bug in float32 optimization. > > +// The (inlined and optimized) code for `add()` created > > +// MDiv instructions specialized to integers, which was > > I see the assertion indeed occurs in a MDiv node, but this happens in some > internal hosted function, if I understand correctly. Could you add a note > for that, please? I will expand the comment, but what you wrote isn't strictly correct. The error occurs when we generate an (optimized) version of the add function -- the access to the fields generates MIR nodes doing address computations, some of which are MDiv nodes. There is no internal hosted function involved. > Actually, I think that's not the right fix. For instance, adding two > constant integers and storing them into a float32 array would imply a > conversion with this fix, whereas we could just convert the two constants > into floats at compile time and then store the result directly. This is just > an example but there is a lot of potential other cases. I considered this, but I thought that in that case the MAdd would not be specialized (yet). Perhaps I am wrong about this. In any case, your fix sounds better, the one I did seemed kind of simplistic.
Assignee | ||
Comment 25•10 years ago
|
||
(In reply to Niko Matsakis [:nmatsakis] from comment #24) > > I will expand the comment, but what you wrote isn't strictly correct. The > error occurs when we generate an (optimized) version of the add function -- > the access to the fields generates MIR nodes doing address computations, > some of which are MDiv nodes. There is no internal hosted function involved. You are right. I wrote this part of the comment before finding out the real cause and forgot to update it. I'll take care of the fix.
Assignee | ||
Comment 26•10 years ago
|
||
Sean, some context: the current canConsumeFloat32() function can be wrong for MInstructions that aren't unary instructions, as it doesn't take into account which operand we consider. For instance, MStoreTypedArray can consume a float32 when the elements node is a Float32Array, but if you've used a float32 as the index, this is wrong! This never happened as there were tricks in IonBuilder to avoid this situation (adds of ToInt32 on the index). This patch adds a MUse argument to canConsumeFloat32 so that we can retrieve the index of the use and thus forbid some uses. nbp told me that we can directly get the index' use, so it seems useful to have the MUse in the canConsumeFloat32 function. That modifies the API of the debug function isConsistentFloat32Use() as well, but it's for the good. We could directly use the index instead of the MUse but that would makes the API weirder in my opinion, and further modifications might also take advantage of other MUse's fields.
Updated•10 years ago
|
Attachment #8375480 -
Flags: review?(sstangl) → review+
Reporter | ||
Updated•10 years ago
|
Whiteboard: [jsbugmon:update,bisect,origRev=7133bb431eba,testComment=0] → [jsbugmon:update,origRev=7133bb431eba,testComment=0]
Reporter | ||
Comment 27•10 years ago
|
||
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/e31629cfa31c user: Nicholas D. Matsakis date: Mon Nov 11 20:29:53 2013 -0500 summary: Bug 937391 - Refactor prop/elem optimizations r=jandem This iteration took 1.803 seconds to run.
Assignee | ||
Comment 28•10 years ago
|
||
Carrying forward r+ from sstangl. Updated patch contains a one-liner fix for MSetElementCache (that used the 2nd input (index) instead of the 3rd (value) to check whether the use is legit). Now passes all tests locally on x86 and x64, should be good. https://hg.mozilla.org/integration/mozilla-inbound/rev/9058ac74ec28
Attachment #8375480 -
Attachment is obsolete: true
Attachment #8380640 -
Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9058ac74ec28
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Reporter | ||
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 30•10 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Assignee | ||
Comment 31•10 years ago
|
||
Comment on attachment 8380640 [details] [diff] [review] Updated patch This is affecting gecko since bug 888109 landed, but there are no cases so far that haven't been caught by the fuzzers. Asking for approval though, as it might fix correctness issues, which are more difficult to catch. [Approval Request Comment] Regression caused by (bug #): 888109 User impact if declined: correctness issues, maybe crashes Testing completed (on m-c, etc.): m-i and m-c Risk to taking this patch (and alternatives if risky): low risk String or IDL/UUID changes made by this patch: N/A
Attachment #8380640 -
Flags: approval-mozilla-release?
Attachment #8380640 -
Flags: approval-mozilla-beta?
Attachment #8380640 -
Flags: approval-mozilla-aurora?
Comment 32•10 years ago
|
||
Comment on attachment 8380640 [details] [diff] [review] Updated patch Will leave the m-r nom in case we want to pick this up as a ride-along, though at this point there is no indication we would be doing a 27.0.2
Attachment #8380640 -
Flags: approval-mozilla-beta?
Attachment #8380640 -
Flags: approval-mozilla-beta+
Attachment #8380640 -
Flags: approval-mozilla-aurora?
Attachment #8380640 -
Flags: approval-mozilla-aurora+
Comment 33•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/8035c44ef2df https://hg.mozilla.org/releases/mozilla-beta/rev/07e9d1f64d55
Comment 34•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/07e9d1f64d55
status-b2g18:
--- → unaffected
status-b2g-v1.1hd:
--- → unaffected
status-b2g-v1.2:
--- → unaffected
status-b2g-v1.3:
--- → fixed
status-b2g-v1.4:
--- → fixed
Flags: in-testsuite+
Updated•10 years ago
|
Group: javascript-core-security
Updated•10 years ago
|
status-b2g-v1.3T:
--- → fixed
Comment 35•10 years ago
|
||
Comment on attachment 8380640 [details] [diff] [review] Updated patch It is in 29 and we are going to release it next week...
Attachment #8380640 -
Flags: approval-mozilla-release? → approval-mozilla-release-
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•