src: load addon images on Windows and delete them again

src/node_binding.cc

@@ -8,6 +8,7 @@
 #include "permission/permission.h"
 #include "util.h"
 
+#include <cstdlib>
 #include <string>
 #include <utility>
 #include <vector>
@@ -18,7 +19,6 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <cerrno>
-#include <cstdlib>
 #if defined(__linux__)
 #include <sys/mman.h>
 #include <sys/syscall.h>
@@ -477,10 +477,47 @@ int NodeMemfdCreate(const char* name, unsigned int flags) {
 #endif  // __linux__
 #else   // _WIN32
 
-// Delete-on-close handles kept alive until process exit so their temp files
-// outlive the loaded DLLs and are removed once the process ends.
-Mutex g_retained_addon_handles_mutex;
-std::vector<HANDLE>* g_retained_addon_handles = nullptr;
+// Windows refuses to unlink a file that backs a mapped image section: neither
+// delete-on-close, nor DeleteFile(), nor a POSIX-semantics disposition can
+// remove it while the DLL is loaded. A materialized image therefore has to
+// outlive its load, and the only moment it can go is once the module is
+// unloaded again. Node keeps addons loaded for the life of the process, so
+// that moment is process exit: each image is kept here with the module it was
+// loaded as, and released together at exit.
+struct RetainedAddonImage {
+  HMODULE module;
+  std::wstring path;
+};
+Mutex g_retained_addon_images_mutex;
+std::vector<RetainedAddonImage>* g_retained_addon_images = nullptr;
+
+// Unloads the images this process materialized -- and only those; addons loaded
+// from a real path are left alone -- so that each file can finally be deleted.
+// This has to happen after everything that might still call into an addon, so
+// it is registered during static initialisation below: atexit() runs handlers
+// last-registered-first, so registering before main() puts this behind every
+// handler that is registered while running.
+void ReleaseRetainedAddonImages() {
+  Mutex::ScopedLock lock(g_retained_addon_images_mutex);
+  if (g_retained_addon_images == nullptr) return;
+  for (auto it = g_retained_addon_images->rbegin();
+       it != g_retained_addon_images->rend();
+       ++it) {
+    // Deleting first doubles as the test for whether the image is still
+    // mapped, because that is the only thing that can stop it: an FFI library
+    // the caller already close()d is gone by now, and unloading it a second
+    // time through a stale module handle would be wrong.
+    if (DeleteFileW(it->path.c_str())) continue;
+    if (it->module != nullptr) FreeLibrary(it->module);
+    DeleteFileW(it->path.c_str());
+  }
+  g_retained_addon_images->clear();
+}
+
+// Arms the hook before main() rather than at the first load; see above.
+const struct RetainedAddonImageExitHook {
+  RetainedAddonImageExitHook() { atexit(ReleaseRetainedAddonImages); }
+} g_retained_addon_image_exit_hook;
 
 #endif  // !_WIN32
 
@@ -490,12 +527,6 @@ std::vector<HANDLE>* g_retained_addon_handles = nullptr;
 // dynamically shared objects, node_ffi.cc, can reuse it; see the header for
 // the platform-by-platform description.
 
-AddonImage::AddonImage() {
-#ifdef _WIN32
-  handle_ = INVALID_HANDLE_VALUE;
-#endif
-}
-
 #ifdef _WIN32
 
 // static
@@ -524,67 +555,75 @@ bool AddonImage::Materialize(const char* data, size_t len) {
     errmsg_ = "could not create a temporary file name";
     return false;
   }
-  // Reopen the just-created file delete-on-close, sharing delete so the loader
-  // can map it while it is delete-pending; the file is removed when this handle
-  // and the loader's section are both released (i.e. at process exit).
-  handle_ = CreateFileW(file,
-                        GENERIC_READ | GENERIC_WRITE,
-                        FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
-                        nullptr,
-                        CREATE_ALWAYS,
-                        FILE_ATTRIBUTE_TEMPORARY | FILE_FLAG_DELETE_ON_CLOSE,
-                        nullptr);
-  if (handle_ == INVALID_HANDLE_VALUE) {
+  // Write the image and close it again: nothing may still hold the file open
+  // when the loader gets to it. Sharing is checked in both directions, and the
+  // loader opens a DLL for read and execute while sharing read alone, so any
+  // handle of ours holding write access fails the load with
+  // ERROR_SHARING_VIOLATION however permissive this side's share mode is.
+  HANDLE writer =
+      CreateFileW(file,
+                  GENERIC_WRITE,
+                  FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                  nullptr,
+                  CREATE_ALWAYS,
+                  FILE_ATTRIBUTE_TEMPORARY,
+                  nullptr);
+  if (writer == INVALID_HANDLE_VALUE) {
     errmsg_ = "could not create a temporary file for the native addon";
+    DeleteFileW(file);
     return false;
   }
   size_t off = 0;
   while (off < len) {
     DWORD chunk =
         len - off > MAXDWORD ? MAXDWORD : static_cast<DWORD>(len - off);
     DWORD written = 0;
-    if (!WriteFile(handle_, data + off, chunk, &written, nullptr)) {
+    if (!WriteFile(writer, data + off, chunk, &written, nullptr)) {
       errmsg_ = "could not write the native addon to a temporary file";
-      CloseHandle(handle_);
-      handle_ = INVALID_HANDLE_VALUE;
+      CloseHandle(writer);
+      DeleteFileW(file);
       return false;
     }
     off += written;
   }
+  CloseHandle(writer);
+
   int utf8_len =
       WideCharToMultiByte(CP_UTF8, 0, file, -1, nullptr, 0, nullptr, nullptr);
   if (utf8_len <= 0) {
     errmsg_ = "could not encode the temporary file path";
-    CloseHandle(handle_);
-    handle_ = INVALID_HANDLE_VALUE;
+    DeleteFileW(file);
     return false;
   }
   path_.resize(utf8_len - 1);
   WideCharToMultiByte(
       CP_UTF8, 0, file, -1, path_.data(), utf8_len, nullptr, nullptr);
+  wpath_ = file;
   return true;
 }
 
-void AddonImage::AfterOpen(bool opened) {
+void AddonImage::AfterOpen(bool opened, void* module) {
   consumed_ = true;
-  if (handle_ == INVALID_HANDLE_VALUE) return;
+  if (wpath_.empty()) return;
   if (!opened) {
-    CloseHandle(handle_);  // delete-on-close removes the file
-    handle_ = INVALID_HANDLE_VALUE;
+    DeleteFileW(wpath_.c_str());  // nothing mapped it, so it can go now
+    wpath_.clear();
     return;
   }
-  Mutex::ScopedLock lock(g_retained_addon_handles_mutex);
-  if (g_retained_addon_handles == nullptr) {
-    g_retained_addon_handles = new std::vector<HANDLE>();
+  // The load mapped it, so it has to stay until that module is unloaded again.
+  Mutex::ScopedLock lock(g_retained_addon_images_mutex);
+  if (g_retained_addon_images == nullptr) {
+    g_retained_addon_images = new std::vector<RetainedAddonImage>();
   }
-  g_retained_addon_handles->push_back(handle_);
-  handle_ = INVALID_HANDLE_VALUE;
+  g_retained_addon_images->push_back(
+      {static_cast<HMODULE>(module), std::move(wpath_)});
+  wpath_.clear();
 }
 
 AddonImage::~AddonImage() {
-  // Materialized but Open() was never reached (e.g. an exception in between):
-  // closing the delete-on-close handle removes the file.
-  if (!consumed_ && handle_ != INVALID_HANDLE_VALUE) CloseHandle(handle_);
+  // Materialized but the load was never reached (e.g. an exception in
+  // between): nothing mapped the file, so remove it now.
+  if (!consumed_ && !wpath_.empty()) DeleteFileW(wpath_.c_str());
 }
 
 #else  // !_WIN32
@@ -666,10 +705,12 @@ bool AddonImage::MaterializeTempFile(const char* data, size_t len) {
   return true;
 }
 
-void AddonImage::AfterOpen(bool opened) {
+void AddonImage::AfterOpen(bool opened, void* module) {
   consumed_ = true;
-  // The right cleanup is the same whether or not the load worked.
+  // 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) {
     close(fd_);
@@ -778,7 +819,7 @@ static void DLOpenImpl(const FunctionCallbackInfo<Value>& args,
     Mutex::ScopedLock lock(dlib_load_mutex);
 
     const bool is_opened = dlib->Open();
-    image.AfterOpen(is_opened);
+    image.AfterOpen(is_opened, is_opened ? dlib->handle_ : nullptr);
 
     // Objects containing v14 or later modules will have registered themselves
     // on the pending list.  Activate all of them now.  At present, only one

src/node_binding.h

@@ -179,16 +179,17 @@ void DLOpenBinary(const v8::FunctionCallbackInfo<v8::Value>& args);
 //                 the bytes never touch the filesystem.
 //   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 opened FILE_FLAG_DELETE_ON_CLOSE; its handle is
-//                 retained for the process lifetime so the file is removed
-//                 automatically once the process (and the loaded DLL) exit.
+//   Windows:      a temp file, written and closed before the load because the
+//                 loader shares read alone. It cannot be unlinked while its
+//                 image is mapped, so it is kept with the module it loaded as
+//                 and both are released at process exit.
 // Used for a native addon or an FFI library that lives somewhere the dynamic
 // loader cannot open by path, such as a virtual file system. Call exactly one
 // of Materialize()+AfterOpen() around the load; a destroyed image that never
 // reached AfterOpen() cleans up after itself.
 class AddonImage {
  public:
-  AddonImage();
+  AddonImage() = default;
   ~AddonImage();
   AddonImage(const AddonImage&) = delete;
   AddonImage& operator=(const AddonImage&) = delete;
@@ -204,19 +205,20 @@ class AddonImage {
   const std::string& errmsg() const { return errmsg_; }
 
   // Call exactly once, right after the load; `opened` says whether the load
-  // succeeded. Releases the transient resources that are no longer needed (a
-  // successful load holds its own mapping): on POSIX closes the memfd or
-  // unlinks the temp file; on Windows retains the delete-on-close handle for
-  // the process lifetime when opened, or closes it (deleting the file) on
-  // failure.
-  void AfterOpen(bool opened);
+  // 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
+  // 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.
+  void AfterOpen(bool opened, void* module);
 
  private:
   std::string path_;
   std::string errmsg_;
   bool consumed_ = false;
 #ifdef _WIN32
-  void* handle_;  // HANDLE; void* keeps windows.h out of this header
+  std::wstring wpath_;  // the path of the image, to delete it again at exit
 #else
   bool MaterializeTempFile(const char* data, size_t len);
   int fd_ = -1;

src/node_ffi.cc

@@ -561,7 +561,8 @@ void DynamicLibrary::New(const FunctionCallbackInfo<Value>& args) {
   CHECK(lib->is_closed());
   // Open the library
   const bool opened = uv_dlopen(library_path, &lib->lib_) == 0;
-  image.AfterOpen(opened);
+  image.AfterOpen(opened, opened ? static_cast<void*>(lib->lib_.handle)
+                                 : nullptr);
   if (!opened) {
     THROW_ERR_FFI_CALL_FAILED(env, "dlopen failed: %s", uv_dlerror(&lib->lib_));
     return;

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

@@ -0,0 +1,88 @@
+// Flags: --expose-internals
+'use strict';
+
+// Loading an addon from bytes materializes them into a private image so the
+// dynamic loader has a real path to open. That image is transient and must not
+// outlive the process that loaded it. How it is held differs by platform, so
+// 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.
+//   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
+//                the image has to stay put; a retained FILE_FLAG_DELETE_ON_CLOSE
+//                handle removes it when the process ends.
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const { spawnSync } = require('child_process');
+const tmpdir = require('../common/tmpdir');
+
+const addonPath = path.join(
+  __dirname, '..', 'addons', 'hello-world', 'build', 'Release', 'binding.node');
+if (!fs.existsSync(addonPath)) common.skip('the hello-world addon is not built');
+
+tmpdir.refresh();
+
+// Where a temp-file image would land: GetTempPathW() reads TMP/TEMP and
+// TempDir() reads TMPDIR, so pointing all three at a directory this test owns
+// keeps any image the child writes somewhere it can inspect afterwards. Linux
+// normally uses a memfd and never writes here at all.
+const imageDir = tmpdir.resolve('addon-images');
+fs.mkdirSync(imageDir, { recursive: true });
+
+const child = `
+  const fs = require('fs');
+  const { internalBinding } = require('internal/test/binding');
+  const { dlopenBinary } = internalBinding('process_methods');
+  const bytes = fs.readFileSync(${JSON.stringify(addonPath)});
+  // A path that does not exist on disk, as a VFS-resident addon would be, so
+  // 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.
+  const fdDir = '/proc/self/fd';
+  const countFds = () => {
+    try { return fs.readdirSync(fdDir).length; } catch { return -1; }
+  };
+  const before = countFds();
+
+  // 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++) {
+    const m = { exports: {} };
+    dlopenBinary(m, virtualPath, 0, bytes);
+    if (m.exports.hello() !== 'world') throw new Error('addon did not load');
+  }
+
+  const after = countFds();
+  if (before !== -1 && after > before) {
+    throw new Error(\`descriptor leak: \${before} -> \${after} after 5 loads\`);
+  }
+  process.exit(0);
+`;
+
+const res = spawnSync(process.execPath, ['--expose-internals', '-e', child], {
+  env: { ...process.env, TMPDIR: imageDir, TMP: imageDir, TEMP: imageDir },
+  encoding: 'utf8',
+});
+
+// The load itself must succeed. On Windows a retained writable handle makes the
+// loader fail with ERROR_SHARING_VIOLATION ("The process cannot access the file
+// because it is being used by another process").
+assert.strictEqual(res.status, 0, `child failed:\n${res.stderr}`);
+
+// Nothing an image left behind may outlive the process that created it. Match
+// the shapes the two on-disk paths produce rather than requiring the directory
+// to be empty, so an unrelated temp file cannot fail this.
+const leftovers = fs.readdirSync(imageDir).filter(
+  (name) => /^nod.*\.tmp$/i.test(name) || name.startsWith('node-addon-'));
+assert.deepStrictEqual(
+  leftovers, [],
+  `materialized addon image outlived the process that loaded it: ${leftovers}`);