Closed Bug 1073861 Opened 5 years ago Closed 5 years ago

Crash while reading CSP blog post

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jryans, Assigned: h4writer)

References

()

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

Crash Signature: [@ _ZL14firstCharKinds ] [@ js::BoxNonStrictThis(JSContext*, JS::Handle<JS::Value>) ] [@ js::types::TypeObject::markPropertyNonData(js::ExclusiveContext*, jsid) ]
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?
Flags: needinfo?(hv1989)
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);
}
Flags: needinfo?(hv1989)
Attached patch Fix (obsolete) — Splinter Review
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 ...)
Assignee: nobody → hv1989
Attachment #8497381 - Flags: review?(jdemooij)
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.
Attachment #8497381 - Flags: review?(jdemooij)
This removes the buggy "setResultType" on both policies.
Attachment #8497381 - Attachment is obsolete: true
Attachment #8497439 - Flags: review?(jdemooij)
Blocks: 1070463
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.
Attachment #8497439 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/f4eaa493bf0c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1079850
Duplicate of this bug: 1059190
Duplicate of this bug: 1070463
You need to log in before you can comment on or make changes to this bug.