function f(x, y) { return (undefined >> 0) % y } for (var j = 0; j < 1; j++) { f(f(0, 1)) } asserts js debug shell on m-c changeset 81651ad5e43c with --ion-eager --ion-parallel-compile=off at Assertion failure: ins->lhs()->type() == MIRType_Int32, at jit/Lowering.cpp My configure flags are: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --with-ccache --enable-threadsafe <other NSPR options> autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/87feb441d562 user: Dan Gohman date: Mon May 05 15:02:02 2014 -0700 summary: Bug 998580 - IonMonkey: Generalize RangeAnalysis truncation to handle other kinds of paths to integer types. r=nbp Dan, is bug 998580 a likely regressor?

This is a fuzzblocker for jsfunfuzz.

for (var j = 0; j < 1; j++) { (function() { return -0 / true })() } Assertion failure: lhs->type() == MIRType_Int32, at jit/Lowering.cpp I'm guessing this testcase is also related (run with "--ion-parallel-compile=off --ion-eager"), as autoBisect points to the same regressing changeset.

Let's lock this first because bug 1006561 is s-s (crash in the wild?) and is very likely related.

Exploitable testcase found: function f(x) { return (undefined | 0) <= (null > 0) ? (x | 0) % 2147483647 | 0 : 0 } f(-2147483647) f(null) f(null) $ ./js-opt-64-dm-ts-linux-4e4e0f502969 --ion-eager --ion-parallel-compile=off w523-reduced.js Segmentation fault (core dumped) (also asserts in Assertion failure: ins->lhs()->type() == MIRType_Int32, at jit/Lowering.cpp) I'm sorry to say that, in addition to this exploitable testcase and the real-world crash in bug 1006561, plus that this is a fuzzblocker, I'm deciding to back bug 998580 out, unfortunately.

JSBugMon: The testcase found in this bug no longer reproduces (tried revision fef1a56f0237).

The underlying bug here is that the patch made MMod::truncate and MDiv::truncate set their specialization_ to MIRType_Int32 without providing an MMod::operandTruncateKind and MDiv::operandTruncateKind to arrange for their operands to be specialized to MIRType_Int32. Attached is the same patch but with MMod::operandTruncateKind and MDiv::operandTruncateKind specializations added. With this patch, all three of the testcases in this bug pass.

JSBugMon: Fix Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first good revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/23d16c2f0f67 user: Gary Kwong date: Tue May 06 22:57:59 2014 -0700 summary: Backout 87feb441d562 for causing issues. This iteration took 235.655 seconds to run.

Comment on attachment 8418827 [details] [diff] [review] fixed-truncate-kinds.patch Requesting fuzzing :-). The patch is based on 7fffbe8dc9ec9f0dc124485c112419b5a2882674.

Comment on attachment 8418827 [details] [diff] [review] fixed-truncate-kinds.patch (function(stdlib, n, heap) { "use asm"; var Uint16ArrayView = new stdlib.Uint16Array(heap) function f(i1) { i1 = i1 | 0; Uint16ArrayView[8] = (.0 > ((+(2 ^ i1)) % +2)) } })() Crashes debug shell [@ js::jit::LRecoverInfo::appendResumePoint] $ ./js-dbg-64-dm-ts-linux-8be0e21fd300-1006301-c7-ae2ba773efa6 --ion-parallel-compile=off --ion-eager 623-interesting.js Segmentation fault (core dumped) (does not crash without the patch) Configuration parameters: AR=ar sh /home/fuzz2lin/trees/mozilla-central/js/src/configure --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

Attached is a new patch which fixes the asm.js issue. When MCompare converts to integer, it may do so with the understanding that its operands can bail out if they produce a value which is not integer, but under asm.js, there are no resume points, so attempting to setup a bailout does not go well. This patch fixes the problem by having MCompare::truncate check whether there is a resume point present before converting to integer.

