build: delay-load node.exe imports so addons work in any Node-API host on Windows - #83
build: delay-load node.exe imports so addons work in any Node-API host on Windows#83hexbinoct wants to merge 3 commits into
Conversation
Sorry, something went wrong.
| return NULL; | ||
|
|
||
| // Prefer libnode.dll to support a Node.js built as a shared library. | ||
| m = GetModuleHandle(TEXT("libnode.dll")); |
There was a problem hiding this comment.
Could this be like node.exe as well?
if (_stricmp(info->szDll, "libnode.dll") != 0)
return NULL;
Sorry, something went wrong.
There was a problem hiding this comment.
Good call, the hook now accepts libnode.dll too, and the addons delay-load both names, with /ignore:4199 so the linker stays quiet about whichever name the import library does not reference (node-gyp ships the same flag).
One note for context: in this repo's own build the import table always says NODE.EXE, because the import library is generated from the merged def whose header is NAME NODE.EXE. So the libnode.dll path only becomes live for addons linked against a shared-library Node build. Seemed worth covering anyway since the hook is meant to be reusable.
Rebuilt and re-ran: 48/48 under node, and bun.exe and deno.exe under their real names still load the addons.
Sorry, something went wrong.
There was a problem hiding this comment.
Would you mind removing the dead code path as well?
Sorry, something went wrong.
There was a problem hiding this comment.
Happy to. Just want to make sure I remove the right thing, since there are two libnode.dll paths in the hook now: the name check added in the last commit (unreachable here because the generated import library always says NODE.EXE), and the GetModuleHandle("libnode.dll") preference from the original version (unreachable because none of the hosts the suite runs against load a shared libnode.dll). Should both go, leaving just the node.exe match and GetModuleHandle(NULL)?
Sorry, something went wrong.
There was a problem hiding this comment.
Please remove L33.
Sorry, something went wrong.
There was a problem hiding this comment.
That path and the m local are gone, so the hook now returns GetModuleHandle(NULL) for both names, exactly like node-gyp's. I kept the libnode.dll name match and the /DELAYLOAD:LIBNODE.DLL + /ignore:4199 pair, since dropping those would leave an addon whose import table names libnode.dll with no redirect at all.
Rebuilt and re-ran on Windows: no LNK4199, 48/48 under node, and bun.exe and deno.exe still load the addons under their real names.
Sorry, something went wrong.
The MSVC-built addons bind their napi_* imports to a module literally named NODE.EXE, so only a host process named node.exe can load them. Delay-load those imports and resolve them to the current process image with a delay-load hook, the same approach node-gyp uses, so any Node-API host executable can load the addons regardless of its name. Signed-off-by: hexbinoct <abubakarm@gmail.com>
Covers addons linked against a shared-library Node import lib. In this repo's own build the generated import library is NAME NODE.EXE, so the new name is inert here; /ignore:4199 silences LNK4199 for whichever of the two names an import table does not reference (node-gyp does the same).
GetModuleHandle("libnode.dll") cannot fire for the hosts the suite runs
against, none of which load a shared libnode.dll, and node-gyp's own
hook resolves every delay-loaded name to the process image the same way.
The libnode.dll name match stays, so an addon whose import table names
it is still redirected rather than left to the loader.
Signed-off-by: hexbinoct <abubakarm@gmail.com>
Reviewers
At least 1 approving review is required to merge this pull request.
On Windows the test addons are linked against an import library generated from the
.def files, whose module name is NODE.EXE. Every .node file therefore carries an
import table that binds the napi_* symbols to a module literally named NODE.EXE.
That works when the host process is node.exe, but any Node-API runtime with a
different executable name cannot load the addons at all: the loader has no module
named NODE.EXE to resolve against. In my testing, process.dlopen of a CTS addon
makes Bun 1.3.14 panic with a segfault and makes Deno 2.9.5 crash with 0xC0000005.
The only workaround was renaming the runtime's exe to node.exe, which is not
something a conformance suite should require. Linux and macOS are unaffected because
their loaders resolve undefined symbols against the host executable directly.
This is the same problem node-gyp solved years ago, and this PR applies the same
standard fix: the node.exe imports become delay-loaded (/DELAYLOAD:NODE.EXE plus
delayimp.lib), and a small delay-load hook (src/win_delay_load_hook.cc, modeled on
node-gyp's win_delay_load_hook.cc) resolves the node.exe module to the current
process image via GetModuleHandle(NULL) at runtime, whatever the executable is
called. The hook also prefers libnode.dll when present, matching node-gyp, so a
shared-library Node build works too. The hook and flags are added inside
add_node_api_cts_addon under if(MSVC), so every addon target gets them and
non-Windows builds are untouched.
Verified on Windows 11 with VS 2022:
(1.3.14) panics in process.dlopen (crash report names NODE.EXE), and under
deno.exe (2.9.5) segfaults with 0xC0000005. Node passes.
their own names, no rename. A deliberate failing assertion added to the test makes
both exit non-zero, so the addon code is genuinely executing.
produced before: bun 40 pass / 7 fail / 1 timeout, deno 32 pass / 14 fail / 2
crashes, with the same per-test verdicts. The remaining failures are runtime
conformance and harness-portability issues unrelated to this change.
npm run node:test and npm run lint all pass on this branch.
One behavior note: with delay-loading, an addon that references a symbol the host
does not export now fails when the symbol is first called rather than at load time.
For Node itself nothing changes, since all suite symbols come from the .def files
that Node exports.
Claude found this, wrote the fix, ran the verification, and drafted this text;
I reviewed both the fix and the text.