SEGFAULT in REPL #38695

aduh95 · 2021-05-15T10:54:41Z

REPL crashes when doing a dynamic import when the working directory contains a folder with the name src:

$ mkdir repro
$ cd repro
$ mkdir src
$ node
Welcome to Node.js v16.1.0.
Type ".help" for more information.
> const fn = () => import('crypto')
undefined
> fn('')
Promise { <pending> }
> undefined
undefined
> fn('')
[1]    59895 segmentation fault  node
$ node_master
Welcome to Node.js v17.0.0-pre.
Type ".help" for more information.
> const fn = () => import('crypto')
undefined
> fn('')
[1]    59992 segmentation fault  ../../node/out/Release/node
$ node_master
Welcome to Node.js v17.0.0-pre.
Type ".help" for more information.
> const fn = () => import('crypto')
undefined
> fn('')
Promise { <pending> }
> fn('')
Promise { <pending> }
> fn('')
Promise { <pending> }
> fn('')
Promise { <pending> }
> fn('')
[1]    59997 segmentation fault  ../../node/out/Release/node

Here's the info given by lldb when the process crashes:

Process 59603 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x18)
    frame #0: 0x000000010008a952 node`node::loader::ImportModuleDynamically(v8::Local<v8::Context>, v8::Local<v8::ScriptOrModule>, v8::Local<v8::String>, v8::Local<v8::FixedArray>) + 489
node`node::loader::ImportModuleDynamically:
->  0x10008a952 <+489>: movq   0x18(%rax), %rcx
    0x10008a956 <+493>: movq   0x8(%rcx), %rax
    0x10008a95a <+497>: testq  %rax, %rax
    0x10008a95d <+500>: movq   -0x58(%rbp), %rbx
Target 0: (node) stopped.

The crash also happen with a module other than crypto.

Originally posted by @rayfoss in #38090 (comment)

The text was updated successfully, but these errors were encountered:

seangoedecke · 2021-05-17T07:05:57Z

After a bit of digging I think it's the same problem as here: #25424

You can reproduce straightforwardly by creating a function like const fn = () => import('crypto'), runnning global.gc(), then trying to run fn(). The ContexifyScript wrapping import('crypto') gets GCd, the destructor removes that entry from id_to_script_map, then when fn() tries to read the map it segfaults.

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in #46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: #48510 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Previously we maintain a strong persistent reference to the ModuleWrap to retrieve the ID-to-ModuleWrap mapping from the HostImportModuleDynamicallyCallback using the number ID stored in the host-defined options. As a result the ModuleWrap would be kept alive until the Environment is shut down, which would be a leak for user code. With the new symbol-based host-defined option we can just get the ModuleWrap from the JS-land WeakMap so there's now no need to maintain this strong reference. This would at least fix the leak for vm.SyntheticModule. vm.SourceTextModule is still leaking due to the strong persistent reference to the v8::Module. PR-URL: #48510 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: #48510 Refs: #44211 Refs: #42080 Refs: #47096 Refs: #43205 Refs: #38695 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

joyeecheung · 2023-09-14T15:48:08Z

Closing as #48510 has landed and should fix this. We can re-open if that turns out to be incorrect (hopefully not).

Previously when managing the importModuleDynamically callback of vm.compileFunction(), we use an ID number as the host defined option and maintain a per-Environment ID -> CompiledFnEntry map to retain the top-level referrer function returned by vm.compileFunction() in order to pass it back to the callback, but it would leak because with how we used v8::Persistent to maintain this reference, V8 would not be able to understand the cycle and would just think that the CompiledFnEntry was supposed to live forever. We made an attempt to make that reference known to V8 by making the CompiledFnEntry weak and using a private symbol to make CompiledFnEntry strongly references the top-level referrer function in nodejs#46785, but that turned out to be unsound, because the there's no guarantee that the top-level function must be alive while import() can still be initiated from that function, since V8 could discard the top-level function and only keep inner functions alive, so relying on the top-level function to keep the CompiledFnEntry alive could result in use-after-free which caused a revert of that fix. With this patch we use a symbol in the host defined options instead of a number, because with the stage-3 symbol-as-weakmap-keys proposal we could directly use that symbol to keep the referrer alive using a WeakMap. As a bonus this also keeps the other kinds of referrers alive as long as import() can still be initiated from that Script/Module, so this also fixes the long-standing crash caused by vm.Script being GC'ed too early when its importModuleDynamically callback still needs it. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Previously we maintain a strong persistent reference to the ModuleWrap to retrieve the ID-to-ModuleWrap mapping from the HostImportModuleDynamicallyCallback using the number ID stored in the host-defined options. As a result the ModuleWrap would be kept alive until the Environment is shut down, which would be a leak for user code. With the new symbol-based host-defined option we can just get the ModuleWrap from the JS-land WeakMap so there's now no need to maintain this strong reference. This would at least fix the leak for vm.SyntheticModule. vm.SourceTextModule is still leaking due to the strong persistent reference to the v8::Module. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>

Replace the persistent handles to v8::Module and v8::UnboundScript with an internal reference that V8's GC is aware of to fix the leaks. PR-URL: nodejs#48510 Refs: nodejs#44211 Refs: nodejs#42080 Refs: nodejs#47096 Refs: nodejs#43205 Refs: nodejs#38695 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>