-
-
Notifications
You must be signed in to change notification settings - Fork 36.6k
vfs: load native addons from a mounted file system #65680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6543f83
4d2d7d9
ccf16a9
81af7e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -914,6 +914,26 @@ function installModuleLoaderOverrides() { | |
| }); | ||
| } | ||
|
|
||
| let originalDlopen; | ||
|
|
||
| function installAddonLoader() { | ||
| originalDlopen = process.dlopen; | ||
| process.dlopen = function(module, filename, flags) { | ||
|
Comment on lines
+920
to
+921
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just reflecting... we need a better way to do this stuff. This kind of monkeypatching makes me sad.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, but this hits only code loading when on VFS. Making it generic would cause it to be run on every module load, which I was just very hesitant to do. This way it's only a thing if VFS is in use. Maybe the right answer is to make a note that this needs cleaning up (I volunteer) once VFS is out of experimental. Until then I think this is the right shape. |
||
| // dlopen(2) cannot open a native addon that lives in a VFS by path (it has | ||
| // no real inode). Read its bytes and hand them to the internal | ||
| // dlopenBinary(), which writes them to a private, self-cleaning temporary | ||
| // image - an in-memory memfd on Linux - and loads that. Only VFS paths take | ||
| // this route; everything else loads straight from disk through the | ||
| // unchanged process.dlopen(). | ||
| if (StringPrototypeStartsWith(filename, normalizedVfsRootPrefix)) { | ||
| const { readFileSync } = require('fs'); | ||
| const { dlopenBinary } = internalBinding('process_methods'); | ||
| return dlopenBinary(module, filename, flags, readFileSync(filename)); | ||
| } | ||
| return originalDlopen(module, filename, flags); | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Install all VFS hooks: module loader overrides and fs handlers. | ||
| */ | ||
|
|
@@ -922,6 +942,7 @@ function installHooks() { | |
| debug('install hooks'); | ||
| normalizedVfsRootPrefix = getNormalizedVfsRoot() + sep; | ||
| installModuleLoaderOverrides(); | ||
| installAddonLoader(); | ||
| vfsHandlerObj = createVfsHandlers(); | ||
| setVfsHandlers(vfsHandlerObj); | ||
| hooksInstalled = true; | ||
|
|
@@ -939,6 +960,7 @@ function uninstallHooks() { | |
| setLoaderOverrides(); | ||
| setVfsHandlers(null); | ||
| vfsHandlerObj = undefined; | ||
| process.dlopen = originalDlopen; | ||
| hooksInstalled = false; | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably better sent in a different PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure why this is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was being checked in vfs but wasn’t actually accepted on command-line. So when you enforce permissions testing against vfs fails because that permission can’t be set. So I need that, or I can’t test the permissions that @jasnell asked for.
so it’s either this here, or in a separate PR that goes in first, or no tests for permissions.
I chose to include it as it’s just the flag (tiny) and fixes a bug that bites in the tests for this.