Skip to content

Commit 5fea2c6

Browse files
committed
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>
1 parent 2929073 commit 5fea2c6

6 files changed

Lines changed: 245 additions & 22 deletions

File tree

‎src/node_binding.cc‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,27 @@ bool WriteAllToFd(int fd, const char* data, size_t len) {
476476
int NodeMemfdCreate(const char* name, unsigned int flags) {
477477
return static_cast<int>(syscall(SYS_memfd_create, name, flags));
478478
}
479+
480+
// The dynamic loader knows a loaded object by the path it was opened with, and
481+
// hands back the object already loaded under a path instead of opening the
482+
// file again. An image is loaded through /proc/self/fd/N and its fd is closed
483+
// right after, so the kernel can hand N to the next image while the first is
484+
// still loaded. So ask the loader, and move to a higher number while the path
485+
// is taken. Holding the fd from here through the load keeps anyone else from
486+
// taking the path in between, and an object that has really been unloaded gives
487+
// its number back, so only images loaded at the same time compete for numbers.
488+
int MoveFdToFreeImagePath(int fd) {
489+
while (true) {
490+
const std::string path = "/proc/self/fd/" + std::to_string(fd);
491+
void* loaded = dlopen(path.c_str(), RTLD_LAZY | RTLD_NOLOAD);
492+
if (loaded == nullptr) return fd;
493+
dlclose(loaded);
494+
const int moved = fcntl(fd, F_DUPFD_CLOEXEC, fd + 1);
495+
close(fd);
496+
if (moved == -1) return -1;
497+
fd = moved;
498+
}
499+
}
479500
#endif // __linux__
480501
#else // _WIN32
481502

@@ -636,7 +657,8 @@ bool AddonImage::Materialize(const char* data, size_t len) {
636657
// reaches the filesystem, so there is no temp file to secure, unlink, or
637658
// leak, and no noexec-mount problem. Request an executable memfd (MFD_EXEC,
638659
// Linux 6.3+); retry without it on older kernels that reject the flag, then
639-
// fall back to a temp file if memfd is unavailable entirely.
660+
// fall back to a temp file if memfd is unavailable entirely, or if so many
661+
// images are loaded that no fd number is left to load another through.
640662
#ifndef MFD_CLOEXEC
641663
#define MFD_CLOEXEC 0x0001U
642664
#endif
@@ -654,6 +676,9 @@ bool AddonImage::Materialize(const char* data, size_t len) {
654676
fd_ = -1;
655677
return false;
656678
}
679+
fd_ = MoveFdToFreeImagePath(fd_);
680+
}
681+
if (fd_ != -1) {
657682
path_ = "/proc/self/fd/" + std::to_string(fd_);
658683
return true;
659684
}

‎src/node_binding.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,9 @@ void DLOpenBinary(const v8::FunctionCallbackInfo<v8::Value>& args);
177177
// dlopen()/LoadLibrary() can load, with the smallest, most private on-disk
178178
// footprint each platform allows:
179179
// Linux: an anonymous in-memory memfd, loaded via /proc/self/fd/N -
180-
// the bytes never touch the filesystem.
180+
// the bytes never touch the filesystem. The fd is closed right
181+
// after the load, and a number whose path still names a loaded
182+
// object is skipped (the loader would return that object).
181183
// other POSIX: a 0700 mkdtemp() directory plus an O_EXCL|O_NOFOLLOW file,
182184
// unlink()ed right after the load (the mapping keeps it alive).
183185
// Windows: a temp file, written and closed before the load because the

‎test/ffi/test-ffi-vfs.js‎

Lines changed: 115 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,12 @@
33
const common = require('../common');
44
common.skipIfFFIMissing();
55
const assert = require('node:assert');
6+
const { spawnSync } = require('node:child_process');
67
const fs = require('node:fs');
78
const os = require('node:os');
89
const path = require('node:path');
910
const { test } = require('node:test');
11+
const tmpdir = require('../common/tmpdir');
1012
const ffi = require('node:ffi');
1113
const vfs = require('node:vfs');
1214
const { fixtureSymbols, libraryPath } = require('./ffi-test-common');
@@ -21,8 +23,12 @@ myVfs.writeFileSync(`/${libraryName}`, fs.readFileSync(libraryPath));
2123
const mountPoint = myVfs.mount();
2224
const virtualPath = path.join(mountPoint, libraryName);
2325

26+
// Images land in TMPDIR, which other tests loading libraries share. Point it at
27+
// a directory this test owns, so their images cannot pass for a leak here.
28+
tmpdir.refresh();
29+
process.env.TMPDIR = tmpdir.path;
30+
2431
test('ffi.dlopen() loads a library from a mounted VFS', () => {
25-
const before = new Set(fs.readdirSync(os.tmpdir()));
2632
const { lib, functions } = ffi.dlopen(virtualPath, {
2733
add_i32: fixtureSymbols.add_i32,
2834
});
@@ -40,12 +46,64 @@ test('ffi.dlopen() loads a library from a mounted VFS', () => {
4046
// filesystem; on other POSIX it is unlinked right after loading.
4147
// (Windows keeps a delete-on-close file until exit, so skip there.)
4248
if (!common.isWindows) {
43-
const leaked = fs.readdirSync(os.tmpdir())
44-
.filter((f) => f.startsWith('node-addon') && !before.has(f));
49+
const leaked = fs.readdirSync(tmpdir.path)
50+
.filter((f) => f.startsWith('node-addon'));
4551
assert.deepStrictEqual(leaked, [], `image not cleaned up: ${leaked}`);
4652
}
4753
});
4854

55+
test('library images leave no file descriptor behind', { skip: !common.isLinux }, () => {
56+
const images = () => fs.readdirSync('/proc/self/fd').filter((fd) => {
57+
try {
58+
return fs.readlinkSync(`/proc/self/fd/${fd}`).startsWith('/memfd:node-addon');
59+
} catch {
60+
return false;
61+
}
62+
});
63+
64+
const { lib, functions } = ffi.dlopen(virtualPath, {
65+
add_i32: fixtureSymbols.add_i32,
66+
});
67+
try {
68+
assert.deepStrictEqual(images(), []);
69+
assert.strictEqual(functions.add_i32(40, 2), 42);
70+
} finally {
71+
lib.close();
72+
}
73+
assert.deepStrictEqual(images(), []);
74+
});
75+
76+
test('closed library images give their fd numbers back', { skip: !common.isLinux }, () => {
77+
// More loads than the limit allows fds, one at a time. There is no usable
78+
// temporary directory, so this only works if each stays an in-memory image.
79+
const limit = 128;
80+
const child = `
81+
const assert = require('node:assert');
82+
const fs = require('node:fs');
83+
const path = require('node:path');
84+
const ffi = require('node:ffi');
85+
const vfs = require('node:vfs');
86+
const myVfs = vfs.create();
87+
myVfs.writeFileSync('/lib.so', fs.readFileSync(${JSON.stringify(libraryPath)}));
88+
const libPath = path.join(myVfs.mount(), 'lib.so');
89+
for (let i = 0; i < ${limit * 4}; i++) {
90+
const { lib, functions } = ffi.dlopen(libPath, {
91+
add_i32: ${JSON.stringify(fixtureSymbols.add_i32)},
92+
});
93+
assert.strictEqual(functions.add_i32(40, 2), 42);
94+
lib.close();
95+
}
96+
`;
97+
const { status, stderr } = spawnSync('/bin/sh', [
98+
'-c', `ulimit -n ${limit} && exec "$0" "$@"`,
99+
process.execPath, '--disable-warning=ExperimentalWarning', '-e', child,
100+
], {
101+
encoding: 'utf8',
102+
env: { ...process.env, TMPDIR: path.join(os.tmpdir(), 'no-such-directory') },
103+
});
104+
assert.strictEqual(status, 0, stderr);
105+
});
106+
49107
test('new ffi.DynamicLibrary() loads from a mounted VFS', () => {
50108
const lib = new ffi.DynamicLibrary(virtualPath);
51109

@@ -92,3 +150,57 @@ test('a library loaded from a VFS outlives the mount', () => {
92150
lib.close();
93151
}
94152
});
153+
154+
const addonPath = (name) => path.join(
155+
__dirname, '..', 'addons', name, 'build', common.buildType, 'binding.node');
156+
157+
test('an addon loaded after a VFS library is its own module', (t) => {
158+
const helloPath = addonPath('hello-world');
159+
if (!fs.existsSync(helloPath)) {
160+
t.skip('the hello-world addon is not built');
161+
return;
162+
}
163+
const seqVfs = vfs.create();
164+
seqVfs.writeFileSync(`/${libraryName}`, fs.readFileSync(libraryPath));
165+
seqVfs.writeFileSync('/hello.node', fs.readFileSync(helloPath));
166+
const seqMount = seqVfs.mount();
167+
const { lib, functions } = ffi.dlopen(path.join(seqMount, libraryName), {
168+
add_i32: fixtureSymbols.add_i32,
169+
});
170+
171+
try {
172+
// Resolved to the library, the addon would fail with "Module did not
173+
// self-register".
174+
const addon = require(path.join(seqMount, 'hello.node'));
175+
assert.strictEqual(addon.hello(), 'world');
176+
assert.strictEqual(functions.add_i32(20, 22), 42);
177+
} finally {
178+
lib.close();
179+
seqVfs.unmount();
180+
}
181+
});
182+
183+
test('a VFS library loaded after an addon is its own library', (t) => {
184+
const functionPath = addonPath('hello-world-function-export');
185+
if (!fs.existsSync(functionPath)) {
186+
t.skip('the hello-world-function-export addon is not built');
187+
return;
188+
}
189+
const seqVfs = vfs.create();
190+
seqVfs.writeFileSync('/function.node', fs.readFileSync(functionPath));
191+
seqVfs.writeFileSync(`/${libraryName}`, fs.readFileSync(libraryPath));
192+
const seqMount = seqVfs.mount();
193+
const addon = require(path.join(seqMount, 'function.node'));
194+
assert.strictEqual(addon(), 'world');
195+
196+
// Resolved to the addon, the library would have no add_i32 to look up.
197+
const { lib, functions } = ffi.dlopen(path.join(seqMount, libraryName), {
198+
add_i32: fixtureSymbols.add_i32,
199+
});
200+
try {
201+
assert.strictEqual(functions.add_i32(40, 2), 42);
202+
} finally {
203+
lib.close();
204+
seqVfs.unmount();
205+
}
206+
});

‎test/parallel/test-dlopen-binary-image-cleanup.js‎

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,8 @@
77
// this checks both halves of the contract:
88
//
99
// Linux: an anonymous memfd loaded through /proc/self/fd - nothing ever
10-
// reaches the filesystem, and AfterOpen() closes the descriptor
11-
// once the load owns its mapping, so repeated loads must not
12-
// accumulate open descriptors.
10+
// reaches the filesystem, and the fd is closed right after the
11+
// load, whether or not it worked.
1312
// other POSIX: a mkdtemp() directory unlinked and rmdir()ed right after the
1413
// load, so nothing is left even while the process runs.
1514
// Windows: the loader maps the file by path for the DLL's lifetime, so
@@ -45,27 +44,48 @@ const child = `
4544
// the load can only come from the bytes and their materialized image.
4645
const virtualPath = ${JSON.stringify(path.join(addonPath, '..', 'nowhere', 'binding.node'))};
4746
48-
// On Linux the image is a descriptor rather than a file, so count them: each
49-
// load must hand its fd to the mapping and close it, leaving no growth.
47+
// On Linux the image is a memfd rather than a file, so count those: no load
48+
// may keep one.
5049
const fdDir = '/proc/self/fd';
51-
const countFds = () => {
52-
try { return fs.readdirSync(fdDir).length; } catch { return -1; }
50+
const imageFds = () => {
51+
let fds;
52+
try { fds = fs.readdirSync(fdDir); } catch { return null; }
53+
return fds.filter((fd) => {
54+
try {
55+
return fs.readlinkSync(\`\${fdDir}/\${fd}\`).startsWith('/memfd:node-addon');
56+
} catch { return false; }
57+
});
5358
};
54-
const before = countFds();
5559
5660
// Load repeatedly: each load materializes its own image, so a leak of an
57-
// image, a descriptor or a retained handle shows up as growth.
58-
for (let i = 0; i < 5; i++) {
61+
// image or a retained handle shows up as growth.
62+
const loads = 5;
63+
for (let i = 0; i < loads; i++) {
5964
const m = { exports: {} };
6065
// flags undefined: keep the default dlopen(2) mode - 0 is not valid
6166
// everywhere (glibc rejects it with EINVAL).
6267
dlopenBinary(m, virtualPath, undefined, bytes);
6368
if (m.exports.hello() !== 'world') throw new Error('addon did not load');
6469
}
70+
const afterLoads = imageFds();
6571
66-
const after = countFds();
67-
if (before !== -1 && after > before) {
68-
throw new Error(\`descriptor leak: \${before} -> \${after} after 5 loads\`);
72+
for (let i = 0; i < loads; i++) {
73+
try {
74+
dlopenBinary({ exports: {} }, virtualPath, undefined, Buffer.from('nope'));
75+
throw new Error('loading garbage bytes did not fail');
76+
} catch (err) {
77+
if (err.code !== 'ERR_DLOPEN_FAILED') throw err;
78+
}
79+
}
80+
const afterFailures = imageFds();
81+
82+
if (afterLoads !== null) {
83+
if (afterLoads.length !== 0) {
84+
throw new Error(\`loads kept images: \${afterLoads}\`);
85+
}
86+
if (afterFailures.length !== 0) {
87+
throw new Error(\`failed loads kept images: \${afterFailures}\`);
88+
}
6989
}
7090
process.exit(0);
7191
`;
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
// Flags: --disable-warning=ExperimentalWarning
2+
'use strict';
3+
const common = require('../common');
4+
const assert = require('node:assert');
5+
const { spawnSync } = require('node:child_process');
6+
const fs = require('node:fs');
7+
const path = require('node:path');
8+
const vfs = require('node:vfs');
9+
10+
const addonPath = (name) => path.join(
11+
__dirname, '..', 'addons', name, 'build', common.buildType, 'binding.node');
12+
const helloPath = addonPath('hello-world');
13+
const functionPath = addonPath('hello-world-function-export');
14+
if (!fs.existsSync(helloPath) || !fs.existsSync(functionPath)) {
15+
common.skip('the hello-world addons are not built');
16+
}
17+
18+
const myVfs = vfs.create();
19+
myVfs.writeFileSync('/hello.node', fs.readFileSync(helloPath));
20+
myVfs.writeFileSync('/function.node', fs.readFileSync(functionPath));
21+
const mountPoint = myVfs.mount();
22+
23+
const hello = require(path.join(mountPoint, 'hello.node'));
24+
assert.strictEqual(typeof hello, 'object');
25+
assert.strictEqual(hello.hello(), 'world');
26+
27+
// hello-world-function-export replaces its exports with a function. Resolved
28+
// to the first image, this would be another { hello } object instead.
29+
const fn = require(path.join(mountPoint, 'function.node'));
30+
assert.strictEqual(typeof fn, 'function');
31+
assert.strictEqual(fn(), 'world');
32+
assert.notStrictEqual(fn, hello);
33+
34+
myVfs.unmount();
35+
36+
// An addon on the main thread stays loaded and keeps the path it was loaded
37+
// through, so enough of them leave no fd number to load another through. Loading
38+
// has to carry on regardless, through the temp-file fallback.
39+
if (common.isLinux) {
40+
const limit = 128;
41+
const child = `
42+
const assert = require('node:assert');
43+
const fs = require('node:fs');
44+
const path = require('node:path');
45+
const vfs = require('node:vfs');
46+
const myVfs = vfs.create();
47+
myVfs.writeFileSync('/function.node', fs.readFileSync(${JSON.stringify(functionPath)}));
48+
const mountPoint = myVfs.mount();
49+
for (let i = 0; i < ${limit * 2}; i++) {
50+
const addonPath = path.join(mountPoint, 'function.node');
51+
assert.strictEqual(require(addonPath)(), 'world');
52+
delete require.cache[addonPath];
53+
}
54+
`;
55+
const { status, stderr } = spawnSync('/bin/sh', [
56+
'-c', `ulimit -n ${limit} && exec "$0" "$@"`,
57+
process.execPath, '--disable-warning=ExperimentalWarning', '-e', child,
58+
], { encoding: 'utf8' });
59+
assert.strictEqual(status, 0, stderr);
60+
}

‎test/parallel/test-vfs-addon.js‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
const common = require('../common');
44
const assert = require('assert');
55
const fs = require('fs');
6-
const os = require('os');
76
const path = require('path');
7+
const tmpdir = require('../common/tmpdir');
88
const vfs = require('node:vfs');
99

1010
const addonPath = path.join(
@@ -21,16 +21,20 @@ const myVfs = vfs.create();
2121
myVfs.writeFileSync('/binding.node', fs.readFileSync(addonPath));
2222
const mountPoint = myVfs.mount();
2323

24-
const before = new Set(fs.readdirSync(os.tmpdir()));
24+
// Images land in TMPDIR, which other tests loading addons share. Point it at a
25+
// directory this test owns, so their images cannot pass for a leak here.
26+
tmpdir.refresh();
27+
process.env.TMPDIR = tmpdir.path;
28+
2529
const addon = require(path.join(mountPoint, 'binding.node'));
2630
assert.strictEqual(addon.hello(), 'world');
2731

2832
// On Linux the memfd path never touches the filesystem; on other POSIX the
2933
// temp image is unlinked right after loading, so no addon temp file lingers.
3034
// (Windows keeps a delete-on-close file until exit, so skip the check there.)
3135
if (process.platform !== 'win32') {
32-
const leaked = fs.readdirSync(os.tmpdir())
33-
.filter((f) => f.startsWith('node-addon') && !before.has(f));
36+
const leaked = fs.readdirSync(tmpdir.path)
37+
.filter((f) => f.startsWith('node-addon'));
3438
assert.deepStrictEqual(leaked, [], `addon temp not cleaned up: ${leaked}`);
3539
}
3640

0 commit comments

Comments
 (0)