Skip to content

resolver: fix panic on relative entries in require.resolve options.paths - #33174

Closed
robobun wants to merge 1 commit into
mainfrom
farm/da868934/fix-require-resolve-paths-panic
Closed

robobun wants to merge 1 commit into
mainfrom
farm/da868934/fix-require-resolve-paths-panic

Conversation

@robobun

@robobun robobun commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Any non-absolute entry in require.resolve(id, { paths }) (or Module._resolveFilename(..., { paths })) aborted the process:

require.resolve("lodash", { paths: ["sub"] });
panic: cannot resolve DirInfo for non-absolute path: sub

In Node this is valid: Module._nodeModulePaths runs path.resolve(from) on every entry, so relative paths are resolved against the working directory before the node_modules walk. Bun's resolver handed the raw user string to check_package_path, whose dir_info_cached asserts the directory is absolute.

This was found by fuzzing (the fuzzer passed a function, which toWTFString turns into its source text, another non-absolute string), but plain relative strings hit the same assertion.

The fix resolves each custom path against top_level_dir with abs_buf_checked before the lookup, in the one place that feeds user input into check_package_path. Absolute entries behave as before (join_abs_string_buf keeps the last absolute component). The relative-import branch of the same loop was already safe because check_relative_path does this join internally.

How did you verify your code works?

bun bd test test/js/node/module/node-module-module.test.js passes. The new test spawns a fixture whose cwd is a temp dir containing sub/node_modules/paths-rel-pkg, resolves the package through paths: ["sub"], and requires it. Before the fix the subprocess aborts with the panic above; with USE_SYSTEM_BUN=1 the test fails. The existing options.paths tests in module-resolve-filename-paths.test.js (all absolute entries) still pass.

require.resolve(id, { paths }) and Module._resolveFilename forward the
paths entries to the resolver, which handed them directly to
check_package_path. That function requires an absolute directory, so any
non-absolute entry tripped the "cannot resolve DirInfo for non-absolute
path" assertion and aborted the process.

Resolve each custom path against the top-level directory before the
node_modules walk, matching Node's Module._nodeModulePaths, which calls
path.resolve(from) on every entry. Absolute entries are unchanged.
@github-actions github-actions Bot added the claude label Jul 1, 2026
@robobun

robobun commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:45 AM PT - Jul 1st, 2026

@robobun, your commit 0fbcc5c is building: #67529

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3fc1e13a-62be-47e3-a65d-0d04d8d8febf

📥 Commits

Reviewing files that changed from the base of the PR and between d816daf and 0fbcc5c.

📒 Files selected for processing (2)
  • src/resolver/resolver.rs
  • test/js/node/module/node-module-module.test.js

Walkthrough

The resolver's per-thread scratch buffers gained a custom_dir_path_abs field used to convert custom_dir_paths entries to absolute paths via abs_buf_checked before calling check_package_path, skipping entries that fail conversion. A corresponding test verifies require.resolve() resolves relative paths entries against the process working directory.

Changes

Custom Path Resolution Fix

Layer / File(s) Summary
Absolute path conversion for custom_dir_paths
src/resolver/resolver.rs
Adds custom_dir_path_abs: PathBuffer to Bufs and converts each custom directory path to absolute form via abs_buf_checked before passing it to check_package_path, skipping entries that fail conversion.
Test coverage for require.resolve paths option
test/js/node/module/node-module-module.test.js
Imports tempDir and adds a test that spawns a subprocess in a temporary directory with a nested node_modules package, verifying require.resolve() with options.paths resolves a relative entry against the working directory.

Related PRs: None identified.

Suggested labels: bug, resolver, needs-tests

Suggested reviewers: None identified.

🐰 A path once relative, now stands up straight,
Absolute and certain, it finds its own gate,
A buffer for scratch work, a test in a shell,
The resolver hops on, and all resolves well.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix: handling relative entries in require.resolve options.paths without panicking.
Description check ✅ Passed The description follows the required template and includes both the change summary and verification steps.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. resolver: fix panic on non-absolute require.resolve paths entries #33175 - Nearly identical fix: same abs_buf_checked approach in the same check_package_path loop in resolver.rs to resolve relative require.resolve paths entries to absolute
  2. Fix crash in require.resolve when options.paths contains invalid entries #31566 - Broader fix for the same panic on non-absolute options.paths entries, also making them absolute before check_package_path, plus additional edge-case handling

🤖 Generated with Claude Code

@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.

LGTM — targeted crash fix on a cold resolver path, uses the existing abs_buf_checked helper with a dedicated buffer, and the test follows harness conventions.

Extended reasoning...

Overview

This PR fixes a panic in require.resolve(id, { paths }) when a paths entry is non-absolute. Two files are touched: src/resolver/resolver.rs (adds a new custom_dir_path_abs scratch buffer to Bufs and resolves each user-provided custom path against top_level_dir via abs_buf_checked before handing it to check_package_path) and test/js/node/module/node-module-module.test.js (adds a subprocess test that resolves a package via a relative paths entry).

Security risks

None. The change only normalizes a user-provided directory string to an absolute path before a directory lookup — the same operation the sibling check_relative_path branch already performs internally. No new I/O surface, no auth/crypto/permissions code touched. The _checked variant returns None on buffer overflow, which the loop handles by continueing rather than panicking.

Level of scrutiny

The resolver is core, but this change is confined to the custom_dir_paths.is_some() branch, which is only set for the duration of a require.resolve/_resolveFilename call with an explicit paths option (see BunObject.rs:1434) and is annotated bun_core::hint::cold(). The fix reuses the established fs_ref().abs_buf_checked helper (which joins against top_level_dir, kept in sync with cwd via process.chdir), and join_abs_string_buf preserves absolute inputs by keeping the last absolute component, so existing absolute-path callers are unaffected. The new custom_dir_path_abs buffer is dedicated to this one call site, so there's no aliasing with buffers used inside check_package_path.

Other factors

The bug-hunting system found nothing. The test uses tempDir, bunExe/bunEnv, drains stdout/stderr/exited concurrently, and asserts stdout before exit code per repo conventions. The PR description confirms it fails under USE_SYSTEM_BUN=1 and that the existing absolute-path options.paths tests still pass. No CODEOWNERS entry covers the resolver, and there are no outstanding reviewer comments.

@robobun

robobun commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as a duplicate of #31566, which is the older and more complete fix for this panic.

After the duplicate flag I re-tested this branch and it still panics on an entry that platform::Loose treats as absolute but the host does not. abs_buf_checked joins with Loose, which accepts a Windows drive prefix on every platform, so on Linux such an entry passes through the join unchanged and dir_info_cached trips the same assertion it does on main:

require.resolve("x", { paths: ["C:/foo"] });
// panic: cannot resolve DirInfo for non-absolute path: C:/foo

The relative-specifier loop is reachable the same way, and this PR does not touch it at all:

require.resolve("./x", { paths: ["C:/foo"] });
// panic: cannot resolve DirInfo for non-absolute path: C:/foo/x

#31566 adds a host-native join (abs_buf_checked_native over platform::Auto, which agrees with the bun_paths::is_absolute the assertion uses) and applies it to both loops, so it covers both of these. It is green in CI and is waiting on a maintainer for the two design calls noted in its thread.

@robobun robobun closed this Jul 1, 2026
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.

1 participant