Repository navigation
Conversation
require(), require.resolve(), and Module._resolveFilename() accept an options.paths array of directories to search. Entries that were not absolute paths were passed as-is into the directory cache, which asserts that its input is absolute: panic: cannot resolve DirInfo for non-absolute path: relative Join non-absolute entries against the top-level directory before using them, matching Node, which resolves options.paths entries with path.resolve() and therefore treats relative entries as relative to the current working directory.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Confirmed, this is the same panic and the same fix idea as #31566: absolutize non-absolute #31566 is further along (it covers both |
WalkthroughModifies the resolver's package path resolution to normalize non-absolute entries from ChangesRelative paths resolution
Related PRs: None identified. Suggested labels: bug, resolver, needs-tests-pass Suggested reviewers: None identified. 🥕 A path once bare, now anchored true, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/resolver/resolver.rs (1)
2097-2109: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse
abs_buf_checkedforoptions.pathsentries. Long JS-controlled entries can still take the unchecked join path and panic on overflow; the checked helper already returnsNoneinstead of crashing resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/resolver/resolver.rs` around lines 2097 - 2109, The custom paths handling in resolver resolution should use the checked absolute-path helper instead of the unchecked join path. Update the logic in Resolver::check_package_path processing self.custom_dir_paths so JS-controlled options.paths entries go through abs_buf_checked, preserving the existing cwd-relative behavior but returning None on overflow rather than panicking. Ensure the change is applied in the custom_path/custom_dir branch before calling check_package_path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/resolver/resolver.rs`:
- Around line 2097-2109: The custom paths handling in resolver resolution should
use the checked absolute-path helper instead of the unchecked join path. Update
the logic in Resolver::check_package_path processing self.custom_dir_paths so
JS-controlled options.paths entries go through abs_buf_checked, preserving the
existing cwd-relative behavior but returning None on overflow rather than
panicking. Ensure the change is applied in the custom_path/custom_dir branch
before calling check_package_path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3d760f8-3042-4196-b3c3-328d18121ba9
📒 Files selected for processing (2)
src/resolver/resolver.rstest/js/bun/resolve/require.test.ts
| let custom_dir = if bun_paths::is_absolute(custom_utf8.slice()) { | ||
| custom_utf8.slice() | ||
| } else { | ||
| self.fs_ref() | ||
| .abs_buf(&[custom_utf8.slice()], bufs!(custom_dir_path_abs)) | ||
| }; |
There was a problem hiding this comment.
🔴 This trades one user-reachable panic for two others on the same input surface. (1) abs_buf is the unchecked variant — it normalizes into the fixed-size custom_dir_path_abs PathBuffer (~4096 bytes), so require.resolve('x', { paths: ['a'.repeat(1<<20)] }) overflows and panics; the sibling check_relative_path loop the PR description cites as precedent uses abs_buf_checked for exactly this reason (see the doc comment at resolve_path.rs:1697). (2) The gate uses bun_paths::is_absolute (host-native) but abs_buf joins under Platform::Loose, which treats [A-Z]:/... as absolute on every host — so on Linux/macOS, require.resolve('x', { paths: ['C:/foo'] }) has abs_buf return "C:/foo" unchanged and dir_info_cached's assert (resolver.rs:4204) fires with the exact panic this PR set out to fix. Suggested fix: use abs_buf_checked (continue on None) and additionally verify the result is native-absolute before handing it to check_package_path.
Extended reasoning...
What the bug is
The PR replaces the dir_info_cached "cannot resolve DirInfo for non-absolute path" panic with a call to self.fs_ref().abs_buf(...) for non-absolute options.paths entries. Two distinct panics remain reachable from the same user-controlled input surface:
-
Long input →
abs_bufbuffer overflow.abs_buf(src/resolver/lib.rs:347-350) wrapsjoin_abs_string_buf::<platform::Loose>, the unchecked variant._join_abs_string_bufheap-allocates aJoinScratchfor the intermediate concatenation, but the finalnormalize_string_bufat resolve_path.rs:1850-1852 writes into the caller-suppliedbuf— herebufs!(custom_dir_path_abs), a fixed-sizePathBufferofPATH_MAX_BYTES(~4096 on POSIX). A relative entry longer than that (no..segments to shrink it) overflows the output slice and panics on the Rust bounds check, in both debug and release. -
Windows-style path on POSIX → original
dir_info_cachedassert. The gate at resolver.rs:2103 usesbun_paths::is_absolute, which oncfg(not(windows))is exactlyp[0] == b'/'(bun_core/lib.rs:317-321). Butabs_bufjoins underPlatform::Loose, whoseis_absolutedelegates tois_absolute_windows_tand treatsX:/.../X:\\...as absolute on every host. When those two predicates disagree,abs_bufdiscardstop_level_dirand returns the input unchanged — which then fails the native-absolute assert downstream.
Step-by-step proof
Case 1 — require.resolve('pkg', { paths: ['a'.repeat(1<<20)] }):
custom_dir_pathsis populated directly from JS at BunObject.rs:1398-1434 with no per-entry length clamping.- resolver.rs:2103:
bun_paths::is_absolute(b"aaa...")→'a' != '/'→ false → else branch. - resolver.rs:2106-2107:
abs_buf(&[b"aaa..."], bufs!(custom_dir_path_abs))→_join_abs_string_buf::<Loose>.JoinScratch::init(resolve_path.rs:1669-1677) heap-allocatestemp_buflarge enough to hold cwd + the 1 MiB segment, so the concat step succeeds. - resolve_path.rs:1848-1853:
buf[..leading_len].copy_from_slice(...)thennormalize_string_buf(&temp_buf[..], &mut buf[leading_len..]).bufis ~4096 bytes; the normalized output is ~1 MiB → slice bounds panic. - The doc comment on
join_abs_string_buf_checked(resolve_path.rs:1697-1701) explicitly says: "Use this whenpartsmay contain user-controlled input of arbitrary length." The siblingcheck_relative_pathloop over the samecustom_dir_pathsinput (resolver.rs:2136-2141) already usesabs_buf_checkedandcontinues onNone— the PR description cites that loop as its model but then uses the unchecked variant.
Case 2 — require.resolve('pkg', { paths: ['C:/foo'] }) on Linux/macOS:
- resolver.rs:2103:
bun_paths::is_absolute(b"C:/foo")→is_absolute_nativeon POSIX →'C' != '/'→ false → else branch. abs_buf→_join_abs_string_buf::<Loose>(top_level_dir, buf, &[b"C:/foo"]). At resolve_path.rs:1748,P::P == Windows || (cfg!(windows) && Loose)is false on POSIX+Loose, so it does not dispatch to the Windows joiner.- resolve_path.rs:1796:
Platform::Loose.is_absolute(b"C:/foo")→is_absolute_windows_t(paths/lib.rs:77-93):len >= 3 && p[1] == ':' && p[2] == '/'→ true. Socwdis replaced withb"C:/foo",top_level_diris discarded, andpartsbecomes empty. - resolve_path.rs:1834:
Platform::Loose.leading_separator_indextries the Windows arm first (line 1322). Lines 1299-1304:'C'is in'A'..='Z',p[1] == ':',p[2] == '/'→Some(2).leading_len = 3,leading_buf = b"C:/"; normalized tail is"foo". Result:b"C:/foo"— first byte'C', not'/'. check_package_path(b"C:/foo", ...)(resolver.rs:2265) →dir_info_cached(b"C:/foo"). Not"."/"./";len < MAX_PATH_BYTES; the#[cfg(windows)]normalization block is skipped on POSIX; then resolver.rs:4204:assert!(bun_paths::is_absolute(b"C:/foo"))→'C' != '/'→ panic with the exact message this PR set out to fix. This is a plainassert!, so release builds abort.- (Lowercase drive letters happen to escape:
is_absolute_windows_thas no alphabetic gate socwdis still replaced, butleading_separator_index's Windows arm gates on'A'..='Z', so lowercase falls through to theleading_buf[0] = b'/'fallback at resolve_path.rs:1843 and produces a/-prefixed result. Only[A-Z]:/...and[A-Z]:\\...hit the panic — but that is the common form.)
Why existing code doesn't prevent it
Nothing upstream validates options.paths entries for length or shape — BunObject.rs stores the raw BunString slice directly. The new gate checks native absoluteness, but the join it guards uses a different absoluteness predicate (Loose), and the buffer it writes into does not grow. The downstream dir_info_cached assert uses the native predicate again, so anything abs_buf returns that isn't native-absolute still trips it.
Impact
Both are user-reachable process aborts from plain JavaScript on the exact input class the PR title claims to fix ("paths entries are not absolute paths" — neither 'a'.repeat(1<<20) nor 'C:/foo' is an absolute path on POSIX). Per the repo's guidance, "a panic on user input is a DoS" and "fix the whole class in the same PR".
How to fix
Match the sibling loop: use abs_buf_checked and continue on None (handles case 1). Then, because abs_buf's Loose semantics can still hand back a non-native-absolute result, either gate on Platform::Loose.is_absolute() so the predicate matches what abs_buf will do, or — simpler and more robust — check bun_paths::is_absolute(custom_dir) on the result and continue if it's still not native-absolute before calling check_package_path (handles case 2). Both are one-line changes; worth adding ['a'.repeat(1<<20)] and ['C:/foo'] to the pathsValues array in the new test.
What does this PR do?
Fixes a panic in the module resolver when the
pathsoption ofrequire(),require.resolve(), orModule._resolveFilename()contains an entry that is not an absolute path:Any non-absolute entry hits this: a relative directory name, an empty string, or a non-string value (the array elements are stringified, so
paths: [2.2250738585072014e-308]becomes the "path""2.2250738585072014e-308", which is how the fuzzer found it). The assert is a plainassert!, so release builds abort too.How did you verify your code works?
The
custom_dir_pathsloop inresolve_without_symlinkshanded the raw user string tocheck_package_path, which requires an absolute directory (dir_info_cachedasserts it). That assert is a real invariant of the directory cache, so it stays. Instead, non-absolute entries are now joined against the top-level directory (cwd) before use. That is what Node does:Module._nodeModulePathsruns eachoptions.pathsentry throughpath.resolve(), so relative entries mean "relative to the current working directory". The sibling loop for relative specifiers already behaved this way viaabs_buf_checked.With this change,
require.resolve("pkg", { paths: ["vendor"] })resolves through$CWD/vendor/node_moduleslike Node instead of panicking, and garbage entries produce a normalMODULE_NOT_FOUNDerror.Added two tests to
test/js/bun/resolve/require.test.ts. Both fail with the panic above on an unfixed build (checked against the release build viaUSE_SYSTEM_BUN=1) and pass with this change:pathsentries (numbers,"relative","./relative","") throwMODULE_NOT_FOUNDacrossrequire,require.resolve, andModule._resolveFilenamepathsentry resolves the same module as its absolute spelling