1439416 - Assertion failure: lineNumber > 0, at js/src/builtin/ModuleObject.cpp:306

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 48b635e3d447 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

Object.defineProperty(this.__proto__, "line", {}, TypeError);
function parseAsModule(source) {
    return Reflect.parse(source, {
        target: "module"
    });
}
parseAsModule("import d, {} from 'a'");


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00000000005ac608 in js::RequestedModuleObject::create (cx=cx@entry=0x7ffff5f16000, moduleSpecifier=..., lineNumber=0, columnNumber=18) at js/src/builtin/ModuleObject.cpp:306
#0  0x00000000005ac608 in js::RequestedModuleObject::create (cx=cx@entry=0x7ffff5f16000, moduleSpecifier=..., lineNumber=0, columnNumber=18) at js/src/builtin/ModuleObject.cpp:306
#1  0x00000000005b456e in js::ModuleBuilder::maybeAppendRequestedModule (this=this@entry=0x7fffffffbe60, specifier=..., node=0x7ffff5f82140) at js/src/builtin/ModuleObject.cpp:1512
#2  0x00000000005b4cad in js::ModuleBuilder::processImport (this=0x7fffffffbe60, pn=pn@entry=0x7ffff5f82170) at js/src/builtin/ModuleObject.cpp:1325
#3  0x0000000000505200 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::importDeclaration (this=this@entry=0x7fffffffc490) at js/src/frontend/Parser.cpp:5280
#4  0x0000000000521ab0 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::importDeclaration (this=0x7fffffffc490) at js/src/frontend/Parser.cpp:5298
#5  js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementListItem (this=this@entry=0x7fffffffc490, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, canHaveDirectives=<optimized out>) at js/src/frontend/Parser.cpp:7736
#6  0x000000000052279d in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementList (this=this@entry=0x7fffffffc490, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:4209
#7  0x0000000000526a0d in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::moduleBody (this=this@entry=0x7fffffffc490, modulesc=modulesc@entry=0x7fffffffbd40) at js/src/frontend/Parser.cpp:2268
#8  0x00000000005e2a8c in reflect_parse (cx=0x7ffff5f16000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/ReflectParse.cpp:3503
#9  0x0000000000573351 in js::CallJSNative (cx=0x7ffff5f16000, native=0x5e1070 <reflect_parse(JSContext*, uint32_t, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:290
[...]
#23 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9317
rax	0x0	0
rbx	0x0	0
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffb3a0	140737488335776
rsp	0x7fffffffb2e0	140737488335584
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7fffffffb450	140737488335952
r13	0x7fffffffbec8	140737488338632
r14	0x7fffffffbea0	140737488338592
r15	0x7fffffffbe60	140737488338528
rip	0x5ac608 <js::RequestedModuleObject::create(JSContext*, JS::Handle<JSAtom*>, unsigned int, unsigned int)+584>
=> 0x5ac608 <js::RequestedModuleObject::create(JSContext*, JS::Handle<JSAtom*>, unsigned int, unsigned int)+584>:	movl   $0x0,0x0
   0x5ac613 <js::RequestedModuleObject::create(JSContext*, JS::Handle<JSAtom*>, unsigned int, unsigned int)+595>:	ud2


I have no clue if lineNumber is used anywhere else except for informational purposes, but I'm marking this s-s in case it is used as an index anywhere.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/e1d01918122f
user:        Jon Coppeard
date:        Wed Aug 23 17:24:37 2017 +0100
summary:     Bug 1362098 - Report source position information for module export resolution failures r=till

This iteration took 257.458 seconds to run.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jon, is bug 1362098 a likely regressor?

Comment 3

[Jon Coppeard (:jonco)]

Line number information is just for information purposes.  I didn't think zero was a valid line number, but in fact you can set the starting line to zero when using Reflect.parse.

This is not s-s.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1439416-module-line-number

Patch to relax line number assertions in module code.  It turns out you can generate zero-based line numbers by calling |Reflect.parse(source, { line: 0 })|.

Comment 5

[André Bargull [:anba]]

Comment on attachment 8952328 [details] [diff] [review]
bug1439416-module-line-number

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

r+ with the assertion in [1] updated to handle this related case:

---
function parseAsModule(source) {
    return Reflect.parse(source, {
        target: "module",
        line: 0x7FFFFFFF + 1,
    });
}
parseAsModule(`export {a} from ""`);
---

[1] https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/js/src/builtin/ModuleObject.cpp#83

Comment 6

[Jon Coppeard (:jonco)]

(In reply to André Bargull [:anba] from comment #5)
That testcase passes for me, because the accessor in [1] is not invoked during Reflect.parse.  Do you see something different?

Comment 7

[André Bargull [:anba]]

Ups, that was the wrong test case, here's the correct one. Sorry for the confusion!

---
function parseAsModule(source) {
    return Reflect.parse(source, {
        target: "module",
        line: 0x7FFFFFFF + 1,
    });
}
parseAsModule(`
    import {a} from "";
    export {a};
`);
---

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1439416-module-line-number-2

Nice catch, thanks.

Comment 9

[André Bargull [:anba]]

Comment on attachment 8952424 [details] [diff] [review]
bug1439416-module-line-number-2

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

Alternatively we could also just remove the assertion and rely on implicit int32 -> uint32 conversion for negative values, but it probably doesn't matter much here, does it? r+ either way.

::: js/src/builtin/ModuleObject.cpp
@@ +80,5 @@
>      cls::name() const                                                         \
>      {                                                                         \
>          Value value = cls##_##name##Value(this);                              \
> +        if (value.isInt32()) {                                                \
> +            MOZ_ASSERT(value.toInt32() >= 0);                                 \

Assert |MOZ_ASSERT(value.toNumber() >= 0);| directly after retrieving the value to ensure the number is positive even when it cannot be represented as an int32.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3fbff4246c3
Relax line number assertions when creating module objects r=anba

Comment 11

[Natalia Csoregi [:nataliaCs]]

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