From a8397993bbef8f7825c5d3202822b3eede96a582 Mon Sep 17 00:00:00 2001 From: Philipp Dunkel Date: Tue, 27 Oct 2026 18:08:36 +0100 Subject: [PATCH] vfs: keep each loaded memfd image open until exit On Linux an addon or FFI library inside a VFS is loaded from an anonymous memfd through /proc/self/fd/N, and the memfd was closed right after a successful load. The next image then got the same fd number, hence the same path, and the dynamic loader, which knows a loaded object by the path it was opened with, returned the object already loaded under that path instead of loading the new file. So the second of any two addons or libraries loaded from a VFS resolved to the first: an addon after an FFI library failed with "Module did not self-register", an FFI library after an addon had none of its own symbols, and two addons silently became the same one. Keep a memfd open for the life of the process once it has been loaded, so no two images ever share a path; MFD_CLOEXEC keeps it out of child processes. A failed load still closes its memfd. The temp-file fallback is unaffected: each image there gets a fresh mkdtemp() directory, and Windows already keeps its images until exit. Signed-off-by: Philipp Dunkel --- src/node_binding.cc | 18 ++++-- src/node_binding.h | 9 ++- test/ffi/test-ffi-vfs.js | 54 +++++++++++++++++ .../test-dlopen-binary-image-cleanup.js | 58 ++++++++++++++----- test/parallel/test-vfs-addon-sequence.js | 33 +++++++++++ 5 files changed, 151 insertions(+), 21 deletions(-) create mode 100644 test/parallel/test-vfs-addon-sequence.js diff --git a/src/node_binding.cc b/src/node_binding.cc index 3c7dee9bb33d..b39705b0905a 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -709,17 +709,25 @@ bool AddonImage::MaterializeTempFile(const char* data, size_t len) { void AddonImage::AfterOpen(bool opened, void* module) { consumed_ = true; - // The right cleanup is the same whether or not the load worked, and the - // module never has to be unloaded: the name is already gone by now. - (void)opened; + // The module never has to be unloaded here: Node keeps addons loaded for + // the life of the process, and an FFI library is closed by its owner. (void)module; - // memfd: the load's mapping (or nothing, on failure) owns it from here. if (fd_ != -1) { - close(fd_); + // The dynamic loader knows a loaded object by the path it was opened + // with, and hands back the object already loaded under a path instead of + // opening the file again. Closing a loaded memfd would free its number + // for the next image, which would then be loaded through the very same + // /proc/self/fd/N and silently resolve to this one. So a memfd that was + // loaded stays open, keeping its path to itself, for the life of the + // process; MFD_CLOEXEC keeps it out of child processes. A failed load + // left nothing under that path, so its number can go back. + if (!opened) close(fd_); fd_ = -1; return; } if (!temp_dir_.empty()) { + // A temp file can go at once: it lives in a fresh mkdtemp() directory, + // so its path is not handed out again the way an fd number is. // On success the mapping keeps the inode alive, so unlinking now leaves no // on-disk trace; on failure this just cleans up. unlink(path_.c_str()); diff --git a/src/node_binding.h b/src/node_binding.h index ee30117d2068..4287c42cc8cf 100644 --- a/src/node_binding.h +++ b/src/node_binding.h @@ -177,7 +177,9 @@ void DLOpenBinary(const v8::FunctionCallbackInfo& args); // dlopen()/LoadLibrary() can load, with the smallest, most private on-disk // footprint each platform allows: // Linux: an anonymous in-memory memfd, loaded via /proc/self/fd/N - -// the bytes never touch the filesystem. +// the bytes never touch the filesystem. A loaded memfd is kept +// open until exit, so no later image reuses its path (the +// loader would return the earlier object by that name). // other POSIX: a 0700 mkdtemp() directory plus an O_EXCL|O_NOFOLLOW file, // unlink()ed right after the load (the mapping keeps it alive). // Windows: a temp file, written and closed before the load because the @@ -207,8 +209,9 @@ class AddonImage { // Call exactly once, right after the load; `opened` says whether the load // succeeded and `module` is the module handle it produced. Releases what is - // no longer needed: on POSIX closes the memfd or unlinks the temp file, which - // a successful load keeps alive through its own mapping. Windows cannot + // no longer needed: on POSIX unlinks the temp file, which a successful load + // keeps alive through its own mapping, and closes the memfd only when the + // load failed, keeping a loaded one's path unique until exit. Windows cannot // unlink a mapped image, so there the file is removed at once only when the // load failed; otherwise it is kept, with `module`, until process exit, where // the module is unloaded and the file finally deleted. diff --git a/test/ffi/test-ffi-vfs.js b/test/ffi/test-ffi-vfs.js index 37926e9c3dda..fa4857fbe912 100644 --- a/test/ffi/test-ffi-vfs.js +++ b/test/ffi/test-ffi-vfs.js @@ -92,3 +92,57 @@ test('a library loaded from a VFS outlives the mount', () => { lib.close(); } }); + +const addonPath = (name) => path.join( + __dirname, '..', 'addons', name, 'build', common.buildType, 'binding.node'); + +test('an addon loaded after a VFS library is its own module', (t) => { + const helloPath = addonPath('hello-world'); + if (!fs.existsSync(helloPath)) { + t.skip('the hello-world addon is not built'); + return; + } + const seqVfs = vfs.create(); + seqVfs.writeFileSync(`/${libraryName}`, fs.readFileSync(libraryPath)); + seqVfs.writeFileSync('/hello.node', fs.readFileSync(helloPath)); + const seqMount = seqVfs.mount(); + const { lib, functions } = ffi.dlopen(path.join(seqMount, libraryName), { + add_i32: fixtureSymbols.add_i32, + }); + + try { + // Resolved to the library, the addon would fail with "Module did not + // self-register". + const addon = require(path.join(seqMount, 'hello.node')); + assert.strictEqual(addon.hello(), 'world'); + assert.strictEqual(functions.add_i32(20, 22), 42); + } finally { + lib.close(); + seqVfs.unmount(); + } +}); + +test('a VFS library loaded after an addon is its own library', (t) => { + const functionPath = addonPath('hello-world-function-export'); + if (!fs.existsSync(functionPath)) { + t.skip('the hello-world-function-export addon is not built'); + return; + } + const seqVfs = vfs.create(); + seqVfs.writeFileSync('/function.node', fs.readFileSync(functionPath)); + seqVfs.writeFileSync(`/${libraryName}`, fs.readFileSync(libraryPath)); + const seqMount = seqVfs.mount(); + const addon = require(path.join(seqMount, 'function.node')); + assert.strictEqual(addon(), 'world'); + + // Resolved to the addon, the library would have no add_i32 to look up. + const { lib, functions } = ffi.dlopen(path.join(seqMount, libraryName), { + add_i32: fixtureSymbols.add_i32, + }); + try { + assert.strictEqual(functions.add_i32(40, 2), 42); + } finally { + lib.close(); + seqVfs.unmount(); + } +}); diff --git a/test/parallel/test-dlopen-binary-image-cleanup.js b/test/parallel/test-dlopen-binary-image-cleanup.js index dabaeabaa0cc..c843cd1dec13 100644 --- a/test/parallel/test-dlopen-binary-image-cleanup.js +++ b/test/parallel/test-dlopen-binary-image-cleanup.js @@ -7,9 +7,10 @@ // this checks both halves of the contract: // // Linux: an anonymous memfd loaded through /proc/self/fd - nothing ever -// reaches the filesystem, and AfterOpen() closes the descriptor -// once the load owns its mapping, so repeated loads must not -// accumulate open descriptors. +// reaches the filesystem. A loaded memfd stays open until exit, +// close-on-exec, so that no later image is loaded through the +// same /proc/self/fd/N path (the loader would return this one); +// a failed load closes its memfd again. // other POSIX: a mkdtemp() directory unlinked and rmdir()ed right after the // load, so nothing is left even while the process runs. // Windows: the loader maps the file by path for the DLL's lifetime, so @@ -45,27 +46,58 @@ const child = ` // the load can only come from the bytes and their materialized image. const virtualPath = ${JSON.stringify(path.join(addonPath, '..', 'nowhere', 'binding.node'))}; - // On Linux the image is a descriptor rather than a file, so count them: each - // load must hand its fd to the mapping and close it, leaving no growth. + // On Linux the image is a memfd rather than a file, so count those: each + // successful load keeps exactly one, close-on-exec, and a failed load none. const fdDir = '/proc/self/fd'; - const countFds = () => { - try { return fs.readdirSync(fdDir).length; } catch { return -1; } + const imageFds = () => { + let fds; + try { fds = fs.readdirSync(fdDir); } catch { return null; } + return fds.filter((fd) => { + try { + return fs.readlinkSync(\`\${fdDir}/\${fd}\`).startsWith('/memfd:node-addon'); + } catch { return false; } + }); }; - const before = countFds(); + const before = imageFds(); // Load repeatedly: each load materializes its own image, so a leak of an - // image, a descriptor or a retained handle shows up as growth. - for (let i = 0; i < 5; i++) { + // image or a retained handle shows up as growth. + const loads = 5; + for (let i = 0; i < loads; i++) { const m = { exports: {} }; // flags undefined: keep the default dlopen(2) mode - 0 is not valid // everywhere (glibc rejects it with EINVAL). dlopenBinary(m, virtualPath, undefined, bytes); if (m.exports.hello() !== 'world') throw new Error('addon did not load'); } + const loaded = imageFds(); - const after = countFds(); - if (before !== -1 && after > before) { - throw new Error(\`descriptor leak: \${before} -> \${after} after 5 loads\`); + for (let i = 0; i < loads; i++) { + try { + dlopenBinary({ exports: {} }, virtualPath, undefined, Buffer.from('nope')); + throw new Error('loading garbage bytes did not fail'); + } catch (err) { + if (err.code !== 'ERR_DLOPEN_FAILED') throw err; + } + } + const failed = imageFds(); + + if (before !== null) { + const kept = loaded.filter((fd) => !before.includes(fd)); + // Zero when memfd is unavailable and the temp-file fallback was used. + if (kept.length !== 0 && kept.length !== loads) { + throw new Error(\`expected \${loads} retained images, got \${kept.length}\`); + } + if (failed.length !== loaded.length) { + throw new Error(\`failed loads kept images: \${loaded} -> \${failed}\`); + } + for (const fd of kept) { + const info = fs.readFileSync(\`/proc/self/fdinfo/\${fd}\`, 'utf8'); + const flags = parseInt(/^flags:\\s*([0-7]+)/m.exec(info)[1], 8); + if ((flags & 0o2000000) === 0) { // O_CLOEXEC + throw new Error(\`retained image fd \${fd} is not close-on-exec\`); + } + } } process.exit(0); `; diff --git a/test/parallel/test-vfs-addon-sequence.js b/test/parallel/test-vfs-addon-sequence.js new file mode 100644 index 000000000000..436e2b2d3862 --- /dev/null +++ b/test/parallel/test-vfs-addon-sequence.js @@ -0,0 +1,33 @@ +// Flags: --disable-warning=ExperimentalWarning +'use strict'; +const common = require('../common'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); +const vfs = require('node:vfs'); + +const addonPath = (name) => path.join( + __dirname, '..', 'addons', name, 'build', common.buildType, 'binding.node'); +const helloPath = addonPath('hello-world'); +const functionPath = addonPath('hello-world-function-export'); +if (!fs.existsSync(helloPath) || !fs.existsSync(functionPath)) { + common.skip('the hello-world addons are not built'); +} + +const myVfs = vfs.create(); +myVfs.writeFileSync('/hello.node', fs.readFileSync(helloPath)); +myVfs.writeFileSync('/function.node', fs.readFileSync(functionPath)); +const mountPoint = myVfs.mount(); + +const hello = require(path.join(mountPoint, 'hello.node')); +assert.strictEqual(typeof hello, 'object'); +assert.strictEqual(hello.hello(), 'world'); + +// hello-world-function-export replaces its exports with a function. Resolved +// to the first image, this would be another { hello } object instead. +const fn = require(path.join(mountPoint, 'function.node')); +assert.strictEqual(typeof fn, 'function'); +assert.strictEqual(fn(), 'world'); +assert.notStrictEqual(fn, hello); + +myVfs.unmount();