Skip to content

Commit a839799

Browse files
committed
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 <pip@pipobscure.com>
1 parent 98dbf60 commit a839799

5 files changed

Lines changed: 151 additions & 21 deletions

File tree

‎src/node_binding.cc‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -709,17 +709,25 @@ bool AddonImage::MaterializeTempFile(const char* data, size_t len) {
709709

710710
void AddonImage::AfterOpen(bool opened, void* module) {
711711
consumed_ = true;
712-
// The right cleanup is the same whether or not the load worked, and the
713-
// module never has to be unloaded: the name is already gone by now.
714-
(void)opened;
712+
// The module never has to be unloaded here: Node keeps addons loaded for
713+
// the life of the process, and an FFI library is closed by its owner.
715714
(void)module;
716-
// memfd: the load's mapping (or nothing, on failure) owns it from here.
717715
if (fd_ != -1) {
718-
close(fd_);
716+
// The dynamic loader knows a loaded object by the path it was opened
717+
// with, and hands back the object already loaded under a path instead of
718+
// opening the file again. Closing a loaded memfd would free its number
719+
// for the next image, which would then be loaded through the very same
720+
// /proc/self/fd/N and silently resolve to this one. So a memfd that was
721+
// loaded stays open, keeping its path to itself, for the life of the
722+
// process; MFD_CLOEXEC keeps it out of child processes. A failed load
723+
// left nothing under that path, so its number can go back.
724+
if (!opened) close(fd_);
719725
fd_ = -1;
720726
return;
721727
}
722728
if (!temp_dir_.empty()) {
729+
// A temp file can go at once: it lives in a fresh mkdtemp() directory,
730+
// so its path is not handed out again the way an fd number is.
723731
// On success the mapping keeps the inode alive, so unlinking now leaves no
724732
// on-disk trace; on failure this just cleans up.
725733
unlink(path_.c_str());

‎src/node_binding.h‎

Lines changed: 6 additions & 3 deletions
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. A loaded memfd is kept
181+
// open until exit, so no later image reuses its path (the
182+
// loader would return the earlier object by that name).
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
@@ -207,8 +209,9 @@ class AddonImage {
207209

208210
// Call exactly once, right after the load; `opened` says whether the load
209211
// succeeded and `module` is the module handle it produced. Releases what is
210-
// no longer needed: on POSIX closes the memfd or unlinks the temp file, which
211-
// a successful load keeps alive through its own mapping. Windows cannot
212+
// no longer needed: on POSIX unlinks the temp file, which a successful load
213+
// keeps alive through its own mapping, and closes the memfd only when the
214+
// load failed, keeping a loaded one's path unique until exit. Windows cannot
212215
// unlink a mapped image, so there the file is removed at once only when the
213216
// load failed; otherwise it is kept, with `module`, until process exit, where
214217
// the module is unloaded and the file finally deleted.

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

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,3 +92,57 @@ test('a library loaded from a VFS outlives the mount', () => {
9292
lib.close();
9393
}
9494
});
95+
96+
const addonPath = (name) => path.join(
97+
__dirname, '..', 'addons', name, 'build', common.buildType, 'binding.node');
98+
99+
test('an addon loaded after a VFS library is its own module', (t) => {
100+
const helloPath = addonPath('hello-world');
101+
if (!fs.existsSync(helloPath)) {
102+
t.skip('the hello-world addon is not built');
103+
return;
104+
}
105+
const seqVfs = vfs.create();
106+
seqVfs.writeFileSync(`/${libraryName}`, fs.readFileSync(libraryPath));
107+
seqVfs.writeFileSync('/hello.node', fs.readFileSync(helloPath));
108+
const seqMount = seqVfs.mount();
109+
const { lib, functions } = ffi.dlopen(path.join(seqMount, libraryName), {
110+
add_i32: fixtureSymbols.add_i32,
111+
});
112+
113+
try {
114+
// Resolved to the library, the addon would fail with "Module did not
115+
// self-register".
116+
const addon = require(path.join(seqMount, 'hello.node'));
117+
assert.strictEqual(addon.hello(), 'world');
118+
assert.strictEqual(functions.add_i32(20, 22), 42);
119+
} finally {
120+
lib.close();
121+
seqVfs.unmount();
122+
}
123+
});
124+
125+
test('a VFS library loaded after an addon is its own library', (t) => {
126+
const functionPath = addonPath('hello-world-function-export');
127+
if (!fs.existsSync(functionPath)) {
128+
t.skip('the hello-world-function-export addon is not built');
129+
return;
130+
}
131+
const seqVfs = vfs.create();
132+
seqVfs.writeFileSync('/function.node', fs.readFileSync(functionPath));
133+
seqVfs.writeFileSync(`/${libraryName}`, fs.readFileSync(libraryPath));
134+
const seqMount = seqVfs.mount();
135+
const addon = require(path.join(seqMount, 'function.node'));
136+
assert.strictEqual(addon(), 'world');
137+
138+
// Resolved to the addon, the library would have no add_i32 to look up.
139+
const { lib, functions } = ffi.dlopen(path.join(seqMount, libraryName), {
140+
add_i32: fixtureSymbols.add_i32,
141+
});
142+
try {
143+
assert.strictEqual(functions.add_i32(40, 2), 42);
144+
} finally {
145+
lib.close();
146+
seqVfs.unmount();
147+
}
148+
});

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

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,10 @@
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. A loaded memfd stays open until exit,
11+
// close-on-exec, so that no later image is loaded through the
12+
// same /proc/self/fd/N path (the loader would return this one);
13+
// a failed load closes its memfd again.
1314
// other POSIX: a mkdtemp() directory unlinked and rmdir()ed right after the
1415
// load, so nothing is left even while the process runs.
1516
// Windows: the loader maps the file by path for the DLL's lifetime, so
@@ -45,27 +46,58 @@ const child = `
4546
// the load can only come from the bytes and their materialized image.
4647
const virtualPath = ${JSON.stringify(path.join(addonPath, '..', 'nowhere', 'binding.node'))};
4748
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.
49+
// On Linux the image is a memfd rather than a file, so count those: each
50+
// successful load keeps exactly one, close-on-exec, and a failed load none.
5051
const fdDir = '/proc/self/fd';
51-
const countFds = () => {
52-
try { return fs.readdirSync(fdDir).length; } catch { return -1; }
52+
const imageFds = () => {
53+
let fds;
54+
try { fds = fs.readdirSync(fdDir); } catch { return null; }
55+
return fds.filter((fd) => {
56+
try {
57+
return fs.readlinkSync(\`\${fdDir}/\${fd}\`).startsWith('/memfd:node-addon');
58+
} catch { return false; }
59+
});
5360
};
54-
const before = countFds();
61+
const before = imageFds();
5562
5663
// 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++) {
64+
// image or a retained handle shows up as growth.
65+
const loads = 5;
66+
for (let i = 0; i < loads; i++) {
5967
const m = { exports: {} };
6068
// flags undefined: keep the default dlopen(2) mode - 0 is not valid
6169
// everywhere (glibc rejects it with EINVAL).
6270
dlopenBinary(m, virtualPath, undefined, bytes);
6371
if (m.exports.hello() !== 'world') throw new Error('addon did not load');
6472
}
73+
const loaded = imageFds();
6574
66-
const after = countFds();
67-
if (before !== -1 && after > before) {
68-
throw new Error(\`descriptor leak: \${before} -> \${after} after 5 loads\`);
75+
for (let i = 0; i < loads; i++) {
76+
try {
77+
dlopenBinary({ exports: {} }, virtualPath, undefined, Buffer.from('nope'));
78+
throw new Error('loading garbage bytes did not fail');
79+
} catch (err) {
80+
if (err.code !== 'ERR_DLOPEN_FAILED') throw err;
81+
}
82+
}
83+
const failed = imageFds();
84+
85+
if (before !== null) {
86+
const kept = loaded.filter((fd) => !before.includes(fd));
87+
// Zero when memfd is unavailable and the temp-file fallback was used.
88+
if (kept.length !== 0 && kept.length !== loads) {
89+
throw new Error(\`expected \${loads} retained images, got \${kept.length}\`);
90+
}
91+
if (failed.length !== loaded.length) {
92+
throw new Error(\`failed loads kept images: \${loaded} -> \${failed}\`);
93+
}
94+
for (const fd of kept) {
95+
const info = fs.readFileSync(\`/proc/self/fdinfo/\${fd}\`, 'utf8');
96+
const flags = parseInt(/^flags:\\s*([0-7]+)/m.exec(info)[1], 8);
97+
if ((flags & 0o2000000) === 0) { // O_CLOEXEC
98+
throw new Error(\`retained image fd \${fd} is not close-on-exec\`);
99+
}
100+
}
69101
}
70102
process.exit(0);
71103
`;
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// Flags: --disable-warning=ExperimentalWarning
2+
'use strict';
3+
const common = require('../common');
4+
const assert = require('node:assert');
5+
const fs = require('node:fs');
6+
const path = require('node:path');
7+
const vfs = require('node:vfs');
8+
9+
const addonPath = (name) => path.join(
10+
__dirname, '..', 'addons', name, 'build', common.buildType, 'binding.node');
11+
const helloPath = addonPath('hello-world');
12+
const functionPath = addonPath('hello-world-function-export');
13+
if (!fs.existsSync(helloPath) || !fs.existsSync(functionPath)) {
14+
common.skip('the hello-world addons are not built');
15+
}
16+
17+
const myVfs = vfs.create();
18+
myVfs.writeFileSync('/hello.node', fs.readFileSync(helloPath));
19+
myVfs.writeFileSync('/function.node', fs.readFileSync(functionPath));
20+
const mountPoint = myVfs.mount();
21+
22+
const hello = require(path.join(mountPoint, 'hello.node'));
23+
assert.strictEqual(typeof hello, 'object');
24+
assert.strictEqual(hello.hello(), 'world');
25+
26+
// hello-world-function-export replaces its exports with a function. Resolved
27+
// to the first image, this would be another { hello } object instead.
28+
const fn = require(path.join(mountPoint, 'function.node'));
29+
assert.strictEqual(typeof fn, 'function');
30+
assert.strictEqual(fn(), 'world');
31+
assert.notStrictEqual(fn, hello);
32+
33+
myVfs.unmount();

0 commit comments

Comments
 (0)