1287410 - Crash [@ js::NativeObject::lookup] with ES6 Modules

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 711963e8daa3 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

let moduleRepo = {};
setModuleResolveHook(function(module, specifier) {
    if (specifier in moduleRepo)
        return moduleRepo[specifier];
    throw "Module '" + specifier + "' not found";
});
let a = moduleRepo['a'] = parseModule("export var a = 1; export var b = 2;");
let b = moduleRepo['b'] = parseModule("export var b = 3; export var c = 4;");
let c = moduleRepo['c'] = parseModule("export * from 'a'; export * from 'b';");
c.declarationInstantiation();
eval(`
let a = moduleRepo['a'] = parseModule("export var a = 1; export var b = 2;");
let d = moduleRepo['d'] = parseModule("import { a } from 'c'; a;");
d.declarationInstantiation();
`);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000abcd8b in js::NativeObject::lookup (this=0x0, cx=cx@entry=0x7ffff6965000, id=...) at js/src/vm/NativeObject.cpp:233
#0  0x0000000000abcd8b in js::NativeObject::lookup (this=0x0, cx=cx@entry=0x7ffff6965000, id=...) at js/src/vm/NativeObject.cpp:233
#1  0x0000000000c320f7 in js::IndirectBindingMap::putNew (this=0x7ffff69ae440, cx=cx@entry=0x7ffff6965000, name=..., name@entry=..., environment=..., environment@entry=..., localName=..., localName@entry=...) at js/src/builtin/ModuleObject.cpp:239
#2  0x0000000000b02057 in js::ModuleEnvironmentObject::createImportBinding (this=0x7ffff7e78400, cx=cx@entry=0x7ffff6965000, importName=..., importName@entry=..., module=..., module@entry=..., localName=..., localName@entry=...) at js/src/vm/ScopeObject.cpp:532
#3  0x0000000000b0d7e4 in intrinsic_CreateImportBinding (cx=cx@entry=0x7ffff6965000, argc=<optimized out>, vp=0x7fffffffb478) at js/src/vm/SelfHosting.cpp:2148
#4  0x0000000000aa7424 in js::CallJSNative (cx=cx@entry=0x7ffff6965000, native=0xb0d660 <intrinsic_CreateImportBinding(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:232
[...]
#47 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff69ae440	140737330734144
rcx	0x7fffffffaef0	140737488334576
rdx	0x7ffff7e00940	140737352042816
rsi	0x7ffff6965000	140737330434048
rdi	0x0	0
rbp	0x7fffffffadc0	140737488334272
rsp	0x7fffffffada0	140737488334240
r8	0x7fffffffaed0	140737488334544
r9	0x7fffffffafb0	140737488334768
r10	0x7ffff6956000	140737330372608
r11	0xfff9000000000000	-1970324836974592
r12	0x7ffff6965000	140737330434048
r13	0x7fffffffaec0	140737488334528
r14	0x7fffffffaef0	140737488334576
r15	0x7fffffffaeb0	140737488334512
rip	0xabcd8b <js::NativeObject::lookup(js::ExclusiveContext*, jsid)+11>
=> 0xabcd8b <js::NativeObject::lookup(js::ExclusiveContext*, jsid)+11>:	mov    (%rdi),%rax
   0xabcd8e <js::NativeObject::lookup(js::ExclusiveContext*, jsid)+14>:	mov    (%rax),%rax

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150923073515" and the hash "f4233421a0091c7ff9da20e917e026bf60f93c8f".
The "bad" changeset has the timestamp "20150923075616" and the hash "db4c17553be905e5d4e3106718f61f7421b91994".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f4233421a0091c7ff9da20e917e026bf60f93c8f&tochange=db4c17553be905e5d4e3106718f61f7421b91994

Comment 2

[Jon Coppeard (:jonco)]

Another way in which a buggy module loader can mess things up.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1287410-add-module-state

The problem here is that HostResolveImportedModule (which is supplied by the embedding) is always supposed to return the same module for a given pair of arguments.  Failure to do this is a bug in the module loader, but this is something we should be handle.

This patch replaces the module's evaluated property with a state property which covers not just whether the module has been evaluated but the other possible states it can be in too.  It also replaces the setting of the environment value to null if instantiation failed.  The next patch will add more checks using this state.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1287410-check-module-state

Update state property appropriately and use it to check that the module returned by HostResolveImportedModule is in the correct state.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8772483 [details] [diff] [review]
bug1287410-add-module-state

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

Let me see if I understand. Are finer grain states needed for instantiation due to the error checking in the next patch? That is, is it because the correctness condition differ among steps in ModuleDeclarationInstantiation? The loader sure is complicated.

::: js/src/builtin/SelfHostingDefines.h
@@ +103,5 @@
>  
> +#define MODULE_STATE_FAILED       0
> +#define MODULE_STATE_PARSED       1
> +#define MODULE_STATE_INSTANTIATED 2
> +#define MODULE_STATE_EVALUATED    3

Since these are checked with <, please add a static assert somewhere that they're ordered as expected.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Shu-yu Guo [:shu] from comment #5)
Yes, adding fine grained states makes it possible to check that the loader is behaving correctly and obeying its invariants.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c197d68a2cf1
Add more fine-grained module state r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/1537eb623f79
Check state of module returned by HostResolveImportedModule r=shu

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c197d68a2cf1
https://hg.mozilla.org/mozilla-central/rev/1537eb623f79

Comment 9

[Andrew Overholt [:overholt]]

Can/should we uplift this to 50 (seems it's also affected)?

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Andrew Overholt [:overholt] from comment #9)
> Can/should we uplift this to 50 (seems it's also affected)?

Modules aren't being used for much at the moment, so probably no point uplifting this.

Comment 11

[Andrew Overholt [:overholt]]

(In reply to Jon Coppeard (:jonco) from comment #10)
> (In reply to Andrew Overholt [:overholt] from comment #9)
> > Can/should we uplift this to 50 (seems it's also affected)?
> 
> Modules aren't being used for much at the moment, so probably no point
> uplifting this.

OK, thanks.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'js::NativeObject::lookup':
 - nightly (version 51): 0 crashes from 2016-08-01.
 - aurora  (version 50): 0 crashes from 2016-08-01.
 - beta    (version 49): 50 crashes from 2016-08-02.
 - release (version 48): 52 crashes from 2016-07-25.
 - esr     (version 45): 8 crashes from 2016-05-02.

Crash volume on the last weeks (Week N is from 08-22 to 08-28):
            W. N-1  W. N-2  W. N-3
 - nightly       0       0       0
 - aurora        0       0       0
 - beta          9      13       9
 - release      15      18       7
 - esr           1       2       0

Affected platform: Windows

Crash rank on the last 7 days:
           Browser   Content     Plugin
 - nightly
 - aurora
 - beta    #1034     #484
 - release #1148
 - esr     #1706