resolver: check union tag before reading cached .entries in dirInfoCachedMaybeLog#30170
Conversation
…nfoCachedMaybeLog EntriesOption is a tagged union of .entries (*DirEntry) and .err (DirEntry.Err). When readDirectory() fails with a non-ENOENT error (EACCES, EMFILE, ...) it stores .err in rfs.entries. If the directory later becomes openable and dirInfoCachedMaybeLog processes it as queue slot [0], the cached .err was read as .entries without a tag check, reinterpreting two anyerror values as a *DirEntry pointer and dereferencing it for .generation. dirInfoForResolution already had the correct guard; apply the same here. When the cached entry is .err, fall through with needs_iter=true so the directory is re-read now that it can be opened.
|
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 (1)
WalkthroughA guard is added in the resolver to only access cached-directory ChangesCached-directory variant guard and regression test
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
|
Updated 12:50 AM PT - May 3rd, 2026
❌ @autofix-ci[bot], your commit befbdc4 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30170That installs a local version of the PR into your bun-30170 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/resolve/resolve.test.ts`:
- Around line 498-505: The test currently assumes the "nobody" user has UID/GID
65534; instead, parse the actual UID and GID from the "/etc/passwd" entry
detected by the hasNobody check and use those values when calling chownSync.
Locate the hasNobody detection and canUseRunuser consts, extract the numeric uid
and gid from the matching passwd line (split on ':' and convert fields 2 and 3),
and propagate those parsed uid/gid into the permission setup (the chownSync
call) rather than using the hard-coded 65534.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06ba1a27-259a-4708-a3ca-a4f796f49cf9
📒 Files selected for processing (2)
src/resolver/resolver.zigtest/js/bun/resolve/resolve.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal tag-check guard matching the existing pattern in dirInfoForResolution, with a solid regression test; prior review nits have been addressed.
Extended reasoning...
Overview
This PR fixes a crash in dirInfoCachedMaybeLog (src/resolver/resolver.zig:2972) where the EntriesOption tagged union was dereferenced as .entries without first checking the active tag. When readDirectory() had previously cached an .err variant (e.g. after EACCES), the two anyerror values were reinterpreted as a *DirEntry pointer and dereferenced — a safety panic in debug, segfault in release. The 7-line Zig change wraps the existing block in if (cached_entry.* == .entries), which is byte-for-byte the same guard already used in dirInfoForResolution at resolver.zig:2265. When the tag is .err, control falls through with needs_iter = true and in_place = null, so the directory is re-iterated fresh now that it's openable — exactly the right behavior.
The accompanying test in test/js/bun/resolve/resolve.test.ts reproduces the crash sequence (chmod 000 → resolve → chmod 755 → resolve) in a subprocess, with sensible platform gating: skipped on Windows, and on Linux-as-root it drops to nobody via runuser since root bypasses DAC. The test uses tempDir with using, restores permissions in a finally so cleanup works even on crash, and follows the repo's stderr/stdout/exitCode assertion ordering.
Security risks
None. This is a defensive null-tag check that turns a crash into a re-iteration. No new inputs, no auth/crypto/permission logic touched.
Level of scrutiny
Low. The Zig change is mechanical and mirrors an existing, proven pattern in the same file. The fallthrough semantics (needs_iter stays true, in_place stays null) are correct by construction — the needs_iter block immediately below handles in_place == null by allocating a fresh dirname-store entry. The test is additive and well-guarded against environments where it can't run.
Other factors
Both prior review comments (my timeout nit and CodeRabbit's hardcoded-uid/gid concern) were addressed in commit 5425fe5 and the threads are resolved. The PR description notes that #29919 independently includes this same guard as part of a larger change; this PR is the minimal targeted fix with a dedicated regression test, which is reasonable to land on its own. The robobun build failures on 5425fe5 are all in scripts/build/ci.ts across every platform shard, which points to transient CI infrastructure rather than this change.
…chedMaybeLog (oven-sh#30170) ## Problem `EntriesOption` is a tagged union: ```zig pub const EntriesOption = union(Tag) { entries: *DirEntry, err: DirEntry.Err, // { original_err: anyerror, canonical_error: anyerror } }; ``` When `readDirectory()` fails with a non-ENOENT error (EACCES, EMFILE, …), `readDirectoryError` stores `.err` in `rfs.entries` (fs.zig:1002). If the directory later becomes openable and `dirInfoCachedMaybeLog` processes it as queue slot [0] (the target path itself, which is never pre-checked against `rfs.entries` during queue construction), it reached: ```zig if (rfs.entries.atIndex(cached_dir_entry_result.index)) |cached_entry| { if (cached_entry.entries.generation >= r.generation) { // no tag check ``` Reading `.entries` while `.err` is active reinterprets the two `anyerror` values as a `*DirEntry` pointer and dereferences it. In debug this is a safety panic; in release it's a segfault at e.g. `0x1D401D401F8`. ## Repro ```js chmodSync(bad, 0o000); try { Bun.resolveSync('./bad/index.js', root); } catch {} // caches .err for 'bad' chmodSync(bad, 0o755); Bun.resolveSync('./bad', root); // dirInfoCached(bad): open OK, cache has .err -> crash ``` Before: ``` panic(main thread): access of union field 'entries' while field 'err' is active resolver.zig:2973:33 in dirInfoCachedMaybeLog ``` ## Fix Add the same `if (cached_entry.* == .entries)` guard that `dirInfoForResolution` already has (resolver.zig:2265). When the cached entry is `.err`, fall through with `needs_iter = true` so the directory is re-iterated now that it can be opened. ## Verification Added a test to `test/js/bun/resolve/resolve.test.ts` that reproduces the exact sequence. Runs as the current user when non-root, or drops to `nobody` via `runuser` when root (since root bypasses DAC). Skipped on Windows. - Without fix: panics with `access of union field 'entries' while field 'err' is active` - With fix: resolves to `bad/index.js` --- Note: oven-sh#29919 independently adds this same guard as part of a larger memory-leak fix; this PR is the minimal targeted crash fix with a dedicated regression test. --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
Problem
EntriesOptionis a tagged union:When
readDirectory()fails with a non-ENOENT error (EACCES, EMFILE, …),readDirectoryErrorstores.errinrfs.entries(fs.zig:1002). If the directory later becomes openable anddirInfoCachedMaybeLogprocesses it as queue slot [0] (the target path itself, which is never pre-checked againstrfs.entriesduring queue construction), it reached:Reading
.entrieswhile.erris active reinterprets the twoanyerrorvalues as a*DirEntrypointer and dereferences it. In debug this is a safety panic; in release it's a segfault at e.g.0x1D401D401F8.Repro
Before:
Fix
Add the same
if (cached_entry.* == .entries)guard thatdirInfoForResolutionalready has (resolver.zig:2265). When the cached entry is.err, fall through withneeds_iter = trueso the directory is re-iterated now that it can be opened.Verification
Added a test to
test/js/bun/resolve/resolve.test.tsthat reproduces the exact sequence. Runs as the current user when non-root, or drops tonobodyviarunuserwhen root (since root bypasses DAC). Skipped on Windows.access of union field 'entries' while field 'err' is activebad/index.jsNote: #29919 independently adds this same guard as part of a larger memory-leak fix; this PR is the minimal targeted crash fix with a dedicated regression test.