Skip to content

bun:ffi: use LoadLibraryExW on Windows so dlopen accepts non-ASCII paths - #33712

Merged
Jarred-Sumner merged 5 commits into
mainfrom
farm/8b8f3a3b/ffi-dlopen-windows-unicode
Jul 8, 2026
Merged

bun:ffi: use LoadLibraryExW on Windows so dlopen accepts non-ASCII paths#33712
Jarred-Sumner merged 5 commits into
mainfrom
farm/8b8f3a3b/ffi-dlopen-windows-unicode

Conversation

@robobun

@robobun robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Repro (Windows)

import { dlopen, FFIType } from "bun:ffi";
import { mkdirSync, copyFileSync } from "node:fs";

mkdirSync("C:\\tmp\\日本語", { recursive: true });
copyFileSync("C:\\Windows\\System32\\version.dll", "C:\\tmp\\日本語\\version.dll");

dlopen("C:\\tmp\\日本語\\version.dll", {
  GetFileVersionInfoSizeW: { args: [FFIType.ptr, FFIType.ptr], returns: FFIType.u32 },
});

Before: throws Failed to open library "C:\tmp\日本語\version.dll": error code 126 (ERROR_MOD_NOT_FOUND). The same DLL loads fine from an ASCII path. Also fails for café and any other non-ASCII component.

Cause

sys::dlopen on Windows called LoadLibraryA with the UTF-8 path bytes. LoadLibraryA decodes its argument as the system ANSI codepage (cp1252 on most Western installs, cp932 on Japanese, etc.), so every non-ASCII UTF-8 byte sequence is misread and the file is not found. This affects any bun:ffi user whose DLL lives under a profile directory with a non-ASCII username, which is routine on non-English Windows.

Fix

Convert the UTF-8 path to UTF-16 via bun_paths::string_paths::to_w_path and call LoadLibraryExW(path, NULL, 0), matching how every other Windows path in bun_sys is handled. dwFlags = 0 preserves the default DLL search order so bare names like "kernel32.dll" keep working.

Verification

On Windows x64 (system ACP cp1252):

$env:USE_SYSTEM_BUN="1"; bun test test/js/bun/ffi/ffi.test.js -t "non-ASCII"
(fail) dlopen accepts non-ASCII library paths on Windows
  error: Failed to open library "...\bun-ffi-café\version.dll": error code 126

bun bd test test/js/bun/ffi/ffi.test.js -t "non-ASCII"
(pass) dlopen accepts non-ASCII library paths on Windows [486ms]

Full ffi.test.js and ffi-error-messages.test.ts pass on Windows with the change (13 pass, 5 skip, 0 fail), including the existing dlopen("kernel32.dll", ...) name-only lookup test.

The test is Windows-only (it.skipIf(!isWindows)); on Linux it is skipped.

robobun added 2 commits July 7, 2026 23:40
bun:ffi dlopen() passed the library path to LoadLibraryA, which decodes
bytes as the system ANSI codepage. Bun hands it UTF-8, so any non-ASCII
byte in the path (a CJK username, an accented project directory) was
mangled and the load failed with ERROR_MOD_NOT_FOUND even though the
file exists.

Convert the path to UTF-16 and call LoadLibraryExW, matching how every
other Windows path in bun_sys is handled.
@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Reproduced on Windows x64 (system ACP cp1252): dlopen fails with error 126 for any non-ASCII path component. Fixed by widening to UTF-16 and calling LoadLibraryExW.

For reference, Node's process.dlopen goes through libuv's uv_dlopen, which calls LoadLibraryExW(path, NULL, LOAD_WITH_ALTERED_SEARCH_PATH). Bun's own process.dlopen already does the same. This PR passes LOAD_WITH_ALTERED_SEARCH_PATH when the path is absolute (MSDN documents the flag as undefined for relative paths) and 0 otherwise, so bare names like "kernel32.dll" keep the standard search order.

Verified on Windows x64: new test fails against the released bun and passes with this build; ffi.test.js + ffi-error-messages.test.ts are 13 pass / 5 skip / 0 fail. The test is skipIf(!isWindows || isFFIUnavailable) so Linux and Windows ARM64 skip it.

CI is red on unrelated lanes only. The src/ change is entirely inside #[cfg(windows)] so it does not compile into Linux binaries. Remaining failures:

No ffi/dlopen failures on any lane. Ready for review.

@github-actions github-actions Bot added the claude label Jul 7, 2026
@robobun

robobun commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 937b29d1-284d-4f55-bf9d-ca4d0a01b5fd

📥 Commits

Reviewing files that changed from the base of the PR and between 1de8bd3 and daadb07.

📒 Files selected for processing (2)
  • src/sys/lib.rs
  • test/js/bun/ffi/ffi.test.js

Walkthrough

The Windows dlopen implementation in src/sys/lib.rs now widens UTF-8 filenames to UTF-16 and calls LoadLibraryExW instead of LoadLibraryA, avoiding ANSI codepage mangling. Documentation comments were updated accordingly, and a new Windows-only test validates non-ASCII DLL path loading.

Changes

Windows dlopen wide-character support

