function g(x) { return (0 > (Math.max(x, x) || x)) } function f() { return g(g() >> 0) } for (var k = 0; k < 1; ++k) { f(); } crashes js debug shell on m-c changeset 0d962e459db5 with --no-threads --ion-eager at js::jit::LRecoverInfo::appendResumePoint. Debug configure flags: 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-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests === Tinderbox Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20140902125103" and the hash "1d34d6e3f5c8". The "bad" changeset has the timestamp "20140902131004" and the hash "cea25477ad0f". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1d34d6e3f5c8&tochange=cea25477ad0f Setting needinfo? from :sunfish - this may still be related to bug 1054972 again. Setting s-s because possibly-related bug 1062612 was also s-s. This has blown up the fuzzers.

I will look at it later.

The crash happens because the lastResumePoint_ of the lowering is not set. The block which does not have any resume point only contains the following instructions: (gdb) call block->dump() block12: toint3252 = toint32 minmax50 goto53 = goto block14 And the parent block ends with a MTest instruction. As a first guess I will bet on Bug 1045749, where the SplitEdge algorithm is adding a new block, and the ApplyType phase is trying to ensure that all inputs of the Phi has the type that we observed so far. I will investigate more to see if I can fix this.

(In reply to Nicolas B. Pierron [:nbp] from comment #3) > and the ApplyType phase The Range Analysis truncate phase, is adding a MToInt32 as input of the Phi (Bug 1054972)

Comment on attachment 8485861 [details] [diff] [review] IonMonkey: Copy the last resume point visited to the next basic block. Review of attachment 8485861 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/Lowering.cpp @@ +3942,5 @@ > return false; > > + // If we have a resume point check that all the following block do have one, > + // otherwise copy the last resume point as the entry resume point of the > + // basic block. This needs bigger comments about what's going on here and why we're doing it this way. How about: // If we have a resume point check that all the following blocks have one, // otherwise reuse the last resume point as the entry resume point of the // basic block. This is used to handle fallible code which is moved into // split edge blocks, which do not have resume points. // See SplitCriticalEdgesForBlock. And in SplitCriticalEdgesForBlock, change the below comment: // The entry resume point will not be used for anything, and may have // the wrong stack depth, so remove it. To: // The entry resume point won't properly reflect state at the start of the split edge, so remove it. // Split edges start out empty, but might have fallible code moved into them later. // Rather than immediately figure out a valid resume point and pc we can use for the split edge, // we wait until lowering (see LIRGenerator::visitBlock), where this will be easier. @@ +3947,5 @@ > + if (lastResumePoint_) { > + for (size_t s = 0; s < block->numSuccessors(); s++) { > + MBasicBlock *succ = block->getSuccessor(s); > + if (!succ->entryResumePoint()) { > + MOZ_ASSERT(succ->phisBegin() == succ->phisEnd()); MOZ_ASSERT(succ->isSplitEdge()); too

(In reply to Brian Hackett (:bhackett) from comment #6) > > MOZ_ASSERT(succ->isSplitEdge()); too To be honest I intended to see if we could generalize this instead of creating entry resume points all the time, and not only SplitEdge blocks. But I guess these modifications are fine for the purpose of this bug, and safer for now.

I am flagging the status of fx33 as unaffected, but keep fx34 as "?" as Bug 1054972 is not yet backported to fx34, but when it would be we would have to carry this patch over as well.

Should I land the test case with this patch, and ask for approval? Or should I just land with the test case, and backport this patch to fx34 before Bug 1054972?

Normally we don't land testcases for patches with security bugs normally.. This affects more than one branch and is sec-high so it should get sec-approval+ before landing. That said, once it is on all affected branches, since we haven't shipped the defect anywhere and it won't accidentally at that point.

The reason of my question, is that other branches are not yet affected, but we intend to backport one of the patch because of performance regression.

Ok. We should get the patches in ASAP.

(strip comments and test case)

Comment on attachment 8488596 [details] [diff] [review] IonMonkey: Copy the last resume point visited to the next basic block. > [Security approval request comment] > How easily could an exploit be constructed based on the patch? This is a null-deref, which sounds quite exploitable. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No longer. > Which older supported branches are affected by this flaw? No yet, but as soon as bug 1054972 is backported, gecko34 would be affected too. > If not all supported branches, which bug introduced the flaw? Bug 1054972 and Bug 1045749 combined. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch should be trivial to backport. > How likely is this patch to cause regressions; how much testing does it need? Unlikely.

Comment on attachment 8488596 [details] [diff] [review] IonMonkey: Copy the last resume point visited to the next basic block. sec-approval+ for trunk. Can you make sure this gets nominated for Aurora once it is necessary?

Landed with sanitised commit message: https://hg.mozilla.org/integration/mozilla-inbound/rev/a76352a051a7

JSBugMon: This bug has been automatically verified fixed.

Nicolas, could you fill the uplift request for aurora? Merci For other, Nicolas told me that this patch is necessary for the uplift of bug 1054972 in aurora.

Comment on attachment 8488596 [details] [diff] [review] IonMonkey: Copy the last resume point visited to the next basic block. Approval Request Comment [Feature/regressing bug #]: Bug 1054972 & Bug 1045749 [User impact if declined]: SEGV during some Ion compilations. [Describe test coverage new/current, TBPL]: inbound & central. [Risks and why]: Unlikely, because we are adding a missing entryResumePoint by using the last resume point of the previous block. As no MControlInstructions are effectful, this is safe to resume to the last entry resume point. [String/UUID change made/needed]: No

Comment on attachment 8488597 [details] [diff] [review] Add testcase. https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2534f78bab (test case and comments)