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