module: rework of memory management in `vm` APIs with the `importModuleDynamically` option #48510

joyeecheung · 2023-06-21T16:01:05Z

This PR (hopefully) fixes a bunch of memory leaks & use-after-free surrounding vm.Script, vm.compileFunction, vm.SyntheticModule and vm.SourceTextModule when dynamic import is used (see the issues referenced below) and makes it possible for people to upgrade from older versions of Node.js.

This cannot land as-is on v20.x as it depends on #49491 and #49419 which break the ABI on v20x. We need a special Node.js v20-only extension to the V8 API for this to work without breaking the ABI.

module: use symbol in WeakMap to manage host defined options

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.

module: fix leak of vm.SyntheticModule

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.

module: fix the leak in SourceTextModule and ContextifySript

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.

Refs: #44211
Refs: #42080
Refs: #47096
Refs: #43205
Refs: #38695

nodejs-github-bot · 2023-06-21T16:01:09Z

Review requested:

@nodejs/loaders
@nodejs/modules
@nodejs/vm

nodejs-github-bot · 2023-06-21T16:05:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52320/

joyeecheung · 2023-06-21T16:34:12Z

Still need to do a bit more investigation to figure out it's really safe this time around, so marking it as WIP (I've done some walkthrough of the vm.compileFunction() part, but haven't really looked into the ModuleWrap part). I suppose we probably also want to put it behind flags until symbol-as-weakmap-key gets to stage 4 though. Also need to think of more test cases to exercise the new memory management.

lib/internal/vm/module.js

lib/internal/modules/esm/create_dynamic_module.js

lib/internal/modules/esm/loader.js

lib/internal/modules/esm/translators.js

lib/internal/vm.js

lib/vm.js

joyeecheung · 2023-06-22T11:11:55Z

Looks like this fixes the segfault in #43205 and #38695 as well

targos · 2023-06-22T12:21:48Z

amazing!

hildjj · 2023-06-22T13:23:49Z

It looks to me like symbols-as-weakmap-keys is now Stage 4 as of 2023-01-30, but not all of the documents have been updated.

I would guess back-porting this fix to Node 16 or 18 is going to be a challenge though, right?

joyeecheung · 2023-06-22T16:52:48Z

It looks to me like symbols-as-weakmap-keys is now Stage 4 as of 2023-01-30, but not all of the documents have been updated.

Nice. Then I think we don't have to flag this for v20. But yeah v18 or v16 didn't implement that proposal (and we probably shouldn't backport it / cannot do a full upgrade because LTS) so it cannot be backported.

I've tweaked the registry object a bit, added tests, and some analysis to the commit messages about why it fixes the leaks/segfaults. Although I think for the vm.Modules this just fixes vm.SyntheticModule, but vm.SourceTextModule would still leak until we find a way to create ModuleWrap -> v8::Module references without using the problematic v8::Persistent approach (maybe using the namespace object to transitively hold a reference to the Module would work, but that feels hacky to me, so I'll leave it for now).

nodejs-github-bot · 2023-06-22T21:06:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52367/

Original commit message: [api] mark v8::Script and v8::UnboundScript as v8::Data v8::UnboundModuleScript and v8::Module are already v8::Data. Mark v8::Script and v8::UnboundScript as v8::Data so that they can be used in other V8 APIs that takes v8::Data. Refs: nodejs#48510 Bug: v8:14120 Change-Id: I2dd5648528c1b0030292872441758d4fb2cfcc1c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4827307 Reviewed-by: Camillo Bruni <cbruni@chromium.org> Commit-Queue: Joyee Cheung <joyee@igalia.com> Cr-Commit-Position: refs/heads/main@{#89727} Refs: v8/v8@b60a03d PR-URL: nodejs#49491 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>

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>

Notable changes: deps: * add v8::Object::SetInternalFieldForNodeCore() (Joyee Cheung) nodejs#49874 doc: * deprecate `fs.F_OK`, `fs.R_OK`, `fs.W_OK`, `fs.X_OK` (Livia Medeiros) nodejs#49683 * deprecate `util.toUSVString` (Yagiz Nizipli) nodejs#49725 * deprecate calling `promisify` on a function that returns a promise (Antoine du Hamel) nodejs#49647 esm: * set all hooks as release candidate (Geoffrey Booth) nodejs#49597 module: * fix the leak in SourceTextModule and ContextifySript (Joyee Cheung) nodejs#48510 * fix leak of vm.SyntheticModule (Joyee Cheung) nodejs#48510 * use symbol in WeakMap to manage host defined options (Joyee Cheung) nodejs#48510 src: * (SEMVER-MINOR) allow embedders to override NODE_MODULE_VERSION (Cheng Zhao) nodejs#49279 stream: * use bitmap in writable state (Raz Luvaton) nodejs#49834 * use bitmap in readable state (Benjamin Gruenbaum) nodejs#49745 * improve webstream readable async iterator performance (Raz Luvaton) nodejs#49662 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs#49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs#49614 PR-URL: nodejs#49932