Comment on attachment 8419189 [details] [diff] [review] fixed-truncate-kinds.patch Review of attachment 8419189 [details] [diff] [review]: ----------------------------------------------------------------- This patch add MMod and MDiv operandTruncateKind functions, as well as preventing the optimization on MCompare when we have no way to fallback in case of mistake. When a definition is successfully MMod::truncate, it is added to the list of instructions where we are adjusting the inputs of the definition, in order to correctly truncate the operands. Truncating the operands fix the lowering issue, as we were specializing to the MMod to an Int32 operation. We need to ensure that we we specialize the operation in ::truncate, then we correctly specialize the operands of it in ::operandTruncateKind. ::: js/src/jit/RangeAnalysis.cpp @@ +2376,5 @@ > + // they have values that aren't int32. Also, in that case, type inference > + // or asm.js will have hopefully already converted relevant comparisons > + // to integer already. > + if (!block()->entryResumePoint()) > + return false; nit: I suggest that we follow the same rule as in the MIRGenerator[2]. And maybe even up-lift this function to the CompileInfo class, which is supposed to know this kind of things. Also, this should be the first check in MCompare::truncate. [2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/MIRGenerator.h#90

Comment on attachment 8419189 [details] [diff] [review] fixed-truncate-kinds.patch Note: I think we need to update AdjustTruncatedInputs such as we do not eagerly truncate inputs if we have a truncate kind of TruncateAfterBailouts.

To :gkw and :decoder; requesting fuzzing for this new patch, which is based on 4cafec48a1f0 (top of tree as of this writing). This patch has an r+ above with comments, and I'm posting an updated patch with minor changes to address the comments, so I guess it doesn't need another r?. (In reply to Nicolas B. Pierron [:nbp] from comment #14) > Comment on attachment 8419189 [details] [diff] [review] > fixed-truncate-kinds.patch > > Review of attachment 8419189 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch add MMod and MDiv operandTruncateKind functions, as well as > preventing the optimization on MCompare when we have no way to fallback in > case of mistake. > > When a definition is successfully MMod::truncate, it is added to the list of > instructions where we are adjusting the inputs of the definition, in order > to correctly truncate the operands. Truncating the operands fix the > lowering issue, as we were specializing to the MMod to an Int32 operation. > > We need to ensure that we we specialize the operation in ::truncate, then we > correctly specialize the operands of it in ::operandTruncateKind. Yes. With this patch, the operators that specialize in ::truncate are MAdd, MSub, MMul, MDiv, and MMod, and all of these operators specialize their operands in ::operandTruncateKind. > ::: js/src/jit/RangeAnalysis.cpp > @@ +2376,5 @@ > > + // they have values that aren't int32. Also, in that case, type inference > > + // or asm.js will have hopefully already converted relevant comparisons > > + // to integer already. > > + if (!block()->entryResumePoint()) > > + return false; > > nit: I suggest that we follow the same rule as in the MIRGenerator[2]. And > maybe even up-lift this function to the CompileInfo class, which is supposed > to know this kind of things. Done. I also up-lifted the function to CompileInfo. > Also, this should be the first check in MCompare::truncate. Done. (In reply to Nicolas B. Pierron [:nbp] from comment #15) > Note: I think we need to update AdjustTruncatedInputs such as we do not > eagerly truncate inputs if we have a truncate kind of TruncateAfterBailouts. Makes sense. I added code to the patch to make AdjustTrunatedInputs use MToInt32 instead of MTruncateToInt32 for the TruncateAfterBailouts case.

Comment on attachment 8419434 [details] [diff] [review] fixed-truncate-kinds.patch Tentatively marking as feedback+ after a few hours' of fuzzing - due to request backlog on my side, I think this can land soon without it eventually blowing up jsfunfuzz later.

Thanks Gary! https://hg.mozilla.org/integration/mozilla-inbound/rev/b66e279688a1

Comment on attachment 8419434 [details] [diff] [review] fixed-truncate-kinds.patch Cancelling feedback? as :gkw already responded.

This landed: https://hg.mozilla.org/mozilla-central/rev/b66e279688a1

JSBugMon: This bug has been automatically verified fixed.

This landed without sec approval?

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22) > This landed without sec approval? As identified in comment 0, this was a recent regression on mozilla-central.