Repository navigation
Conversation
…ng a wrong path The Zig relativePathAndDepth wrote into depth_buf with no bounds check, so a parent-pointer cycle in a corrupted bun.lockb panicked loudly. The Rust port added a MAX_DEPTH guard but handled it by returning the bare "node_modules" path with depth=0, so callers (bun pm ls, the bin linker, Lockfile::eql) would silently operate on the wrong directory. Fail the same way the adjacent path-too-long and unsafe-folder-name checks do: print a malformed-lockfile error and crash.
|
Status: diff is ready; CI blocked on darwin runner availability. Reproduced with a hex-patched Review feedback addressed in 0bc5d81: the sibling out-of-bounds-parent exit now fails loudly too, and the test covers both shapes via CI: 280/286 lanes pass including every Linux, Windows, and FreeBSD lane; |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesLockfile Tree Parent-Chain Validation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 12:35 PM PT - Jun 15th, 2026
❌ @robobun, your commit 243b97a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32306That installs a local version of the PR into your bun-32306 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-tested hardening that matches the existing path_too_long/folder_name_is_safe crash pattern in the same function; the inline notes are non-blocking follow-ups.
Extended reasoning...
Overview
This PR changes one branch in relative_path_and_depth (src/install/lockfile/Tree.rs): when the parent-chain walk hits MAX_DEPTH (only reachable via a corrupted bun.lockb with cyclic tree parents), it now emits an error and calls Global::crash() instead of silently returning the bare "node_modules" path with depth=0. A new test in test/cli/install/bun-lockb.test.ts hex-patches tree[1].parent = 1 and asserts bun pm ls reports the malformed lockfile and exits non-zero.
Security risks
None introduced. The change strictly hardens handling of corrupted on-disk lockfiles — previously a cyclic parent chain produced a fabricated root path that callers (bun pm ls, link_remaining_bins, Lockfile::eql) would act on; now it fails loudly. The happy path is untouched, and MAX_DEPTH * len("node_modules") already exceeds MAX_PATH_BYTES, so no well-formed lockfile can reach this guard.
Level of scrutiny
Low-to-medium. The diff is ~12 lines (mostly comment) replacing a silent early return with Output::err_generic(...) + Global::crash(), identical to the adjacent path_too_long and folder_name_is_safe checks in the same function. No control-flow or data changes outside the corruption branch. Not under CODEOWNERS.
Other factors
Two inline notes were filed, both non-blocking: (1) the sibling loop exit (parent_id >= trees.len()) still returns a truncated path on out-of-bounds parents — that guard is pre-existing and untouched by this PR, so it's a reasonable follow-up rather than a defect in this change; (2) expect(out).toBeDefined() is vacuous but matches three pre-existing tests in the same file. Neither affects correctness of what's shipped here. The PR has a fail-before/pass-after test and CI is building.
The parent-chain walk has a second corruption-only exit: when a non-root tree's parent index is >= trees.len(), the bounds guard drops out of the loop before reaching the root and the path built below is missing every segment above the break. Check parent_id == 0 after the loop and reject the lockfile if not. The test covers both shapes via it.each.
There was a problem hiding this comment.
LGTM — both earlier comments addressed in 0bc5d81; the remaining note is pre-existing and fine as a follow-up.
Extended reasoning...
Overview
Two-file change: src/install/lockfile/Tree.rs replaces the silent early-return in relative_path_and_depth's MAX_DEPTH guard with Output::err_generic + Global::crash(), and adds a post-loop parent_id != 0 check for the out-of-bounds-parent case. test/cli/install/bun-lockb.test.ts adds an it.each test that hex-patches a generated bun.lockb to exercise both corruption shapes and asserts bun pm ls reports "Lockfile is malformed" and exits non-zero.
Security risks
None introduced — this hardens handling of corrupted/tampered binary lockfiles. Previously a cyclic or out-of-bounds tree.parent made the iterator silently return the root node_modules path (or a truncated one) for a nested node, which could cause link_remaining_bins to link into the wrong directory. Now it fails closed with a clear error. The MAX_DEPTH argument is sound: that many node_modules segments alone exceed MAX_PATH_BYTES, so no well-formed lockfile is rejected.
Level of scrutiny
Low-to-medium. The diff is small, mechanical, and exactly mirrors the adjacent path_too_long / folder_name_is_safe checks in the same function (same err_generic + crash() pattern). Behavior only changes for already-corrupt lockfiles that were previously producing silently wrong output; the well-formed path is untouched. No CODEOWNERS apply to these files.
Other factors
I reviewed the first revision and raised two points (out-of-bounds parent exit; vacuous toBeDefined()). Both were addressed in 0bc5d81 exactly as suggested, and the test was extended to cover the second shape. The one remaining inline note (corrupted tree.id field) is explicitly pre-existing — inherited verbatim from Tree.zig, not touched by this diff — and flagged as a non-blocking follow-up, so it doesn't gate approval.
There was a problem hiding this comment.
LGTM — both review comments addressed in 0bc5d81, and the remaining tree.id note is pre-existing and reasonably deferred to a follow-up alongside the Zig change.
Extended reasoning...
Overview
This PR hardens relative_path_and_depth in src/install/lockfile/Tree.rs against two shapes of corrupted bun.lockb tree-parent data: (1) a parent-pointer cycle that hits the MAX_DEPTH guard, and (2) a parent index pointing past trees.len(). Previously the cycle case silently returned the bare "node_modules" path with depth = 0, and the out-of-bounds case fell through with a truncated path — both letting callers (bun pm ls, link_remaining_bins, Lockfile::eql) operate on the wrong directory. The fix replaces both silent exits with Output::err_generic(...) + Global::crash(), matching the adjacent path_too_long and folder_name_is_safe checks in the same function. A new it.each test in bun-lockb.test.ts generates a real two-tree lockfile via folder deps, hex-patches tree[1].parent, and asserts bun pm ls now reports "Lockfile is malformed" with the right reason and exits non-zero for both shapes.
Security risks
None introduced; this is strictly a hardening change. Both new branches are unreachable on a well-formed lockfile (every non-root parent chain terminates at id 0, and MAX_DEPTH segments of node_modules alone overflow MAX_PATH_BYTES), so the only behavior change is for already-corrupted input, where silent-wrong-path becomes a loud error. That reduces the chance of linking bins into the wrong node_modules from a bit-rotted or tampered bun.lockb.
Level of scrutiny
Moderate — install/lockfile is a load-bearing path, but this diff is ~25 lines of Rust confined to two corruption-only guards inside one function, plus an additive test. The change follows the established fail-loud idiom already used three times in the same function body, so there is no new design surface. The new code paths only execute on malformed on-disk data and cannot regress the well-formed case.
Other factors
I reviewed this PR twice previously. The first round flagged the sibling out-of-bounds exit and a vacuous toBeDefined() assertion — both were fixed in 0bc5d81 (the post-loop parent_id != 0 check and the combined expect({out, err}).toEqual(...)). The second round noted a pre-existing tree.id-vs-tree_id gap inherited verbatim from Tree.zig; the author acknowledged it and scoped it to a follow-up that lands alongside the Zig change, which is reasonable since it is not a regression introduced here and the PR title is scoped to .parent. All inline threads are resolved. The only commit since my last review is a CI retrigger with no code changes. Test coverage is solid: both corruption shapes are exercised end-to-end against a real generated lockfile, with sanity assertions on the on-disk layout before patching.
|
Stale PR review: keep open, rework. The defect is real on main: The shape is the problem. The codebase already has a contract for corrupt |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-06-15, 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
relative_path_and_depthinsrc/install/lockfile/Tree.rswalks a tree node's parent chain to build itsnode_modules/...path. The Zig original wrote intodepth_buf[depth_buf_len]with no bounds check, so a parent-pointer cycle in a corruptedbun.lockbpanicked (index out of bounds in safe builds). The Rust port added adepth_buf_len == MAX_DEPTHguard but handled it by returning early with the bare"node_modules"path anddepth = 0:At that point the path-building loop has not run yet, so callers (
bun pm ls,PackageInstaller::link_remaining_bins,Lockfile::eql) silently receive the rootnode_modulespath for a node that is actually nested and proceed to list/link into the wrong directory. A loud crash became silent wrong output.Fix
Report the corruption and crash, matching the adjacent
path_too_longandfolder_name_is_safechecks in the same function.MAX_DEPTHsegments ofnode_modulesalone already overflowMAX_PATH_BYTES, so reaching the guard can only mean the on-disk trees are cyclic; there is no well-formed lockfile this rejects.Repro
Before: exits 0, silently drops
pkg-bfrom the listing.After:
error: Lockfile is malformed (dependency tree has a cycle or exceeds the maximum depth), exit 1.Test
test/cli/install/bun-lockb.test.tsgenerates a two-treebun.lockbvia folder deps (no registry needed), patchestree[1].parent = 1via the<install.lockfile.Tree>array marker, and assertsbun pm lsnow reports the malformed lockfile and exits non-zero.