diff --git a/src/node_binding.cc b/src/node_binding.cc index 3c7dee9bb33..b39705b0905 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 ee30117d206..4287c42cc8c 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 37926e9c3dd..fa4857fbe91 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 dabaeabaa0c..c843cd1dec1 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 00000000000..436e2b2d386 --- /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();