Commit 985ec87bd16 for nodejs
commit 985ec87bd161a44eedb9c9aeffed3a9c0fe09c1f
Author: Philipp Dunkel <pipobscure@users.noreply.github.com>
Date: Wed Nov 11 00:53:40 2026 +0100
vfs: do not reuse the path of a loaded memfd image
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.
The fd is only needed until dlopen() returns, as the load's mapping
keeps the memory alive from then on; what must not be shared is the
path. Before the load, ask the loader with RTLD_NOLOAD whether the
path is already taken, and if so move the fd to a higher number with
F_DUPFD_CLOEXEC until it is free. Holding the fd from the check
through the load keeps anyone else from taking the path in between,
so this needs neither a counter nor a lock, and a library that has
really been unloaded gives its number back. The fd is closed right
after the load, whether or not it worked. Should no free number be
left, the image goes to a temp file.
The leak checks of test-vfs-addon and test-ffi-vfs compared the shared
temporary directory before and after the load, so off Linux any other
test loading from bytes at the same time could pass for a leak. Point
TMPDIR at a directory each test owns instead.
Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
PR-URL: https://github.com/nodejs/node/pull/67068
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
diff --git a/src/node_binding.cc b/src/node_binding.cc
index 3c7dee9bb33..b8a1a6f64c4 100644
--- a/src/node_binding.cc
+++ b/src/node_binding.cc
@@ -476,6 +476,27 @@ bool WriteAllToFd(int fd, const char* data, size_t len) {
int NodeMemfdCreate(const char* name, unsigned int flags) {
return static_cast<int>(syscall(SYS_memfd_create, name, flags));
}
+
+// 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. An image is loaded through /proc/self/fd/N and its fd is closed
+// right after, so the kernel can hand N to the next image while the first is
+// still loaded. So ask the loader, and move to a higher number while the path
+// is taken. Holding the fd from here through the load keeps anyone else from
+// taking the path in between, and an object that has really been unloaded gives
+// its number back, so only images loaded at the same time compete for numbers.
+int MoveFdToFreeImagePath(int fd) {
+ while (true) {
+ const std::string path = "/proc/self/fd/" + std::to_string(fd);
+ void* loaded = dlopen(path.c_str(), RTLD_LAZY | RTLD_NOLOAD);
+ if (loaded == nullptr) return fd;
+ dlclose(loaded);
+ const int moved = fcntl(fd, F_DUPFD_CLOEXEC, fd + 1);
+ close(fd);
+ if (moved == -1) return -1;
+ fd = moved;
+ }
+}
#endif // __linux__
#else // _WIN32
@@ -636,7 +657,8 @@ bool AddonImage::Materialize(const char* data, size_t len) {
// reaches the filesystem, so there is no temp file to secure, unlink, or
// leak, and no noexec-mount problem. Request an executable memfd (MFD_EXEC,
// Linux 6.3+); retry without it on older kernels that reject the flag, then
- // fall back to a temp file if memfd is unavailable entirely.
+ // fall back to a temp file if memfd is unavailable entirely, or if so many
+ // images are loaded that no fd number is left to load another through.
#ifndef MFD_CLOEXEC
#define MFD_CLOEXEC 0x0001U
#endif
@@ -654,6 +676,9 @@ bool AddonImage::Materialize(const char* data, size_t len) {
fd_ = -1;
return false;
}
+ fd_ = MoveFdToFreeImagePath(fd_);
+ }
+ if (fd_ != -1) {
path_ = "/proc/self/fd/" + std::to_string(fd_);
return true;
}
diff --git a/src/node_binding.h b/src/node_binding.h
index ee30117d206..0a8e3b4a120 100644
--- a/src/node_binding.h
+++ b/src/node_binding.h
@@ -177,7 +177,9 @@ void DLOpenBinary(const v8::FunctionCallbackInfo<v8::Value>& 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. The fd is closed right
+// after the load, and a number whose path still names a loaded
+// object is skipped (the loader would return that object).
// 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
diff --git a/test/ffi/test-ffi-vfs.js b/test/ffi/test-ffi-vfs.js
index 37926e9c3dd..832deda68ac 100644
--- a/test/ffi/test-ffi-vfs.js
+++ b/test/ffi/test-ffi-vfs.js
@@ -3,10 +3,12 @@
const common = require('../common');
common.skipIfFFIMissing();
const assert = require('node:assert');
+const { spawnSync } = require('node:child_process');
const fs = require('node:fs');
const os = require('node:os');
const path = require('node:path');
const { test } = require('node:test');
+const tmpdir = require('../common/tmpdir');
const ffi = require('node:ffi');
const vfs = require('node:vfs');
const { fixtureSymbols, libraryPath } = require('./ffi-test-common');
@@ -21,8 +23,12 @@ myVfs.writeFileSync(`/${libraryName}`, fs.readFileSync(libraryPath));
const mountPoint = myVfs.mount();
const virtualPath = path.join(mountPoint, libraryName);
+// Images land in TMPDIR, which other tests loading libraries share. Point it at
+// a directory this test owns, so their images cannot pass for a leak here.
+tmpdir.refresh();
+process.env.TMPDIR = tmpdir.path;
+
test('ffi.dlopen() loads a library from a mounted VFS', () => {
- const before = new Set(fs.readdirSync(os.tmpdir()));
const { lib, functions } = ffi.dlopen(virtualPath, {
add_i32: fixtureSymbols.add_i32,
});
@@ -40,12 +46,64 @@ test('ffi.dlopen() loads a library from a mounted VFS', () => {
// filesystem; on other POSIX it is unlinked right after loading.
// (Windows keeps a delete-on-close file until exit, so skip there.)
if (!common.isWindows) {
- const leaked = fs.readdirSync(os.tmpdir())
- .filter((f) => f.startsWith('node-addon') && !before.has(f));
+ const leaked = fs.readdirSync(tmpdir.path)
+ .filter((f) => f.startsWith('node-addon'));
assert.deepStrictEqual(leaked, [], `image not cleaned up: ${leaked}`);
}
});
+test('library images leave no file descriptor behind', { skip: !common.isLinux }, () => {
+ const images = () => fs.readdirSync('/proc/self/fd').filter((fd) => {
+ try {
+ return fs.readlinkSync(`/proc/self/fd/${fd}`).startsWith('/memfd:node-addon');
+ } catch {
+ return false;
+ }
+ });
+
+ const { lib, functions } = ffi.dlopen(virtualPath, {
+ add_i32: fixtureSymbols.add_i32,
+ });
+ try {
+ assert.deepStrictEqual(images(), []);
+ assert.strictEqual(functions.add_i32(40, 2), 42);
+ } finally {
+ lib.close();
+ }
+ assert.deepStrictEqual(images(), []);
+});
+
+test('closed library images give their fd numbers back', { skip: !common.isLinux }, () => {
+ // More loads than the limit allows fds, one at a time. There is no usable
+ // temporary directory, so this only works if each stays an in-memory image.
+ const limit = 128;
+ const child = `
+ const assert = require('node:assert');
+ const fs = require('node:fs');
+ const path = require('node:path');
+ const ffi = require('node:ffi');
+ const vfs = require('node:vfs');
+ const myVfs = vfs.create();
+ myVfs.writeFileSync('/lib.so', fs.readFileSync(${JSON.stringify(libraryPath)}));
+ const libPath = path.join(myVfs.mount(), 'lib.so');
+ for (let i = 0; i < ${limit * 4}; i++) {
+ const { lib, functions } = ffi.dlopen(libPath, {
+ add_i32: ${JSON.stringify(fixtureSymbols.add_i32)},
+ });
+ assert.strictEqual(functions.add_i32(40, 2), 42);
+ lib.close();
+ }
+ `;
+ const { status, stderr } = spawnSync('/bin/sh', [
+ '-c', `ulimit -n ${limit} && exec "$0" "$@"`,
+ process.execPath, '--disable-warning=ExperimentalWarning', '-e', child,
+ ], {
+ encoding: 'utf8',
+ env: { ...process.env, TMPDIR: path.join(os.tmpdir(), 'no-such-directory') },
+ });
+ assert.strictEqual(status, 0, stderr);
+});
+
test('new ffi.DynamicLibrary() loads from a mounted VFS', () => {
const lib = new ffi.DynamicLibrary(virtualPath);
@@ -92,3 +150,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..93a6cf7521b 100644
--- a/test/parallel/test-dlopen-binary-image-cleanup.js
+++ b/test/parallel/test-dlopen-binary-image-cleanup.js
@@ -7,9 +7,8 @@
// 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, and the fd is closed right after the
+// load, whether or not it worked.
// 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 +44,48 @@ 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: no load
+ // may keep one.
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();
// 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 afterLoads = 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 afterFailures = imageFds();
+
+ if (afterLoads !== null) {
+ if (afterLoads.length !== 0) {
+ throw new Error(\`loads kept images: \${afterLoads}\`);
+ }
+ if (afterFailures.length !== 0) {
+ throw new Error(\`failed loads kept images: \${afterFailures}\`);
+ }
}
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..cede831c880
--- /dev/null
+++ b/test/parallel/test-vfs-addon-sequence.js
@@ -0,0 +1,60 @@
+// Flags: --disable-warning=ExperimentalWarning
+'use strict';
+const common = require('../common');
+const assert = require('node:assert');
+const { spawnSync } = require('node:child_process');
+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();
+
+// An addon on the main thread stays loaded and keeps the path it was loaded
+// through, so enough of them leave no fd number to load another through. Loading
+// has to carry on regardless, through the temp-file fallback.
+if (common.isLinux) {
+ const limit = 128;
+ const child = `
+ const assert = require('node:assert');
+ const fs = require('node:fs');
+ const path = require('node:path');
+ const vfs = require('node:vfs');
+ const myVfs = vfs.create();
+ myVfs.writeFileSync('/function.node', fs.readFileSync(${JSON.stringify(functionPath)}));
+ const mountPoint = myVfs.mount();
+ for (let i = 0; i < ${limit * 2}; i++) {
+ const addonPath = path.join(mountPoint, 'function.node');
+ assert.strictEqual(require(addonPath)(), 'world');
+ delete require.cache[addonPath];
+ }
+ `;
+ const { status, stderr } = spawnSync('/bin/sh', [
+ '-c', `ulimit -n ${limit} && exec "$0" "$@"`,
+ process.execPath, '--disable-warning=ExperimentalWarning', '-e', child,
+ ], { encoding: 'utf8' });
+ assert.strictEqual(status, 0, stderr);
+}
diff --git a/test/parallel/test-vfs-addon.js b/test/parallel/test-vfs-addon.js
index e28225a6c69..a6e2f37b22e 100644
--- a/test/parallel/test-vfs-addon.js
+++ b/test/parallel/test-vfs-addon.js
@@ -3,8 +3,8 @@
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
-const os = require('os');
const path = require('path');
+const tmpdir = require('../common/tmpdir');
const vfs = require('node:vfs');
const addonPath = path.join(
@@ -21,7 +21,11 @@ const myVfs = vfs.create();
myVfs.writeFileSync('/binding.node', fs.readFileSync(addonPath));
const mountPoint = myVfs.mount();
-const before = new Set(fs.readdirSync(os.tmpdir()));
+// Images land in TMPDIR, which other tests loading addons share. Point it at a
+// directory this test owns, so their images cannot pass for a leak here.
+tmpdir.refresh();
+process.env.TMPDIR = tmpdir.path;
+
const addon = require(path.join(mountPoint, 'binding.node'));
assert.strictEqual(addon.hello(), 'world');
@@ -29,8 +33,8 @@ assert.strictEqual(addon.hello(), 'world');
// temp image is unlinked right after loading, so no addon temp file lingers.
// (Windows keeps a delete-on-close file until exit, so skip the check there.)
if (process.platform !== 'win32') {
- const leaked = fs.readdirSync(os.tmpdir())
- .filter((f) => f.startsWith('node-addon') && !before.has(f));
+ const leaked = fs.readdirSync(tmpdir.path)
+ .filter((f) => f.startsWith('node-addon'));
assert.deepStrictEqual(leaked, [], `addon temp not cleaned up: ${leaked}`);
}