1449153 - Self-hosted JavaScript assertion info: "js/src/builtin/Module.js:194: Bad module state in GetModuleNamespace"

Status: RESOLVED FIXED

Description

[Christian Holler (:decoder)]

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

let moduleRepo = {};
setModuleResolveHook(function(module, specifier) {
    return moduleRepo[specifier];
});
moduleRepo["a"] = parseModule(`
  throw undefined
`);
let c = moduleRepo["c"] = parseModule(`
  import "a";
`);
c.declarationInstantiation();
try {
    c.evaluation()
} catch (e) {}
let b = moduleRepo['b'] = parseModule(`
  import * as ns0 from 'a'
`);
b.declarationInstantiation();


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000000000c0c050 in intrinsic_AssertionFailed (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/vm/SelfHosting.cpp:418
#0  0x0000000000c0c050 in intrinsic_AssertionFailed (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/vm/SelfHosting.cpp:418
#1  0x000000000057aadd in js::CallJSNative (cx=0x7ffff5f16000, native=0xc0bf90 <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/JSContext-inl.h:290
#2  0x000000000056f1ef in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff5f16000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:467
#3  0x000000000056f5cd in InternalCall (cx=0x7ffff5f16000, args=...) at js/src/vm/Interpreter.cpp:516
#4  0x0000000000561d3a in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:522
#5  Interpret (cx=0x7ffff5f16000, state=...) at js/src/vm/Interpreter.cpp:3084
#6  0x000000000056ecf5 in js::RunScript (cx=0x7ffff5f16000, state=...) at js/src/vm/Interpreter.cpp:417
#7  0x000000000056f2b7 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff5f16000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:489
#8  0x000000000056f5cd in InternalCall (cx=0x7ffff5f16000, args=...) at js/src/vm/Interpreter.cpp:516
#9  0x0000000000561d3a in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:522
#10 Interpret (cx=0x7ffff5f16000, state=...) at js/src/vm/Interpreter.cpp:3084
#11 0x000000000056ecf5 in js::RunScript (cx=0x7ffff5f16000, state=...) at js/src/vm/Interpreter.cpp:417
#12 0x000000000056f2b7 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff5f16000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:489
#13 0x000000000056f5cd in InternalCall (cx=0x7ffff5f16000, args=...) at js/src/vm/Interpreter.cpp:516
#14 0x000000000056f750 in js::Call (cx=<optimized out>, fval=..., fval@entry=..., thisv=..., thisv@entry=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:535
#15 0x000000000094a276 in js::jit::InvokeFunction (cx=cx@entry=0x7ffff5f16000, obj=..., obj@entry=..., constructing=constructing@entry=false, ignoresReturnValue=ignoresReturnValue@entry=false, argc=1, argv=0x7fffffffb950, rval=...) at js/src/jit/VMFunctions.cpp:112
#16 0x000000000094a58a in js::jit::InvokeFromInterpreterStub (cx=0x7ffff5f16000, frame=0x7fffffffb928) at js/src/jit/VMFunctions.cpp:141
#17 0x00001c4636245302 in ?? ()
[...]
#60 0x00007fffffffbfa0 in ?? ()
#61 0x0000000000796910 in EnterJit (cx=0x7fffffffb501, state=..., code=0x0) at js/src/jit/Jit.cpp:101
rax	0x0	0
rbx	0x7fffffffa320	140737488331552
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffa360	140737488331616
rsp	0x7fffffffa300	140737488331520
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7ffff4c5b920	140737299986720
r13	0xc0bf90	12631952
r14	0x7ffff486f308	140737295872776
r15	0x1	1
rip	0xc0c050 <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)+192>
=> 0xc0c050 <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)+192>:	movl   $0x0,0x0
   0xc0c05b <intrinsic_AssertionFailed(JSContext*, unsigned int, JS::Value*)+203>:	ud2

Comment 1

[André Bargull [:anba]]

This looks like a spec bug and should be reported to https://github.com/tc39/ecma262/issues.

In https://tc39.github.io/ecma262/#sec-getmodulenamespace, the assertion in step 3
> 3. Assert: If module.[[Status]] is "evaluated", module.[[EvaluationError]] is undefined.

is incorrect. 


Test case using the dynamic import() proposal which results in the same spec assertion. 

m.js
---
import("./a.js");
import("./b.js");
---

a.js:
---
throw undefined;
---

b.js:
---
import* as ns0 from "./a.js";
---

Comment 2

[Jason Orendorff [:jorendorff]]

Filed <https://github.com/tc39/ecma262/issues/1155>.

In the meantime, how bad is this bug?

https://searchfox.org/mozilla-central/source/js/src/builtin/Module.js#191-194

It looks like the error is ignored, and we might create a namespace object for a module that has uninitialized exported bindings. To me, that looks like a spec bug, but it's safe in the security sense, right?

Comment 3

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Filed <https://github.com/tc39/ecma262/issues/1155>.
> 

Thanks!

> In the meantime, how bad is this bug?
> 
> https://searchfox.org/mozilla-central/source/js/src/builtin/Module.js#191-194
> 
> It looks like the error is ignored, and we might create a namespace object
> for a module that has uninitialized exported bindings. To me, that looks
> like a spec bug, but it's safe in the security sense, right?

Uninitialised exported bindings are already possible in other situations, so this shouldn't lead to any sec-issues.

Comment 4

[Jon Coppeard (:jonco)]

Created attachment 8962740 [details] [diff] [review]
bug1449153-bad-module-state

Here's a patch to make GetModuleNamespace in this situation.

Comment 5

[André Bargull [:anba]]

Comment on attachment 8962740 [details] [diff] [review]
bug1449153-bad-module-state

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

Given that there's not yet a decision on how <https://github.com/tc39/ecma262/issues/1155> should be fixed, the change looks good to me. (<https://github.com/tc39/ecma262/issues/1155> may be resolved by simply removing the erroneous assertion instead of throwing when an evaluation error is present.)

::: js/src/builtin/Module.js
@@ +187,5 @@
>  {
>      // Step 1
>      assert(IsModule(module), "GetModuleNamespace called with non-module");
>  
> +    if (module.status === MODULE_STATUS_EVALUATED_ERROR)

Can you add a link to <https://github.com/tc39/ecma262/issues/1155>, so readers are notified that the current spec is buggy and we intentionally deviate from it because of that?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be3b370ae62
Handle errored module in GetModuleNamespace() r=anba

Comment 7

[Raul Gurzau (:RaulGurzau)]

https://hg.mozilla.org/mozilla-central/rev/9be3b370ae62

Comment 8

[Ryan VanderMeulen [:RyanVM]]

IIUC, this doesn't need backport to Beta60, but feel free to set the status to affected and request approval if you feel strongly otherwise.