GeezerVison is showing github.com — module: rework of memory management in `vm` APIs with the `importModuleDynamically` option by joyeecheung · Pull Request #48510 · nodejs/node
Conversation
joyeecheung
commented
Jun 21, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
adanski, rezonant, sirDonovan, viceice, bricss, shrujalshah28, kirillgroshkov, tonivj5, JP250552, hartherbert, and 24 more reacted with thumbs up emoji
viceice, bricss, sapphi-red, kirillgroshkov, tonivj5, jakebailey, MindFreeze, ln-e, jimmy-guzman, oskarols, and 9 more reacted with heart emoji
wyardley, Xunnamius, and dnalborczyk reacted with rocket emoji
shrujalshah28, bricss, kirillgroshkov, tonivj5, ln-e, oskarols, EmilyRosina, andriyor, brunocabral88, blimmer, and 2 more reacted with eyes emoji
All reactions
-
👍
34 reactions
-
❤️
19 reactions
-
🚀
3 reactions
-
👀
12 reactions
|
Review requested:
- @nodejs/loaders
- @nodejs/modules
- @nodejs/vm
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
joyeecheung
changed the title
WIP: fix vm.compileFunction() dynamic import callback leak
WIP: fix leak in vm.compileFunction() dynamic import callback and the ModuleWrap
Jun 21, 2023
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jun 21, 2023
joyeecheung
commented
Jun 21, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
joyeecheung
commented
Jun 22, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
Looks like this fixes the segfault in #43205 and #38695 as well
|
adanski, kirillgroshkov, JP250552, lin72h, and Qard reacted with hooray emoji
tonivj5, targos, kirillgroshkov, lin72h, and Qard reacted with rocket emoji
All reactions
-
🎉
5 reactions
-
🚀
5 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
joyeecheung
changed the title
WIP: fix leak in vm.compileFunction() dynamic import callback and the ModuleWrap
WIP: use symbol in weakmap to maintain lifetime of modules' host defined options
Jun 22, 2023
JakobJingleheimer reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
hildjj
commented
Jun 22, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
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?
|
kirillgroshkov and lin72h reacted with thumbs up emoji
adanski and vidal7 reacted with eyes emoji
All reactions
-
👍
2 reactions
-
👀
2 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
joyeecheung
commented
Jun 22, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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).
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jun 23, 2023
This was referenced Oct 5, 2023
This was referenced Oct 23, 2023
This was referenced Nov 30, 2023
|
@joyeecheung Is there a way to detect this fix from unprivileged code that is better than checking a bunch of version numbers?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 4, 2026
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
commit-queue-rebase
PRs the Commit Queue should land as multiple self-contained commits.
lib / src
Issues and PRs involving general changes in the lib/ or src/ directories.
needs-ci
PRs that need a full CI run.
notable-change
PRs with changes that should be highlighted in changelogs.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|