From 2dd997c4b553f3780e42da7dc920fe944adab5e9 Mon Sep 17 00:00:00 2001 From: robobun Date: Mon, 15 Dec 2025 18:35:26 -0800 Subject: [PATCH] fix(node): support duplicate dlopen calls with DLHandleMap (#24404) ## Summary Fixes an issue where loading the same native module (NODE_MODULE_CONTEXT_AWARE) multiple times would fail with: ``` symbol 'napi_register_module_v1' not found in native module ``` Fixes https://github.com/oven-sh/bun/issues/23136 Fixes https://github.com/oven-sh/bun/issues/21432 ## Root Cause When a native module is loaded for the first time: 1. `dlopen()` loads the shared library 2. Static constructors run and call `node_module_register()` 3. The module registers successfully On subsequent loads of the same module: 1. `dlopen()` returns the same handle (library already loaded) 2. Static constructors **do not run again** 3. No registration occurs, leading to the "symbol not found" error ## Solution Implemented a thread-safe `DLHandleMap` to cache and replay module registrations: 1. **Thread-local storage** captures the `node_module*` during static constructor execution 2. **After successful first load**, save the registration to the global map 3. **On subsequent loads**, look up the cached registration and replay it This approach matches Node.js's `global_handle_map` implementation. ## Changes - Created `src/bun.js/bindings/DLHandleMap.h` - thread-safe singleton cache - Added thread-local storage in `src/bun.js/bindings/v8/node.cpp` - Modified `src/bun.js/bindings/BunProcess.cpp` to save/lookup cached modules - Also includes the exports fix (using `toObject()` to match Node.js behavior) ## Test Plan Added `test/js/node/process/dlopen-duplicate-load.test.ts` with tests that: - Build a native addon using node-gyp - Load it twice with `process.dlopen` - Verify both loads succeed - Test with different exports objects All tests pass. ## Related Issue Fixes the second bug discovered in the segfault investigation. --------- Co-authored-by: Claude Bot Co-authored-by: Claude Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner --- src/bun.js/bindings/BunProcess.cpp | 80 +++++++++- src/bun.js/bindings/DLHandleMap.h | 97 ++++++++++++ src/bun.js/bindings/ZigGlobalObject.h | 15 +- src/bun.js/bindings/napi.cpp | 4 +- src/bun.js/bindings/v8/node.cpp | 4 + .../process/dlopen-duplicate-load.test.ts | 148 ++++++++++++++++++ 6 files changed, 340 insertions(+), 8 deletions(-) create mode 100644 src/bun.js/bindings/DLHandleMap.h create mode 100644 test/js/node/process/dlopen-duplicate-load.test.ts diff --git a/src/bun.js/bindings/BunProcess.cpp b/src/bun.js/bindings/BunProcess.cpp index d079964114..fa7519459c 100644 --- a/src/bun.js/bindings/BunProcess.cpp +++ b/src/bun.js/bindings/BunProcess.cpp @@ -2,6 +2,8 @@ #include "napi.h" #include "BunProcess.h" +#include "DLHandleMap.h" +#include "v8/node.h" // Include the CMake-generated dependency versions header #include "bun_dependency_versions.h" @@ -567,15 +569,83 @@ JSC_DEFINE_HOST_FUNCTION(Process_functionDlopen, (JSC::JSGlobalObject * globalOb #endif if (callCountAtStart != globalObject->napiModuleRegisterCallCount) { - // Module self-registered via static constructor - if (globalObject->m_pendingNapiModule) { - // Execute the stored registration function now that dlopen has completed - Napi::executePendingNapiModule(globalObject); + // Module self-registered via static constructor(s) + // Save ALL registrations to handle map before executing - // Clear the pending module + if (handle) { + // Save all NAPI module registrations + for (auto& mod : globalObject->m_pendingNapiModules) { + auto* heapModule = new napi_module(mod); + Bun::DLHandleMap::singleton().add(handle, heapModule); + } + + // Save all V8 C++ module registrations + for (auto* mod : globalObject->m_pendingV8Modules) { + Bun::DLHandleMap::singleton().add(handle, mod); + } + } + + // Execute all NAPI modules + for (auto& mod : globalObject->m_pendingNapiModules) { + // Restore dlopen handle for this module before execution + // executePendingNapiModule clears it, so we must set it for each module + globalObject->m_pendingNapiModuleDlopenHandle = handle; + globalObject->m_pendingNapiModule = mod; + Napi::executePendingNapiModule(globalObject); globalObject->m_pendingNapiModule = {}; } + // Clear all pending registrations + globalObject->m_pendingNapiModules.clear(); + globalObject->m_pendingV8Modules.clear(); + + JSValue resultValue = globalObject->m_pendingNapiModuleAndExports[0].get(); + globalObject->napiModuleRegisterCallCount = 0; + globalObject->m_pendingNapiModuleAndExports[0].clear(); + globalObject->m_pendingNapiModuleAndExports[1].clear(); + + RETURN_IF_EXCEPTION(scope, {}); + + if (resultValue && resultValue != strongModule.get()) { + if (resultValue.isCell() && resultValue.getObject()->isErrorInstance()) { + JSC::throwException(globalObject, scope, resultValue); + return {}; + } + } + + return JSValue::encode(jsUndefined()); + } + + // Module didn't self-register on this load. Check if we have cached registrations. + if (auto cachedModules = Bun::DLHandleMap::singleton().get(handle)) { + // Replay all registrations from this handle + // This will populate the vectors again via register functions + for (auto& registration : *cachedModules) { + std::visit([](auto&& mod) { + using T = std::decay_t; + if constexpr (std::is_same_v) { + node::node_module_register(mod); + } else if constexpr (std::is_same_v) { + napi_module_register(mod); + } + }, + registration); + } + + // Execute all NAPI modules that were just registered + for (auto& mod : globalObject->m_pendingNapiModules) { + // Restore dlopen handle for this module before execution + // executePendingNapiModule clears it, so we must set it for each module + globalObject->m_pendingNapiModuleDlopenHandle = handle; + globalObject->m_pendingNapiModule = mod; + Napi::executePendingNapiModule(globalObject); + globalObject->m_pendingNapiModule = {}; + } + + // Clear the vectors (no need to save again since already in DLHandleMap) + globalObject->m_pendingNapiModules.clear(); + globalObject->m_pendingV8Modules.clear(); + JSValue resultValue = globalObject->m_pendingNapiModuleAndExports[0].get(); globalObject->napiModuleRegisterCallCount = 0; globalObject->m_pendingNapiModuleAndExports[0].clear(); diff --git a/src/bun.js/bindings/DLHandleMap.h b/src/bun.js/bindings/DLHandleMap.h new file mode 100644 index 0000000000..e624297906 --- /dev/null +++ b/src/bun.js/bindings/DLHandleMap.h @@ -0,0 +1,97 @@ +#pragma once + +#include "root.h" +#include "v8/node.h" +#include "napi.h" +#include +#include +#include +#include +#include +#include + +#if OS(WINDOWS) +#include +#endif + +namespace Bun { + +// A module can be either V8 C++ style or NAPI style +using DLModuleRegistration = std::variant; + +// Thread-safe map for tracking dlopen handles to module registrations. +// This allows re-loading the same native module multiple times, matching Node.js behavior. +// +// A single .node file can register multiple modules (both NAPI and V8 C++), so we store +// a vector of registrations per handle. When a native module is loaded for the first time, +// its static constructors run and call node_module_register() or napi_module_register(). +// On subsequent loads, dlopen() returns the same handle but the constructors don't run again. +// We use this map to look up and replay all saved registrations. +class DLHandleMap { +public: +#if OS(WINDOWS) + using DLHandle = HMODULE; +#else + using DLHandle = void*; +#endif + + // Get the singleton instance + static DLHandleMap& singleton() + { + static std::once_flag s_onceFlag; + static DLHandleMap* s_instance = nullptr; + std::call_once(s_onceFlag, [] { + s_instance = new DLHandleMap(); + }); + return *s_instance; + } + + // Add a V8 C++ module registration to the vector for this handle + void add(DLHandle handle, node::node_module* module) + { + ASSERT(handle != nullptr); + ASSERT(module != nullptr); + + WTF::Locker locker { m_lock }; + auto& registrations = m_map.ensure(handle, [] { return WTF::Vector(); }).iterator->value; + registrations.append(DLModuleRegistration(module)); + } + + // Add a NAPI module registration to the vector for this handle + void add(DLHandle handle, napi_module* module) + { + ASSERT(handle != nullptr); + ASSERT(module != nullptr); + + WTF::Locker locker { m_lock }; + auto& registrations = m_map.ensure(handle, [] { return WTF::Vector(); }).iterator->value; + registrations.append(DLModuleRegistration(module)); + } + + // Look up all previously saved module registrations for this handle + std::optional> get(DLHandle handle) + { + ASSERT(handle != nullptr); + + WTF::Locker locker { m_lock }; + + auto it = m_map.find(handle); + if (it == m_map.end()) { + return std::nullopt; + } + + return it->value; + } + +private: + DLHandleMap() = default; + ~DLHandleMap() = default; + + DLHandleMap(const DLHandleMap&) = delete; + DLHandleMap& operator=(const DLHandleMap&) = delete; + + WTF::Lock m_lock; + WTF::HashMap> m_map; +}; + +} // namespace Bun diff --git a/src/bun.js/bindings/ZigGlobalObject.h b/src/bun.js/bindings/ZigGlobalObject.h index ffbc145c0e..98043cb034 100644 --- a/src/bun.js/bindings/ZigGlobalObject.h +++ b/src/bun.js/bindings/ZigGlobalObject.h @@ -40,6 +40,10 @@ class GlobalInternals; } // namespace shim } // namespace v8 +namespace node { +struct node_module; +} // namespace node + #include "root.h" #include "headers-handwritten.h" #include @@ -651,9 +655,18 @@ public: // We will add it to the resulting napi value. void* m_pendingNapiModuleDlopenHandle = nullptr; - // Store the napi module struct to defer calling nm_register_func until after dlopen completes + // Store ALL napi module structs to defer calling nm_register_func until after dlopen completes + // A single .node file can register multiple modules during static constructors + WTF::Vector m_pendingNapiModules; + + // Temporary storage for current NAPI module being executed + // Used by executePendingNapiModule to execute one module at a time std::optional m_pendingNapiModule = {}; + // Store ALL V8 C++ module pointers to defer execution until after dlopen completes + // A single .node file can register multiple V8 modules during static constructors + WTF::Vector m_pendingV8Modules; + JSObject* nodeErrorCache() const { return m_nodeErrorCache.getInitializedOnMainThread(this); } // LazyProperty accessors for stdin/stderr/stdout diff --git a/src/bun.js/bindings/napi.cpp b/src/bun.js/bindings/napi.cpp index 40d595984f..c68618adf3 100644 --- a/src/bun.js/bindings/napi.cpp +++ b/src/bun.js/bindings/napi.cpp @@ -810,9 +810,9 @@ extern "C" void napi_module_register(napi_module* mod) // knows that napi_module_register was attempted globalObject->napiModuleRegisterCallCount++; - // Store the entire module struct to be processed after dlopen completes + // Append to vector to accumulate ALL module registrations during dlopen if (mod && mod->nm_register_func) { - globalObject->m_pendingNapiModule = *mod; + globalObject->m_pendingNapiModules.append(*mod); // Increment the counter to signal that a module registered itself Bun__napi_module_register_count++; } else { diff --git a/src/bun.js/bindings/v8/node.cpp b/src/bun.js/bindings/v8/node.cpp index 111707a6a8..3eb7d3c311 100644 --- a/src/bun.js/bindings/v8/node.cpp +++ b/src/bun.js/bindings/v8/node.cpp @@ -44,6 +44,10 @@ void node_module_register(void* opaque_mod) auto* mod = reinterpret_cast(opaque_mod); auto keyStr = WTF::String::fromUTF8(mod->nm_modname); + + // Append to GlobalObject vector so BunProcess.cpp can save ALL registrations after dlopen completes + globalObject->m_pendingV8Modules.append(mod); + globalObject->napiModuleRegisterCallCount++; JSValue pendingNapiModule = globalObject->m_pendingNapiModuleAndExports[0].get(); JSObject* object = (pendingNapiModule && pendingNapiModule.isObject()) ? pendingNapiModule.getObject() diff --git a/test/js/node/process/dlopen-duplicate-load.test.ts b/test/js/node/process/dlopen-duplicate-load.test.ts new file mode 100644 index 0000000000..5d9d5951d3 --- /dev/null +++ b/test/js/node/process/dlopen-duplicate-load.test.ts @@ -0,0 +1,148 @@ +import { spawnSync } from "bun"; +import { beforeAll, describe, expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDirWithFiles } from "harness"; +import { join } from "path"; + +// This test verifies that Bun can load the same native module multiple times +// Previously, the second load would fail with "symbol 'napi_register_module_v1' not found" +// because static constructors only run once, so the module registration wasn't replayed + +describe("process.dlopen duplicate loads", () => { + let addonPath: string; + + beforeAll(() => { + const addonSource = ` +#include + +namespace demo { + +using v8::Context; +using v8::FunctionCallbackInfo; +using v8::Isolate; +using v8::Local; +using v8::Object; +using v8::String; +using v8::Value; + +void Hello(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + args.GetReturnValue().Set(String::NewFromUtf8(isolate, "world").ToLocalChecked()); +} + +void Initialize(Local exports, + Local module, + Local context, + void* priv) { + NODE_SET_METHOD(exports, "hello", Hello); +} + +} // namespace demo + +NODE_MODULE_CONTEXT_AWARE(addon, demo::Initialize) +`; + + const bindingGyp = ` +{ + "targets": [ + { + "target_name": "addon", + "sources": [ "addon.cpp" ] + } + ] +} +`; + + const dir = tempDirWithFiles("dlopen-duplicate-test", { + "addon.cpp": addonSource, + "binding.gyp": bindingGyp, + "package.json": JSON.stringify({ + name: "test", + version: "1.0.0", + gypfile: true, + scripts: { + install: "node-gyp rebuild", + }, + devDependencies: { + "node-gyp": "^11.2.0", + }, + }), + }); + + // Build the addon + const build = spawnSync({ + cmd: [bunExe(), "install"], + cwd: dir, + env: bunEnv, + stdout: "inherit", + stderr: "inherit", + }); + + if (!build.success) { + throw new Error("Failed to build native addon"); + } + + addonPath = join(dir, "build", "Release", "addon.node"); + }); + + test("should load the same module twice successfully", async () => { + const testScript = ` + // First load + const m1 = { exports: {} }; + process.dlopen(m1, "${addonPath.replace(/\\/g, "\\\\")}"); + console.log("First load: hello exists?", typeof m1.exports.hello === "function"); + + // Second load - this should work now + const m2 = { exports: {} }; + process.dlopen(m2, "${addonPath.replace(/\\/g, "\\\\")}"); + console.log("Second load: hello exists?", typeof m2.exports.hello === "function"); + + // Verify both work + console.log("First module result:", m1.exports.hello()); + console.log("Second module result:", m2.exports.hello()); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", testScript], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stdout).toContain("First load: hello exists? true"); + expect(stdout).toContain("Second load: hello exists? true"); + expect(stdout).toContain("First module result: world"); + expect(stdout).toContain("Second module result: world"); + expect(exitCode).toBe(0); + }); + + test("should load module with different exports objects", async () => { + const testScript = ` + // First load with empty object + const m1 = { exports: {} }; + process.dlopen(m1, "${addonPath.replace(/\\/g, "\\\\")}"); + console.log("m1.exports.hello:", m1.exports.hello()); + + // Second load with different exports object + const m2 = { exports: { initial: true } }; + process.dlopen(m2, "${addonPath.replace(/\\/g, "\\\\")}"); + console.log("m2.exports.initial:", m2.exports.initial); + console.log("m2.exports.hello:", m2.exports.hello()); + `; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", testScript], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stdout).toContain("m1.exports.hello: world"); + expect(stdout).toContain("m2.exports.initial: true"); + expect(stdout).toContain("m2.exports.hello: world"); + expect(exitCode).toBe(0); + }); +});