Repository navigation
Conversation
The early length guard in resolve_maybe_needs_trailing_slash (specifier.length() > MAX_PATH_BYTES * 1.5) built its Msg with Default::default(), which yields Metadata::Build. ResolveMessage's get_code/get_specifier/get_import_kind only populate on Metadata::Resolve, so a specifier one byte past 6144 lost err.code (ERR_UNKNOWN_BUILTIN_MODULE / MODULE_NOT_FOUND), err.specifier, and err.importKind while the message text survived. Attach the same MetadataResolve the sibling fallback already builds.
|
Reproduced with the script in the report, root cause confirmed at Addressed review (201373b, 46cfce6): per-platform test threshold; clamped the stored specifier to Verification: CI: |
WalkthroughLong resolver error paths now clamp UTF-8 specifiers to ChangesResolve Specifier Handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:55 PM PT - Jul 13th, 2026
✅ @robobun, your commit 46cfce61a4cec5c7a7c3727401040064db45b5da passed in 🧪 To try this PR locally: bunx bun-pr 34097That installs a local version of the PR into your bun-34097 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR does not fix #20374 or #33596. Those issues are about long Fixing those would mean either exempting |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the Rust change is a mechanical copy of the sibling fallback at line ~4420 in the same function, and the test covers both sides of the threshold on Linux/macOS. The two inline nits (per-platform threshold in the test, and the pre-existing BabyString u16 wrap at >64 KiB) are worth a follow-up but don't block.
Extended reasoning...
Overview
The PR adds Metadata::Resolve to the Msg built in resolve_maybe_needs_trailing_slash's early ENAMETOOLONG guard (src/jsc/VirtualMachine.rs:4274-4281), so ResolveMessage getters (.code, .specifier, .importKind) populate correctly for specifiers longer than MAX_PATH_BYTES * 1.5. A parameterized test in test/js/bun/resolve/resolve-error.test.ts covers require/import of node: and relative specifiers at lengths 6144/6145/60000.
Security risks
None. This only affects which metadata variant is attached to an already-thrown resolve error. No new input parsing, no allocation size derived from untrusted input beyond what already existed, no auth/crypto/permissions.
Level of scrutiny
Low. The 6-line Rust change is byte-for-byte identical to the existing fallback at lines 4420-4428 in the same function (same printed.clone(), same BabyString::r#in(&printed, specifier_utf8.slice()), same import_kind, same err: ModuleNotFound). This is filling in a metadata field that was previously left at Default::default() — there's no novel logic to reason about.
Other factors
Two nits surfaced by the bug hunter, both non-blocking:
- The test's hardcoded
[6144, 6145, 60000]encodes the Linux threshold; on Windows (threshold 147453) none of these enter the patched arm, and on macOS (threshold 1536) all three do so there's no below-threshold control. The fix itself is platform-agnostic and is exercised by Linux + macOS CI, so this is a coverage gap rather than a correctness risk. BabyString::r#intruncates length to u16, so specifiers >65535 bytes would wrap. This is a pre-existing property ofBabyString(the sibling fallback uses the same call), not introduced here, and >64 KiB import specifiers are pathological. Pre-PR behavior at those lengths was also wrong (.code === undefined), so this isn't a regression.
The PR includes before/after evidence and the test is confirmed to fail with USE_SYSTEM_BUN=1 and pass with the change on Linux.
…shold BabyString packs a u16 length, so the new ENAMETOOLONG arm wrapped the stored specifier length to 0/1 bytes for specifiers > 65535 bytes, which flipped .code back to MODULE_NOT_FOUND. Clamp to u16::MAX so the stored prefix always carries enough to classify .code. Derive the test's threshold per platform (4096*1.5 Linux, 1024*1.5 macOS, 98302*1.5 Windows) and add a 200000-byte case that exercises the clamp.
Apply the same u16::MAX specifier clamp to the post-_resolve fallback in resolve_maybe_needs_trailing_slash, which is reachable with unbounded specifiers via Bun.resolveSync/Bun.resolve (IS_A_FILE_PATH=false skips the length guard) and via require()/import() on Windows where the guard threshold is 147453. Also mirror both fixes (Metadata::Resolve + clamp) in the near-duplicate resolve_hook in jsc_hooks.rs so the two implementations stay in sync. Add a Bun.resolveSync case to the parameterized long-specifier test.
There was a problem hiding this comment.
LGTM — all prior feedback addressed in 201373b and 46cfce6; the four BabyString::r#in sites are now consistent.
Checked that the clamped prefix is still locatable by index_of inside printed (byte-level search, and data.text is a clone so the stored offset stays valid). The Bun.resolveSync test case now covers the IS_A_FILE_PATH=false path where the length guard is bypassed, and the 200000-byte case exercises the clamp on both the ENAMETOOLONG arm and the post-_resolve fallback. Also confirmed the byte-boundary clamp cannot panic on non-ASCII input since r#in and slice operate on &[u8], not str.
Extended reasoning...
Overview
Attaches Metadata::Resolve to the ENAMETOOLONG early-return in resolve_maybe_needs_trailing_slash so the resulting ResolveMessage exposes .code/.specifier/.importKind for specifiers longer than MAX_PATH_BYTES * 1.5. Adds a u16::MAX clamp before BabyString::r#in at all four call sites (two in VirtualMachine.rs, two in the near-duplicate jsc_hooks.rs::resolve_hook) to prevent length wraparound. ~30 lines of Rust across four near-identical hunks plus a ~75-line parameterized test.
Security risks
None. This only changes which metadata variant is attached to an error object on an already-failing resolve path; it does not alter what resolves or how. The clamp narrows a slice, so no new OOB surface.
Level of scrutiny
Low-to-medium. The Rust change is mechanical — it copies the exact Metadata::Resolve construction from the pre-existing sibling fallback ~150 lines below, and the clamp is a defensive .min() on a byte slice. The jsc_hooks.rs copy has zero live callers today (kept in sync for the crate-layering migration). No control-flow, allocation, or lifetime changes; printed.clone() matches the existing pattern so the BabyString offset computed against &printed is valid against the stored data.text.
Other factors
This PR has been through two rounds of my review. All four findings (per-platform test threshold, u16 wrap, sibling-fallback clamp, jsc_hooks.rs mirror) were addressed with follow-up commits and verified by the author at 65535/65536/65537/70000/200000. The bug-hunting pass on 46cfce6 raised no findings; one candidate (lone-surrogate panic in BabyString::r#in) was examined and ruled out — r#in operates on &[u8] via index_of, not str indexing. Test passes on the debug build and fails with USE_SYSTEM_BUN=1 per the author's evidence.
…s for dead fields (#39327) ### Problem - `LoaderHooks` (src/jsc/ModuleLoader.rs) has a `resolve` field that nothing reads. - Its only dispatch site, `module_loader::resolve_maybe_needs_trailing_slash`, was deleted in #35002; the field and the body it pointed at survived. - The body installed for it, `resolve_hook` plus its inner `resolve` in src/runtime/jsc_hooks.rs (about 470 lines), is a second copy of the live resolver, `VirtualMachine::resolve_maybe_needs_trailing_slash` / `_resolve` in src/jsc/VirtualMachine.rs. Every real caller uses the live one: `Bun.resolveSync` and `Bun__resolveSync*` (src/runtime/api/BunObject.rs), the ESM loader's `Zig__GlobalObject__resolve` (src/jsc/JSGlobalObject.rs), and `Module._findPath` (src/jsc/NodeModuleModule.rs). - The two copies had already drifted and the dead one keeps attracting patches: #37157 added the `data:` ENAMETOOLONG exemption to the live copy only; #34097 and #35703 (open) patch both copies; #39316 was first attempted against the dead copy, where it had no effect. - rustc's `dead_code` lint cannot catch this shape: the field is `pub`, and the body is referenced by the `__BUN_LOADER_HOOKS` static initializer. ### Fix - Delete the `resolve` field of `LoaderHooks`, `resolve_hook`, its inner `resolve`, and the helpers only they used (`EVAL_SUFFIX` / `STDIN_SUFFIX` / `normalize_specifier_for_resolution` / `normalize_source` in jsc_hooks.rs), plus the `resolve: resolve_hook` entry in the static. - Delete `src/bundler_jsc/PluginRunner.rs`, a re-export module whose only user was the dead body, and make the function it re-exported (`plugin_runner_on_resolve_jsc`) `pub(crate)`, since its remaining caller is in the same crate. - No behavior change: the deleted code was unreachable, and the live resolver in VirtualMachine.rs is untouched (the alias lookups there are what #39316 changes). - Test: `test/internal/source-lints/dispatch-hook-tables.test.ts` finds every `safe static __BUN_*: XHooks;` table (`LoaderHooks`, `RuntimeHooks`, `SqlRuntimeHooks`, 48 fields) and fails on any field with no `(hooks.field)(..)` call site. On main it fails naming exactly `LoaderHooks ... resolve`; with this diff it passes. It also covers the next field that loses its dispatch site in any of the three tables. - Call sites are attributed per table: only files that call the table's accessor (`loader_hooks()`, `runtime_hooks()`, `hooks()`) or name its struct in code count, and a site in a file that can reach two tables sharing the field name counts for neither (so `RuntimeHooks::timer_insert` cannot vouch for `SqlRuntimeHooks::timer_insert`). Full-line `//` comments are blanked before matching. - Verified: `bun bd` builds; `bun bd test` on test/js/bun/resolve/{resolve,resolve-error,require,import-query,import-meta,import-meta-resolve,bun-main-entry-point}, test/js/node/module/node-module-module.test.js, module-resolve-filename-paths.test.js, test/js/node/missing-module.test.js, and test/js/bun/plugin (220 + 44 pass); `bun test test/internal/source-lints/` (all 25 lints pass, the existing dead-code inventories are unaffected); `cargo fmt --check` clean. ### Background - Crate tiers: `bun_jsc` (module loader, VM) sits below `bun_runtime` (resolver plumbing, plugins, blob registry) in the crate graph, so `bun_jsc` cannot call `bun_runtime` functions directly. - Hook table: the way around that. The lower crate declares a struct of fn pointers (`LoaderHooks`, `RuntimeHooks`, `SqlRuntimeHooks`) and an `extern "Rust" { safe static __BUN_*_HOOKS: ... }`; `bun_runtime` defines that `#[no_mangle]` static with one body per field; the lower crate calls a field as `(hooks.field)(..)`. A field is live only if such a call exists somewhere. - The live resolver can sit in `bun_jsc` because the two `bun_runtime` pieces it needs are already reachable from there: the blob URL registry through `RuntimeHooks::has_blob_url`, and the plugin `onResolve` runner through `plugin_runner_on_resolve_jsc`, which was moved down into `bun_jsc`. That leaves nothing for a `LoaderHooks::resolve` route to do. <details> <summary>Evidence the field is dead</summary> ``` $ grep -rn "hooks\.resolve)" src # no dispatch site $ grep -rn "loader_hooks()" src # all in src/jsc/ModuleLoader.rs: transpile_source_code, # fetch_builtin_module, transpile_file, get_hardcoded_module, # resolve_embedded_node_file, transpile_virtual_module $ git log --oneline -S "(hooks.resolve)" -- src 85ddc95 Remove ~39k lines of dead Rust across the workspace (#35002) 23427db Rewrite Bun in Rust (#30412) ``` Lint output with main's versions of the five src files checked out: ``` error: LoaderHooks (src/jsc/ModuleLoader.rs) has hook fields that nothing dispatches: resolve (fail) every field of LoaderHooks is dispatched (pass) every field of RuntimeHooks is dispatched (pass) every field of SqlRuntimeHooks is dispatched ``` Not changed here: `bun_bundler_jsc`'s Cargo.toml still lists `bun_resolver`, which this crate no longer uses; its manifest already lists a dozen other crates its sources do not reference, so that is left for a separate manifest cleanup. `VirtualMachine.entry_point` in bun_jsc (read only by the `bun:main` branch of `_resolve`, which the alias table short-circuits before it is reached) looks similarly vestigial but sits in the function #39316 edits, so it is also left alone. </details> --------- Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-14, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
A module specifier one byte past
MAX_PATH_BYTES * 1.5(6144 on Linux) produced aResolveMessagewitherr.code === undefined,err.specifier === "", anderr.importKind === "", while the message text itself was intact. Node.js keepsERR_UNKNOWN_BUILTIN_MODULE/MODULE_NOT_FOUNDat any length.Before:
After:
Error-handling code keyed on
err.code(optional-dependency probes, builtin-vs-missing-dep fallbacks) would silently misclassify any >6 KiB specifier.Why
resolve_maybe_needs_trailing_slashhas an early length guard that short-circuits to anENAMETOOLONGResolveMessage. That arm built itsMsgwith..Default::default(), which yieldsMetadata::Build.ResolveMessage::get_code/get_specifier/get_import_kindonly populate onMetadata::Resolve, so all three getters fell through to their default branch.The fix attaches the same
Metadata::Resolve(MetadataResolve { specifier, import_kind, err })that the sibling resolve-error fallback a few lines below (and the one injsc_hooks.rs) already builds.Test
Added a parameterized test in
test/js/bun/resolve/resolve-error.test.tsthat exercises both sides of the threshold (6144, 6145, 60000) acrossrequire()of anode:specifier,require()of a relative specifier, and dynamicimport().Fails on main with
USE_SYSTEM_BUN=1for 6145/60000, passes with this change.no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/resolve/resolve-error.test.ts