From b1dca5011e03de548376c851192dc010508b1c89 Mon Sep 17 00:00:00 2001 From: Philipp Dunkel Date: Thu, 10 Sep 2026 23:21:55 +0200 Subject: [PATCH] src: load addon images on Windows and delete them again Loading a shared object from bytes never worked on Windows. Materialize() kept its temporary file open with GENERIC_WRITE across the load, and the loader opens a DLL for read and execute while sharing read alone. Sharing is checked in both directions, so an open handle holding write access is refused whatever this side shares: every load failed with ERROR_SHARING_VIOLATION, "The process cannot access the file because it is being used by another process". That is every VFS-resident addon, and with it test-dlopen-binary, test-permission-dlopen-binary and test-vfs-addon. Write the image and close it again before loading, so nothing holds the file when the loader opens it. That exposes the other half. The file cannot be removed while it is loaded: Windows refuses to unlink a file backing a mapped image section, by delete-on-close, by DeleteFile() and by a POSIX-semantics disposition alike, all with ERROR_ACCESS_DENIED. The delete-on-close handle the code retained could therefore never have removed anything; it only appeared to work because the load failed first and the file was deleted on that path. Fixing the load alone leaks an image per addon into the temporary directory. An image can only go once its module is unloaded, and Node keeps addons loaded for the life of the process, so keep each image with the module it was loaded as and release both at exit. Only materialized images are unloaded -- an addon loaded from a real path is untouched. The hook is registered during static initialisation because atexit() runs handlers last-registered-first, which puts it behind every handler registered while running. At exit the delete is attempted before the unload, which doubles as the test for whether the image is still mapped, that being the only thing that can stop it. An FFI library the caller already close()d has been unloaded by uv_dlclose() and its image just goes; unloading it again through the stale module handle would be wrong. With no handle to retain, the delete-on-close file and the read-only reopen go away: Materialize() writes the image, closes it and records its path. Co-Authored-By: Claude Opus 5 (1M context) --- src/node_binding.cc | 125 ++++++++++++------ src/node_binding.h | 24 ++-- src/node_ffi.cc | 3 +- .../test-dlopen-binary-image-cleanup.js | 88 ++++++++++++ 4 files changed, 186 insertions(+), 54 deletions(-) create mode 100644 test/parallel/test-dlopen-binary-image-cleanup.js diff --git a/src/node_binding.cc b/src/node_binding.cc index 5520480293d5..52d388434873 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -8,6 +8,7 @@ #include "permission/permission.h" #include "util.h" +#include #include #include #include @@ -18,7 +19,6 @@ #include #include #include -#include #if defined(__linux__) #include #include @@ -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* 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* 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* 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,18 +555,22 @@ 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; @@ -543,48 +578,52 @@ bool AddonImage::Materialize(const char* data, size_t len) { DWORD chunk = len - off > MAXDWORD ? MAXDWORD : static_cast(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(); + // 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(); } - g_retained_addon_handles->push_back(handle_); - handle_ = INVALID_HANDLE_VALUE; + g_retained_addon_images->push_back( + {static_cast(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& 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 diff --git a/src/node_binding.h b/src/node_binding.h index 3e7d8f64b916..0f19475ab48e 100644 --- a/src/node_binding.h +++ b/src/node_binding.h @@ -179,16 +179,17 @@ void DLOpenBinary(const v8::FunctionCallbackInfo& 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; diff --git a/src/node_ffi.cc b/src/node_ffi.cc index dbdec8d92563..82f9de96315b 100644 --- a/src/node_ffi.cc +++ b/src/node_ffi.cc @@ -561,7 +561,8 @@ void DynamicLibrary::New(const FunctionCallbackInfo& 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(lib->lib_.handle) + : nullptr); if (!opened) { THROW_ERR_FFI_CALL_FAILED(env, "dlopen failed: %s", uv_dlerror(&lib->lib_)); return; diff --git a/test/parallel/test-dlopen-binary-image-cleanup.js b/test/parallel/test-dlopen-binary-image-cleanup.js new file mode 100644 index 000000000000..911a38418c02 --- /dev/null +++ b/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}`);