Assertion failure: store->index()->type() == MIRType_Int32, at jit/TypePolicy.cpp:783 or Crash on Heap near [@ EnterIon] with StructType

VERIFIED FIXED in Firefox 28

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: bbouvier)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla30
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(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)

Details

(Whiteboard: [jsbugmon:update,origRev=7133bb431eba,testComment=0])

Attachments

(3 attachments, 2 obsolete attachments)

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);
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.
Flags: needinfo?(nmatsakis)
Keywords: crash
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Error: Failed to compile specified revision 12d3ba62a599 (maybe try another?)
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
JSBugMon: Bisection requested, failed due to error: Error: Failed to compile specified revision 12d3ba62a599 (maybe try another?)
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.
Group: javascript-core-security
Flags: needinfo?(nmatsakis)
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)
The flags should be --enable-debug --enable-optimize --enable-valgrind --disable-threadsafe and it's a 32 bit Linux build.
Flags: needinfo?(choller)
I'll try again to reproduce given those flags, I have a 32 bit linux system :)
I haven't been able to reproduce.
However, given the assertion, I have suspicions that this may be related to bug 943852
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.
Let's see if JSBugMon can reproduce this on recent m-c.
Whiteboard: [jsbugmon:] → [jsbugmon:update,bisect,origRev=7133bb431eba,testComment=0]
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.
Huh! OK, I'll try a clean build.
Incidentally, I cannot even *build* with --disable-threadsafe (I get unexplained build errors). Perhaps that is the difference between our setups.
Assignee: nobody → nmatsakis
OK, I was able to reproduce! Horray.
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.
Posted patch Bug959119.diff (obsolete) — Splinter Review
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)
(I decided it may be security sensitive after all -- without the assertion, who knows what the generated code would do)
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)
This time with code changes (I think...)
Attachment #8373962 - Attachment is obsolete: true
Attachment #8374155 - Flags: review?(jdemooij)
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)
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)
(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.
(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.
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.
Assignee: nmatsakis → benj
Status: NEW → ASSIGNED
Attachment #8375480 - Flags: review?(sstangl)
Attachment #8375480 - Flags: review?(sstangl) → review+
Whiteboard: [jsbugmon:update,bisect,origRev=7133bb431eba,testComment=0] → [jsbugmon:update,origRev=7133bb431eba,testComment=0]
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.
Posted patch Updated patchSplinter Review
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: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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 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+
Group: javascript-core-security
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-
Group: core-security
You need to log in before you can comment on or make changes to this bug.