1800629 - Assertion failure: isObject() coming from Reflect.parse

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P2)

Description

[Samuel Groß]

The following sample triggers an assertion failure in debug builds of Spidermonkey at latest HEAD:

function main() {
function v0(v1,v2) {
    return v1;
}
function v3(v4,v5,v6,v7) {
    class V8 {
        constructor(v10,...v11) {
        }
        getUTCHours(v13,v14,v15,v16) {
        }
        indexOf(v18,v19,v20) {
        }
        imul(v22,v23) {
        }
    }
    Object.defineProperty(v4, "builder", { enumerable: true, value: v3 })
    return Reflect;
}
function v25() {
    return v0;
}
Object.defineProperty(v3, "identifier", { get: v25 })
const v26 = v3(v3);
const v27 = v26.parse(v3,v3);
gc();
}
main();
// CRASH INFO
// ==========
// TERMSIG: 11
// STDERR:
// Assertion failure: isObject(), at obj-fuzzbuild/dist/include/js/Value.h:935
// #01: JS::Value::toObject() const[obj-fuzzbuild/dist/bin/js +0x188189d]
// #02: ???[obj-fuzzbuild/dist/bin/js +0x1d9a012]
// #03: ???[obj-fuzzbuild/dist/bin/js +0x1d98b0b]
// #04: ???[obj-fuzzbuild/dist/bin/js +0x1d8f4a1]
// #05: ???[obj-fuzzbuild/dist/bin/js +0x1d8696a]
// #06: ???[obj-fuzzbuild/dist/bin/js +0x1d9817f]
// #07: ???[obj-fuzzbuild/dist/bin/js +0x1d88492]
// #08: ???[obj-fuzzbuild/dist/bin/js +0x1d9a4b0]
// #09: ???[obj-fuzzbuild/dist/bin/js +0x1d98b0b]
// #10: ???[obj-fuzzbuild/dist/bin/js +0x1d856ec]
// #11: ???[obj-fuzzbuild/dist/bin/js +0x1d84ef1]
// #12: ???[obj-fuzzbuild/dist/bin/js +0x1d8467c]
// #13: ???[obj-fuzzbuild/dist/bin/js +0x1d48335]
// #14: ???[obj-fuzzbuild/dist/bin/js +0x196529a]
// #15: ???[obj-fuzzbuild/dist/bin/js +0x19646de]
// #16: ???[obj-fuzzbuild/dist/bin/js +0x19576d5]
// #17: ???[obj-fuzzbuild/dist/bin/js +0x194a575]
// #18: ???[obj-fuzzbuild/dist/bin/js +0x19684d2]
// #19: ???[obj-fuzzbuild/dist/bin/js +0x1968b81]
// #20: ???[obj-fuzzbuild/dist/bin/js +0x1b07036]
// #21: JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>)[obj-fuzzbuild/dist/bin/js +0x1b07310]
// #22: ???[obj-fuzzbuild/dist/bin/js +0x18437c4]
// #23: ???[obj-fuzzbuild/dist/bin/js +0x183cb36]
// #24: ???[/lib/x86_64-linux-gnu/libc.so.6 +0x2920a]
// #25: __libc_start_main[/lib/x86_64-linux-gnu/libc.so.6 +0x292bc]
// #26: ???[obj-fuzzbuild/dist/bin/js +0x18074a9]
// #27: ??? (???:???)

The bug seems to be related to the non-standard Reflect.parse API, and so likely cannot be triggered from web content. The bug also reproduces with the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=1798856 applied, so I assume this is a separate issue.

Comment 1

[Iain Ireland [:iain]]

Here's a reduced testcase:

function foo(...x) {}

var options = {
  builder: {
    get identifier() { return (x) => x }
  }
};

const v27 = Reflect.parse(foo, options);

It looks like the problem occurs because we specify a user-defined builder that returns unexpected values. (Expected values look like ["IdExpr", { name: name }]) User-defined builders were added in bug 569487; the documentation was on MDN and is now gone. The only place I can find where we define an identifier method on a builder is in non262/reflect-parse/alternateBuilder.js, which describes itself as "A simple proof-of-concept that the builder API can be used to generate other formats, such as JsonMLAst". I can't find any evidence that user-defined builders, with or without identifier, exist anywhere outside of our own tests.

We may not be able to remove Reflect.parse completely, but we can probably remove this unused builder stuff.

Comment 2

[Iain Ireland [:iain]]

Reflect.parse isn't exposed to users, and there are no uses of the builder feature outside of tests, so this isn't security-sensitive. Opening it up and removing the builder code.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1800629: Remove custom builders from Reflect.parse r=arai

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b9c280f186b
Remove custom builders from Reflect.parse r=arai

Comment 6

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/6b9c280f186b