Layer / File(s) Summary
dlopen implementation and docs update
src/sys/lib.rs
Windows dlopen now widens the UTF-8 filename via to_w_path and calls LoadLibraryExW with dwFlags = 0, replacing the prior LoadLibraryA call; DynLib and dlopen doc comments updated to reflect the wide-character API.
Non-ASCII path test coverage
test/js/bun/ffi/ffi.test.js
A new Windows-only, skipped-elsewhere test spawns a subprocess that copies version.dll into non-ASCII-named temp directories, loads it via dlopen, checks exported symbol types, and asserts JSON output and exit code.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant dlopen
  participant to_w_path
  participant LoadLibraryExW

  Caller->>dlopen: dlopen(filename, flags)
  dlopen->>to_w_path: widen UTF-8 filename to UTF-16
  to_w_path-->>dlopen: pooled W buffer
  dlopen->>LoadLibraryExW: call with widened path, dwFlags=0
  LoadLibraryExW-->>dlopen: handle or null
  dlopen-->>Caller: Option<*mut c_void>
Loading

Related issues: None specified.

Related PRs: None specified.

Suggested labels: windows, ffi, bug

Suggested reviewers: None specified.

🐰 A rabbit hops through paths unseen,
Where accents once made codepages keen,
Now UTF-16 wide and true,
Loads the dll, no mangling due,
A tiny fix, a hopeful sheen.


Comment @coderabbitai help to get the list of available commands.

…open

Node's process.dlopen goes through uv_dlopen, which on Windows calls
LoadLibraryExW with LOAD_WITH_ALTERED_SEARCH_PATH so that a DLL's
dependent DLLs are resolved relative to the DLL itself rather than
bun.exe. Bun's own process.dlopen already does the same.

MSDN documents that flag as undefined for relative paths, so only set it
when the path is absolute; bare names like "kernel32.dll" keep the
standard search order.
Comment thread test/js/bun/ffi/ffi.test.js Outdated
robobun added 2 commits July 8, 2026 00:06
bun:ffi dlopen() throws unconditionally when ENABLE_TINYCC is false,
which is the case on Windows ARM64. Hoist the existing isFFIUnavailable
constant and gate the new test on it, matching the convention used by
every other TinyCC-dependent test in this directory.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the ARM64 gating. The encoding fix itself is clearly correct, but adding LOAD_WITH_ALTERED_SEARCH_PATH also changes dependent-DLL resolution for bun:ffi users (swaps the app directory for the DLL's directory in the search order) — well-reasoned and matches libuv, but worth a maintainer confirming that's the intended semantics for bun:ffi.

Extended reasoning...

Overview

Two files: src/sys/lib.rs swaps the Windows branch of sys::dlopen from LoadLibraryA(utf8_bytes) to LoadLibraryExW(widened_path, NULL, flags) using the crate's standard w_path_buffer_pool::get() + to_w_path() pattern; test/js/bun/ffi/ffi.test.js adds a Windows-x64-only subprocess test that copies version.dll into café/日本語 directories and dlopens it. My earlier feedback (gate on isFFIUnavailable so Windows ARM64 skips) was applied in 36b464d — the isFFIUnavailable constant was hoisted and the test now uses it.skipIf(!isWindows || isFFIUnavailable). The helper functions (to_w_path, is_absolute_windows, kernel32::LoadLibraryExW) all exist and are used the same way elsewhere in bun_sys.

Security risks

DLL search order on Windows is a classic hijacking vector. Beyond fixing the encoding bug, this PR adds LOAD_WITH_ALTERED_SEARCH_PATH for absolute paths, which replaces "the directory containing bun.exe" with "the directory containing the loaded DLL" as the first entry in the dependent-DLL search order. That matches libuv's uv_dlopen and Bun's own process.dlopen, and is generally the safer/expected behavior for plugin loading — but it is a user-visible semantic change for any bun:ffi DLL whose transitive dependencies were previously satisfied from next to bun.exe. Other Windows callers of sys::dlopen (tracy.rs, Terminal.rs) are dev-internal and unlikely to be affected.

Level of scrutiny

Medium. The UTF-8 → UTF-16 widening is a mechanical correctness fix with an obvious root cause and a fail-before/pass-after test. But the search-order flag is a deliberate design choice layered on top, affecting how bun:ffi — which loads arbitrary user native code — resolves transitive DLLs. It's well-argued (MSDN says the flag is undefined for relative paths, hence gating on is_absolute_windows; matches Node), but I'd rather a maintainer confirm this is the desired bun:ffi contract than auto-approve a change to Windows library-loading semantics.

Other factors

No bugs found by the bug-hunting pass. The test follows harness conventions (tempDir, bunEnv spread, concurrent pipe drain, combined-object assertion, exitCode asserted alongside output). The PR description documents fail-before/pass-after on Windows x64 and confirms the existing bare-name kernel32.dll lookup test still passes.

@Jarred-Sumner
Jarred-Sumner merged commit d5f3e79 into main Jul 8, 2026
72 of 78 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/8b8f3a3b/ffi-dlopen-windows-unicode branch July 8, 2026 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants