Crash [@ js::jit::LRecoverInfo::appendResumePoint]

VERIFIED FIXED in Firefox 32

Status

()

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

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31 unaffected, firefox32 verified, firefox-esr24 unaffected)

Details

(crash signature)

Attachments

(5 attachments)

Posted file stack
(function(stdlib, n, heap) {
    "use asm"
    var Int16ArrayView = new stdlib.Int16Array(heap)
    function f() {
        var x = 4.
        Int16ArrayView[~~((1 ? .0 : .9) % x) >> 1] = 0
        u(x)
    }
})()

crashes js debug and opt shell on m-c changeset b66e279688a1 with --ion-parallel-compile=off --ion-eager at js::jit::LRecoverInfo::appendResumePoint

My configure flags are:

AR=ar sh /home/odroid/trees/mozilla-inbound/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>

===

(gdb) x/i $pc
=> 0x11e6da <js::jit::LRecoverInfo::appendResumePoint(js::jit::MResumePoint*)+6>:       ldr     r5, [r1, #28]
(gdb) x/i $r5
   0xf079e0:    pop     {r3, r5, r7}
(gdb) x/i $r1
   0x0: Cannot access memory at address 0x0
(gdb) x/i $r3
   0xffffffff:  Cannot access memory at address 0xfffffffe
(gdb) x/i $r7
   0x2010:      Cannot access memory at address 0x2010
(gdb)

Setting s-s because this may involve accessing 0xfffffffe.
Posted file stack
(function(stdlib, n, heap) {
    "use asm";
    var abs = stdlib.Math.abs;
    var Float64ArrayView = new stdlib.Float64Array(heap)
    function f(x) {
        x = x | 0;
        x = (~~((+abs((+(0xff439dbd >> 0xffffffff)) - (0xff73ea8c ? 67108863.0 : -17179869185.0))) % 4097.0))
        Float64ArrayView[x >> 3] = 262145.0
    }
})()

crashes js debug shell on Mac with --ion-parallel-compile=off --ion-eager at js::jit::LRecoverInfo::appendResumePoint


:sunfish, is bug 998580 or bug 1006301 a likely regressor? (judging by messy bisection results due to multiple landings of that patch)
Assuming worse-case sec-critical again.
Blocks: 1006301
Flags: needinfo?(sunfish)
Keywords: sec-critical
OS: Linux → All
Hardware: ARM → All
This is likely a regressor coming after the addition of Bug 990106, where DCE eagerly make an instruction recoverable where there is no resume point to capture it.

In such case, I guess we should just remove all instructions flagged as recovered.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(sunfish)
Correction, this would be a range analysis error, I suggest we add an assertion such as we abort as soon as we enter buildSnapshot from a MIR instruction compiled with asmjs.

The above case would work seamlessly as we do not Lower these instructions.
I don't fully understand the disassembly analysis in comment 0 and why this should be sec-critical. This is loading something into r5, based on r1 right? and r1 is 0x0. If thats the case, then I think we shouldn't set sec-critical unless we're really reading/writing some bad address or have other evidence that something like this might have happened.
(In reply to Christian Holler (:decoder) from comment #5)
> I don't fully understand the disassembly analysis in comment 0 and why this
> should be sec-critical. This is loading something into r5, based on r1
> right? and r1 is 0x0. If thats the case, then I think we shouldn't set
> sec-critical unless we're really reading/writing some bad address or have
> other evidence that something like this might have happened.

From what I understand r1 is null, because we are in an AsmJS compilation.  So we should just never call buildSnapshot.  I think the problem is coming from the fact that in AsmJS we do not have fallible truncate.

So it seems that we should always truncate inputs in AsmJS, independently of the fallible case in AdjustTruncatedInputs, or we should never produce a TruncateAfterBailout when we are using AsmJS.
This modifies AdjustTruncatedInputs such as we ignore the different levels
of truncations which might exists for Ion.
Attachment #8420709 - Flags: review?(sunfish)
Comment on attachment 8420709 [details] [diff] [review]
Always truncate when compiling AsmJS code.

Review of attachment 8420709 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/RangeAnalysis.cpp
@@ +2495,5 @@
>              truncated->replaceOperand(i, input->getOperand(0));
> +        } else if (kind == MDefinition::TruncateAfterBailouts && !block->info().compilingAsmJS()) {
> +            // When we are compiling with AsmJS, we have no opportunity to bail
> +            // to anything. This means that in all cases we will flow into a
> +            // truncated operation.

This doesn't look safe. A truncated MMod doesn't fully truncate its inputs. For example, (1.1 % 1.2)|0 is not equal to ((1.1|0) % (1.2|0))|0. If we get here with an MMod in asm.js mode, I think it's already too late.

I think it would be better to disable truncation of MMod in asm.js mode in the first place, since the only thing it will be able to do is apply TruncateWithBailouts to its operands.

In fact, it may make sense to just skip RangeAnalysis::truncate() for asm.js entirely. asm.js gives us types for every expression ahead of time, so in theory there shouldn't be any interesting truncation happening anyway.
Attachment #8420709 - Flags: review?(sunfish) → review-
(In reply to Dan Gohman [:sunfish] from comment #8)
> In fact, it may make sense to just skip RangeAnalysis::truncate() for asm.js
> entirely. asm.js gives us types for every expression ahead of time, so in
> theory there shouldn't be any interesting truncation happening anyway.

Luke tried to disable Range Analysis before, but he didn't, I don't know the reasons.

I think you might be right, in which case the range analysis would only be useful for removing bounds check.
This patch disables the automatic truncation done by Range Analysis.
I still need to check AWFY performance to see if this has any effect.
Attachment #8420722 - Flags: review?(sunfish)
Comment on attachment 8420722 [details] [diff] [review]
Disable automatic truncation for Asm.JS

Review of attachment 8420722 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. If there are any performance regressions, we may need to file bugs against Emscripten.
Attachment #8420722 - Flags: review?(sunfish) → review+
Null-deref, not s-s.
Group: core-security
Keywords: sec-critical
Re-locking s-s for the moment even though this is a null deref, I have a bunch of stacks that came from testcases that showed varying symptoms, so let's set this csec-dos and hidden for now.
Group: core-security
Keywords: csectype-dos
Posted file yet another stack
Judging by the different testcases in comment 0, comment 1 and as well as this stack from fuzzing instances which reduce to the same issue, let's keep this hidden for now.
I ran the emscripten benchmark suite on this patch, and saw no regressions.
Keywords: sec-other
https://hg.mozilla.org/mozilla-central/rev/32a8e28507e6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx32
Group: core-security
You need to log in before you can comment on or make changes to this bug.