vfs: close memfd images right after loading

Keeping a loaded memfd open until exit gave each image a path of its
own, but held one file descriptor per load for the life of the process:
ffi's close() did not release it, nor did a worker's exit, nor a load
that failed after dlopen(), so a process opening libraries in a loop
would grow without bound.

The fd is only needed until dlopen() returns, as the load's mapping
keeps the memory alive from then on. What has to last is the path, and
that depends only on the number. So move each image's fd to a number no
earlier image has had, using F_DUPFD_CLOEXEC with a process-wide counter
that only goes up, and close it right after the load again. Unlike
closing it after dlclose(), this also holds for an object that dlclose()
leaves loaded, and for addons on the main thread, which are never
unloaded. Should the numbers run out, the image goes to a temp file.

Signed-off-by: Philipp Dunkel <pip@pipobscure.com>

Author: pipobscure

src/node_binding.cc

@@ -476,6 +476,24 @@ 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 left to the kernel, which hands out the lowest free number,
+// the next image would get the same path and silently resolve to this one.
+// dlclose() does not settle it either: it can leave an object loaded. So each
+// image moves its fd to a number that no earlier image has had. Workers share
+// the fd table, so this is process-wide.
+int MoveFdToUnusedNumber(int fd) {
+  static Mutex mutex;
+  static int next_number = 0;
+  Mutex::ScopedLock lock(mutex);
+  const int moved = fcntl(fd, F_DUPFD_CLOEXEC, next_number);
+  close(fd);
+  if (moved != -1) next_number = moved + 1;
+  return moved;
+}
 #endif  // __linux__
 #else   // _WIN32
 
@@ -636,7 +654,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 the fd
+  // numbers no image has had yet have run out.
 #ifndef MFD_CLOEXEC
 #define MFD_CLOEXEC 0x0001U
 #endif
@@ -647,6 +666,7 @@ bool AddonImage::Materialize(const char* data, size_t len) {
   if (fd_ == -1 && errno == EINVAL) {
     fd_ = NodeMemfdCreate("node-addon", MFD_CLOEXEC);
   }
+  if (fd_ != -1) fd_ = MoveFdToUnusedNumber(fd_);
   if (fd_ != -1) {
     if (!WriteAllToFd(fd_, data, len)) {
       errmsg_ = "could not write the native addon to an in-memory file";
@@ -709,25 +729,17 @@ bool AddonImage::MaterializeTempFile(const char* data, size_t len) {
 
 void AddonImage::AfterOpen(bool opened, void* module) {
   consumed_ = true;
-  // 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.
+  // 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;
   (void)module;
+  // memfd: the load's mapping (or nothing, on failure) owns it from here.
   if (fd_ != -1) {
-    // 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_);
+    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,9 +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. 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).
+//                 the bytes never touch the filesystem. The fd is closed right
+//                 after the load, and its number is not used for another image
+//                 (the loader would return the earlier object by that path).
 //   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
@@ -209,9 +209,8 @@ 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 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
+  // 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
   // 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

@@ -46,6 +46,27 @@ test('ffi.dlopen() loads a library from a mounted VFS', () => {
   }
 });
 
+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('new ffi.DynamicLibrary() loads from a mounted VFS', () => {
   const lib = new ffi.DynamicLibrary(virtualPath);
 

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

@@ -7,10 +7,8 @@
 // this checks both halves of the contract:
 //
 //   Linux:       an anonymous memfd loaded through /proc/self/fd - nothing ever
-//                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.
+//                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
@@ -46,8 +44,8 @@ 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 memfd rather than a file, so count those: each
-  // successful load keeps exactly one, close-on-exec, and a failed load none.
+  // 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 imageFds = () => {
     let fds;
@@ -58,7 +56,6 @@ const child = `
       } catch { return false; }
     });
   };
-  const before = imageFds();
 
   // Load repeatedly: each load materializes its own image, so a leak of an
   // image or a retained handle shows up as growth.
@@ -70,7 +67,7 @@ const child = `
     dlopenBinary(m, virtualPath, undefined, bytes);
     if (m.exports.hello() !== 'world') throw new Error('addon did not load');
   }
-  const loaded = imageFds();
+  const afterLoads = imageFds();
 
   for (let i = 0; i < loads; i++) {
     try {
@@ -80,23 +77,14 @@ const child = `
       if (err.code !== 'ERR_DLOPEN_FAILED') throw err;
     }
   }
-  const failed = imageFds();
+  const afterFailures = 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 (afterLoads !== null) {
+    if (afterLoads.length !== 0) {
+      throw new Error(\`loads kept images: \${afterLoads}\`);
     }
-    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\`);
-      }
+    if (afterFailures.length !== 0) {
+      throw new Error(\`failed loads kept images: \${afterFailures}\`);
     }
   }
   process.exit(0);

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

@@ -2,6 +2,7 @@
 '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');
@@ -31,3 +32,28 @@ assert.strictEqual(fn(), 'world');
 assert.notStrictEqual(fn, hello);
 
 myVfs.unmount();
+
+// Every image takes an fd number no earlier image had, so a low limit runs out
+// of numbers long before it runs out of fds. Loading has to carry on regardless.
+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);
+}