vfs: load native addons from a mounted file system - #65680
Conversation
nodejs-github-bot
added
c++
| originalDlopen = process.dlopen; | ||
| process.dlopen = function(module, filename, flags) { |
There was a problem hiding this comment.
Just reflecting... we need a better way to do this stuff. This kind of monkeypatching makes me sad.
There was a problem hiding this comment.
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.
| assert.ok(!fs.existsSync(fakePath)); | ||
|
|
||
| const m = { exports: {} }; | ||
| dlopenBinary(m, fakePath, flags, bytes); |
There was a problem hiding this comment.
This is inherently rather dangerous. I'd like to see a test that verifies this is protected by the permission system. I would expect that both the fs and addon permissions would need to be granted in order for this to run.
There was a problem hiding this comment.
I am personally not really a fan of permissions (but that's not an argument 😄)
That being said, I'm currently making sure that allow-fs is also checked (allow-addons already was) and adding tests to keep that verified. Push with that coming shortly.
github-actions
Bot
removed
the
request-ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65680 +/- ##
==========================================
- Coverage 90.07% 90.05% -0.03%
==========================================
Files 754 754
Lines 256378 256514 +136
Branches 48498 48521 +23
==========================================
+ Hits 230936 231002 +66
- Misses 16572 16614 +42
- Partials 8870 8898 +28
🚀 New features to boost your workflow:
|
|
Note: codecov level is only due to there being an |
|
971cc1c fs: fix recursive watch error handling landed today and touches that failing test. Nothing I did should go near fs.watch so this is at least suspicious. Anyone have any ideas? |
|
Just need to rebase on |
A native addon inside a virtual file system could not be require()d: dlopen() and LoadLibrary() open a shared object by path, and a VFS path has no inode for them to open. Read the addon's bytes from the VFS and load them from a private, self-cleaning image instead, using the smallest on-disk footprint each platform allows: on Linux an anonymous memfd loaded through /proc/self/fd, so the bytes never reach the file system; on other POSIX platforms a file in a 0700 mkdtemp() directory, unlinked right after loading, with the mapping keeping it alive; on Windows a temp file opened FILE_FLAG_DELETE_ON_CLOSE whose handle is held until exit. This is internal. process.dlopen() keeps its documented (module, filename[, flags]) signature and ignores anything further; the bytes are passed to a dlopenBinary() reachable only through the process_methods binding, which the VFS hook calls for VFS paths. Addons on the real file system are untouched and load directly. Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
Reflow the temporary-image open() call to what `CLANG_FORMAT_START=$(git merge-base HEAD main) make format-cpp` produces, so the C++ formatting check passes. Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
dlopenBinary() materializes the addon's bytes into an image in the temporary directory before handing it to the dynamic loader. Under the permission model that write went unchecked: --allow-addons alone was enough to have Node.js write an executable image out. Require write access to the temporary directory as well, checked before the bytes are materialized. The check does not depend on whether the image reaches the file system on a given platform - Linux uses an anonymous memfd - so that what a program must be granted stays the same everywhere. process.dlopen() is unaffected: it never takes bytes, so it never reaches the check. Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
Mounting a VFS under the permission model is refused with a message telling the user to pass --allow-fs-vfs, but that option was never registered, so `node --allow-fs-vfs` failed with "bad option" and a virtual file system could not be mounted under --permission at all. Register it alongside the other permission flags and document it. Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
|
Rebased on main because CI was failing with an unrelated test. |
5 participants
A native addon inside a virtual file system could not be require()d: dlopen() and LoadLibrary() open a shared object by path, and a VFS path has no inode for them to open.
Read the addon's bytes from the VFS and load them from a private, self-cleaning image instead, using the smallest on-disk footprint each platform allows: on Linux an anonymous memfd loaded through /proc/self/fd, so the bytes never reach the file system; on other POSIX platforms a file in a 0700 mkdtemp() directory, unlinked right after loading, with the mapping keeping it alive; on Windows a temp file opened FILE_FLAG_DELETE_ON_CLOSE whose handle is held until exit.
This is internal. process.dlopen() keeps its documented (module, filename[, flags]) signature and ignores anything further; the bytes are passed to a dlopenBinary() reachable only through the process_methods binding, which the VFS hook calls for VFS paths. Addons on the real file system are untouched and load directly.