1073861 - Crash while reading CSP blog post

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am using Nightly 35.0a1 (2014-09-27).

STR:

1. Go to http://swannodette.github.io/2013/07/12/communicating-sequential-processes/
2. Scroll down so that the "(defn map [f in]" example is about mid-screen
3. Browser crashes within a minute

I quickly racked up numerous crashes from this page, all somewhere in the JS engine:

https://crash-stats.mozilla.com/report/index/0c2f8dc0-7713-4e6a-b503-cec672140927
https://crash-stats.mozilla.com/report/index/79b0900d-2500-4d13-ac80-facca2140927
https://crash-stats.mozilla.com/report/index/81362d30-5c4f-4429-ae58-4fcac2140927
https://crash-stats.mozilla.com/report/index/b69804b2-e290-452c-8815-1d52e2140927
https://crash-stats.mozilla.com/report/index/9428b1d2-6840-4636-8fb1-a863b2140927

Comment 1

[Jan de Mooij [:jandem]]

I get this assertion failure:

Assertion failure: opd->type() == phi->type(), at js/src/jit/Lowering.cpp:3987

In LIRGenerator::visitBlock

(lldb) p opd->type()
(js::jit::MIRType) $0 = MIRType_Value
(lldb) p phi->type()
(js::jit::MIRType) $1 = MIRType_Object
(lldb) p opd->op()
(js::jit::MDefinition::Opcode) $2 = Op_FilterTypeSet

NI from Hannes in case it's FilterTypeSet. Or could it be from bug 1072911?

Comment 2

[Hannes Verschore [:h4writer]]

Shell testcase:

function a(a, b, c, g) {
    for (;;) {
        if (0 > c) return a;
        a: {
            for (;;) {
                var k = a.forward[c];
                if (t(k))
                    if (k.key < b) a = k;
                    else break a;
                else break a
            }
            a = void 0
        }
        null !=
            g && (g[c] = a);
        c -= 1
    }
}

function t(a) {
    return null != a && !1 !== a
}


var d = {forward: [{},null,{}]}
for (var i=0; i < 1000; i++) {
    a(d, 0, 1, null);
    a(d, 0, 0, null);
}

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Fix

The issue is that we can't do "setResultType" in TypePolicy, since the PhiTypes are already fixed. The setResultType was more of a easy way out, so not all transitions had to get added.

There are two solutions to this
1) Add a pass in the beginning of TypeAnalysis that goes RPO that only allows adjusting outputtype. This would be handy for TypeBarrier and FilterTypeSet. Don't think others will gain that much out of it. Except it might be possible to integrate the Float32 conversion in that pass.
2) Don't use setResultType and cover all transitions from input to output type.

I decided to do (2).

(Though one thing that bothers me is that we also have setResultType in TypeBarrierPolicy and this has never tripped, like FilterTypeSet does, while it is used more often ...)

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8497381 [details] [diff] [review]
Fix

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

::: js/src/jit/TypePolicy.cpp
@@ +889,5 @@
> +    }
> +
> +    ins->block()->insertBefore(ins, replace);
> +    ins->replaceOperand(0, replace);
> +    if (!replace->typePolicy()->adjustInputs(alloc, replace))

if (replace->typePolicy() && !replace->typePolicy()->adjustInputs(alloc, replace))

This wasn't caught by jit-tests. Gonna try to write a testcase for it and add it when landing.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Fix typebarrier/FilterTypeSet

This removes the buggy "setResultType" on both policies.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8497439 [details] [diff] [review]
Fix typebarrier/FilterTypeSet

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

Thanks for jumping on this. r=me with comments addressed.

Can you add the testcase too?

::: js/src/jit/IonBuilder.cpp
@@ +3163,5 @@
>              ins->toFilterTypeSet()->setResultTypeSet(intersect);
> +
> +            if (ins->type() == MIRType_Undefined)
> +                current->setSlot(i, constant(UndefinedValue()));
> +            if (replace->type() == MIRType_Null)

s/replace/ins/ ?

::: js/src/jit/TypePolicy.cpp
@@ +862,5 @@
>      }
>  
> +    // Just a sanity check. The type should normally always be in accordance.
> +    // This should be dead code. But leaving it in for the sake of not having
> +    // security bugs when this triggers.

I think we should turn this into a MOZ_CRASH (crashes also in opt builds). It's quite a lot of code, and I think it'd be nice if the fuzzers etc could detect problems.

Comment 7

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4eaa493bf0c

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f4eaa493bf0c