vfs: keep each loaded memfd image open until exit

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());

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. 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.

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();
+  }
+});

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);
 `;

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();