vfs: let unloaded memfd images give their fd number back

Moving each image's fd to a number no earlier image had used up one
number per load, even when the image was closed again, so a process
opening libraries in a loop hit the limit on open files and then needed
the temp-file fallback, which a noexec or read-only temporary directory
rules out.

The number only has to differ from those of objects still loaded, and
the loader can say which those are. So ask it with RTLD_NOLOAD, and move
the fd to a higher number only while the path is taken. 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. An object that
dlclose() leaves loaded keeps its number, and only images loaded at the
same time compete for numbers.

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

Author: pipobscure

src/node_binding.cc

@@ -479,20 +479,23 @@ int NodeMemfdCreate(const char* name, unsigned int 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;
+// 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
@@ -654,8 +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, or if the fd
-  // numbers no image has had yet have run out.
+  // 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
@@ -666,14 +669,16 @@ 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";
       close(fd_);
       fd_ = -1;
       return false;
     }
+    fd_ = MoveFdToFreeImagePath(fd_);
+  }
+  if (fd_ != -1) {
     path_ = "/proc/self/fd/" + std::to_string(fd_);
     return true;
   }

src/node_binding.h

@@ -178,8 +178,8 @@ void DLOpenBinary(const v8::FunctionCallbackInfo<v8::Value>& args);
 // footprint each platform allows:
 //   Linux:        an anonymous in-memory memfd, loaded via /proc/self/fd/N -
 //                 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).
+//                 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

test/ffi/test-ffi-vfs.js

@@ -3,6 +3,7 @@
 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');
@@ -67,6 +68,37 @@ test('library images leave no file descriptor behind', { skip: !common.isLinux }
   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);
 

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

@@ -33,8 +33,9 @@ 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.
+// 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 = `