Repository navigation
Conversation
…lly erroring When running in sandboxed environments (e.g., Landlock), ancestor directories like / or /home may not be readable even though getcwd() succeeds and the CWD is fully accessible. Previously, the resolver's directory tree walk treated EACCES on any ancestor directory as fatal, returning null and causing "CouldntReadCurrentDirectory". Now, EACCES is handled by caching the directory as not found and continuing the walk, allowing bun to function in restricted filesystem environments. Closes #28220 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis PR fixes bun's handling of inaccessible ancestor directories during path resolution. Instead of fatally erroring on EACCES when opening parent directories, the resolver now gracefully skips unreadable directories and continues traversing. A regression test validates the fix in a Landlock sandbox environment. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/resolver/resolver.zig`:
- Around line 2840-2847: The EACCES branch is currently treating permission
errors on queue_top as "not found" unconditionally; change it to
preserve/propagate EACCES when queue_top is the final requested directory. In
the error.EACCES / error.AccessDenied arm, check the remaining queue length
(e.g., queue_slice.len or equivalent) and only run the cache-and-skip logic
(rfs.entries.getOrPut(queue_top.unsafe_path),
r.dir_cache.markNotFound(queue_top.result), rfs.entries.markNotFound(...), break
:open_dir null) when there are deeper entries left in the queue; otherwise
return or propagate the original EACCES (preserve the old fatal/error path)
instead of caching a miss.
In `@test/regression/issue/028220.test.ts`:
- Around line 101-127: The suite setup currently lives in the standalone test
"compile landlock helper" which initializes helperPath and landlockSupported;
move this logic into a proper beforeAll hook or, preferably, inline the
compile-and-check steps at the start of each behavioral test so no test depends
on another. Specifically, take the body of test("compile landlock helper")
(mkdirSync, writeFileSync of LANDLOCK_HELPER_SRC, the Bun.spawnSync gcc compile,
expect checks, and the landlock check that sets landlockSupported) and either
(A) place it inside a beforeAll block that sets helperPath and landlockSupported
for the suite, or (B) copy the same steps into each test that needs them so each
test independently creates the temp dir, compiles via Bun.spawnSync, asserts
exitCode/existsSync, runs the helper to detect LANDLOCK_UNSUPPORTED, and
sets/uses landlockSupported locally.
- Around line 92-96: Replace hard-coded paths /home and /tmp with a
harness-provided temporary directory: change the TEST_BASE constant (currently
"/home/bun-test-user/project") to be derived from the test harness tempDir and
use that tempDir when creating directories (the mkdirSync calls around the
TEST_BASE usage and the similar calls at lines 103-105). Ensure you call the
harness tempDir factory once, create the nested project path under it, and
update any assertions/cleanup to use that temp path so ancestors remain outside
the Landlock allow list while avoiding fixed global paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 18ac231a-5105-4123-93e1-2a8c6ec11438
📒 Files selected for processing (2)
src/resolver/resolver.zigtest/regression/issue/028220.test.ts
| error.EACCES, error.AccessDenied => { | ||
| // In sandboxed environments (e.g., Landlock), ancestor directories | ||
| // may not be readable even though the CWD is accessible. Cache as | ||
| // not found and skip this directory rather than treating it as fatal. | ||
| const cached_dir_entry_result = rfs.entries.getOrPut(queue_top.unsafe_path) catch unreachable; | ||
| r.dir_cache.markNotFound(queue_top.result); | ||
| rfs.entries.markNotFound(cached_dir_entry_result); | ||
| break :open_dir null; |
There was a problem hiding this comment.
Preserve EACCES on the requested directory itself.
This branch also runs on the final queue_top entry, so a direct permission failure on the requested directory now gets cached as a miss and silently treated like “not found.” The skip should only apply while there are deeper entries left in queue_slice; otherwise keep the old fatal/error path.
💡 Suggested guard
- error.EACCES, error.AccessDenied => {
+ error.EACCES, error.AccessDenied => {
+ if (queue_slice.len == 0) {
+ const cached_dir_entry_result = rfs.entries.getOrPut(queue_top.unsafe_path) catch unreachable;
+ r.dir_cache.markNotFound(queue_top.result);
+ rfs.entries.markNotFound(cached_dir_entry_result);
+ if (comptime enable_logging) {
+ r.log.addErrorFmt(
+ null,
+ logger.Loc{},
+ r.allocator,
+ "Cannot read directory \"{s}\": {s}",
+ .{ queue_top.unsafe_path, `@errorName`(err) },
+ ) catch {};
+ }
+ return null;
+ }
// In sandboxed environments (e.g., Landlock), ancestor directories
// may not be readable even though the CWD is accessible. Cache as
// not found and skip this directory rather than treating it as fatal.
const cached_dir_entry_result = rfs.entries.getOrPut(queue_top.unsafe_path) catch unreachable;
r.dir_cache.markNotFound(queue_top.result);
rfs.entries.markNotFound(cached_dir_entry_result);
break :open_dir null;
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/resolver/resolver.zig` around lines 2840 - 2847, The EACCES branch is
currently treating permission errors on queue_top as "not found"
unconditionally; change it to preserve/propagate EACCES when queue_top is the
final requested directory. In the error.EACCES / error.AccessDenied arm, check
the remaining queue length (e.g., queue_slice.len or equivalent) and only run
the cache-and-skip logic (rfs.entries.getOrPut(queue_top.unsafe_path),
r.dir_cache.markNotFound(queue_top.result), rfs.entries.markNotFound(...), break
:open_dir null) when there are deeper entries left in the queue; otherwise
return or propagate the original EACCES (preserve the old fatal/error path)
instead of caching a miss.
| // Use a path under /home where intermediate ancestors (/home, /home/bun-test-user) | ||
| // are NOT in the Landlock allow list. This triggers the bug because the resolver | ||
| // walks up to / and tries to openat each ancestor directory. | ||
| const TEST_BASE = "/home/bun-test-user/project"; | ||
|
|
There was a problem hiding this comment.
Use harness temp dirs instead of fixed /home and /tmp paths.
mkdirSync("/home/bun-test-user/project", ...) will fail on many non-root Linux environments, and both globals are shared across reruns/workers. A tempDir-backed sandbox still reproduces the bug because its ancestors remain outside the Landlock allow list.
As per coding guidelines, "Use tempDir from harness to create temporary directories; do not use tmpdirSync or fs.mkdtempSync."
Also applies to: 103-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/028220.test.ts` around lines 92 - 96, Replace
hard-coded paths /home and /tmp with a harness-provided temporary directory:
change the TEST_BASE constant (currently "/home/bun-test-user/project") to be
derived from the test harness tempDir and use that tempDir when creating
directories (the mkdirSync calls around the TEST_BASE usage and the similar
calls at lines 103-105). Ensure you call the harness tempDir factory once,
create the nested project path under it, and update any assertions/cleanup to
use that temp path so ancestors remain outside the Landlock allow list while
avoiding fixed global paths.
| // Compile the Landlock helper once before all tests | ||
| test("compile landlock helper", () => { | ||
| mkdirSync("/tmp/landlock-helper", { recursive: true }); | ||
| const srcPath = "/tmp/landlock-helper/landlock_sandbox.c"; | ||
| helperPath = "/tmp/landlock-helper/landlock_sandbox"; | ||
|
|
||
| writeFileSync(srcPath, LANDLOCK_HELPER_SRC); | ||
|
|
||
| const result = Bun.spawnSync({ | ||
| cmd: ["gcc", "-o", helperPath, srcPath], | ||
| env: bunEnv, | ||
| }); | ||
| expect(result.exitCode).toBe(0); | ||
| expect(existsSync(helperPath)).toBe(true); | ||
|
|
||
| // Check if Landlock is supported on this kernel | ||
| mkdirSync(TEST_BASE, { recursive: true }); | ||
| writeFileSync(join(TEST_BASE, "check.js"), "console.log('ok');\n"); | ||
| const check = Bun.spawnSync({ | ||
| cmd: [helperPath, TEST_BASE, dirname(bunExe()), bunExe(), "-e", "console.log('test')"], | ||
| env: bunEnv, | ||
| cwd: TEST_BASE, | ||
| }); | ||
| if (check.stdout.toString().includes("LANDLOCK_UNSUPPORTED")) { | ||
| landlockSupported = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Don't use a separate test() as suite setup.
helperPath and landlockSupported are only initialized by "compile landlock helper". Running either behavioral case with a name filter skips that setup and leaves the suite in an invalid state. Inline the small setup per test, or at minimum move it to beforeAll.
Based on learnings, "In oven-sh/bun regression tests, prefer explicit per-test setup and per-test assertions (including repeated tempDir, Bun.spawn, bunExe/bunEnv, and checks of stdout, stderr, and exitCode) rather than extracting shared helpers."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/regression/issue/028220.test.ts` around lines 101 - 127, The suite setup
currently lives in the standalone test "compile landlock helper" which
initializes helperPath and landlockSupported; move this logic into a proper
beforeAll hook or, preferably, inline the compile-and-check steps at the start
of each behavioral test so no test depends on another. Specifically, take the
body of test("compile landlock helper") (mkdirSync, writeFileSync of
LANDLOCK_HELPER_SRC, the Bun.spawnSync gcc compile, expect checks, and the
landlock check that sets landlockSupported) and either (A) place it inside a
beforeAll block that sets helperPath and landlockSupported for the suite, or (B)
copy the same steps into each test that needs them so each test independently
creates the temp dir, compiles via Bun.spawnSync, asserts exitCode/existsSync,
runs the helper to detect LANDLOCK_UNSUPPORTED, and sets/uses landlockSupported
locally.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| error.EACCES, error.AccessDenied => { | ||
| // In sandboxed environments (e.g., Landlock), ancestor directories | ||
| // may not be readable even though the CWD is accessible. Cache as | ||
| // not found and skip this directory rather than treating it as fatal. | ||
| const cached_dir_entry_result = rfs.entries.getOrPut(queue_top.unsafe_path) catch unreachable; | ||
| r.dir_cache.markNotFound(queue_top.result); | ||
| rfs.entries.markNotFound(cached_dir_entry_result); | ||
| break :open_dir null; | ||
| }, |
There was a problem hiding this comment.
🟡 defer top_parent = queue_top.result at line 2797 executes even when an EACCES directory is skipped via continue, setting top_parent to a result with an Unassigned index. The next accessible directory then gets null from atIndex(Unassigned) as its parent, breaking config inheritance (tsconfig, package.json, etc.) from accessible ancestors above the EACCES gap. Move the top_parent assignment after the continue instead of using defer.
Extended reasoning...
The Bug
The defer top_parent = queue_top.result; statement at line 2797 unconditionally updates top_parent at the end of each loop iteration, including when the loop body hits a continue. In Zig, defer runs on scope exit, which includes continue statements. When an EACCES directory is skipped at line 2887 (const open_dir = maybe_open_dir orelse continue;), the defer still fires, overwriting top_parent with the skipped directory's result.
Why the Result Has an Invalid Index
When a new directory is looked up via getOrPut, the returned Result has index = Unassigned (defined as maxInt(u31) - 1 in src/allocators.zig:75). The markNotFound call at line 2845 only updates the hash map entry — it takes the Result by value, so queue_top.result.index remains Unassigned. The defer then copies this Unassigned-indexed result into top_parent.
Step-by-Step Proof
Consider a mixed-accessibility scenario: / is accessible, /home gets EACCES, /home/user is accessible.
-
Processing
/: Opens successfully,dir_cache.putat line 2967 updatesqueue_top.result.indexto a real index. Defer setstop_parentto/'s result (valid index)./gets a properDirInfo. -
Processing
/home:openDirAbsoluteZreturns EACCES. Code breaks toopen_dir = null. At line 2887,maybe_open_dir orelse continuetriggerscontinue. Thedeferfires, settingtop_parent = queue_top.resultwherequeue_top.result.index = Unassigned(markNotFound didn't change the index). -
Processing
/home/user: Opens successfully. At line 2975,r.dir_cache.atIndex(top_parent.index)is called withtop_parent.index = Unassigned. Perallocators.zig:605,atIndexreturnsnullforUnassignedindices. So/home/user'sDirInfogetsnullas its parent.
Impact
With a null parent, /home/user loses inheritance of enclosing_tsconfig_json, enclosing_package_json, package_json_for_browser_field, enclosing_browser_scope, package_json_for_dependencies, and inside_node_modules from /. In the typical Landlock scenario where ALL ancestors are inaccessible, this is mostly harmless since there are no accessible ancestors to inherit from. But in mixed-accessibility scenarios, configs from accessible higher-up directories are silently lost.
Suggested Fix
Replace the defer with an explicit assignment at the end of the successful code path (after line 2979, inside the loop body but only reached when the directory was actually processed). For example, remove line 2797 and add top_parent = queue_top.result; after the dirInfoUncached call around line 2979.
| if (syscall(__NR_landlock_restrict_self, ruleset_fd, 0)) { | ||
| fprintf(stdout, "LANDLOCK_UNSUPPORTED\\n"); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🟡 The Landlock helper C program is missing the required prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) call before landlock_restrict_self(). Without it, landlock_restrict_self returns EPERM on non-root processes, which the helper misclassifies as LANDLOCK_UNSUPPORTED, causing the test to silently skip on any non-root Linux system where Landlock IS supported. The fix is adding prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) before the landlock_restrict_self syscall (the standard pattern from all kernel Landlock documentation).
Extended reasoning...
What the bug is
The Landlock helper C program embedded in the test (lines 80-83 of the test file) calls landlock_restrict_self without first calling prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0). The Linux kernel documentation explicitly states that landlock_restrict_self() requires either: (1) the calling thread has set no_new_privs via prctl(PR_SET_NO_NEW_PRIVS), or (2) the thread has CAP_SYS_ADMIN in its current user namespace.
How it manifests
On a non-root Linux system with kernel >= 5.13 (where Landlock IS supported), the following sequence occurs:
landlock_create_rulesetsucceeds — the kernel supports Landlock- Rules are added successfully
landlock_restrict_selfis called without theno_new_privsattribute set- The kernel returns
EPERMbecause the process lacks bothno_new_privsandCAP_SYS_ADMIN - The helper prints
LANDLOCK_UNSUPPORTEDto stdout - The test reads this output and sets
landlockSupported = false(test line 124-126) - Both actual test cases check
if (!landlockSupported)and return early with a skip message
Why existing code doesn't prevent it
The error handling at lines 80-83 treats ANY failure from landlock_restrict_self as meaning Landlock is unsupported. This conflates "kernel doesn't support Landlock" with "missing required privilege setup". The EPERM from a missing prctl is indistinguishable from an actual unsupported-kernel error in this code path.
Impact
The regression test provides zero actual coverage on any non-root Linux system where Landlock IS supported. The test likely works in CI (Docker/root environments where CAP_SYS_ADMIN is available) but silently skips for any developer running tests locally as a regular user. This undermines the purpose of the regression test — developers cannot verify the fix locally.
Step-by-step proof
- Developer runs
bun test test/regression/issue/028220.test.tson Ubuntu 22.04 (kernel 5.15, Landlock supported) as a regular user - The helper is compiled and executed with
Bun.spawnSync landlock_create_rulesetsucceeds (kernel supports it)landlock_restrict_selffails withEPERM(noprctl, noCAP_SYS_ADMIN)- Helper outputs
LANDLOCK_UNSUPPORTED - Test sets
landlockSupported = falseand both test cases skip - Test suite reports green, but neither sandbox test actually ran
Fix
Add prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) before the landlock_restrict_self syscall, and #include <sys/prctl.h> at the top. This is the standard Landlock setup pattern shown in the kernel documentation (samples/landlock/sandboxer.c) and all official examples.
| @@ -0,0 +1,185 @@ | |||
| import { describe, expect, test } from "bun:test"; | |||
| import { existsSync, mkdirSync, writeFileSync } from "fs"; | |||
| import { bunEnv, bunExe } from "harness"; | |||
There was a problem hiding this comment.
🟡 Nit: rmSync is imported but never used (line 3), and check.js is written at line 118 but never referenced (the Landlock check uses -e for inline eval instead). Consider removing the unused import and dead writeFileSync, and adding an afterAll block to clean up /home/bun-test-user and /tmp/landlock-helper.
Extended reasoning...
Unused import
The test imports rmSync from "fs" on line 3, but there are zero calls to rmSync anywhere in the 185-line file. This is dead code — likely a leftover from planned cleanup logic that was never implemented.
Dead check.js file
At line 118, the test writes a check.js file:
writeFileSync(join(TEST_BASE, "check.js"), "console.log('ok');\n");However, the very next line (120) runs the Landlock support check using -e "console.log('test')" — an inline evaluation flag — instead of executing check.js. The file is created but never referenced by any test command. It appears to be a remnant from an earlier iteration of the test.
Missing cleanup
The test creates directories and files at two locations:
/home/bun-test-user/project(lines 117, 135, 164)/tmp/landlock-helper(line 103)
Neither location is cleaned up after the tests complete. The presence of the unused rmSync import strongly suggests cleanup was planned but never wired up. While CI machines are typically ephemeral and the hardcoded /home/bun-test-user/project path is intentional for Landlock testing (ancestor directories must not be in the allow list, ruling out tempDir), an afterAll block would be good hygiene.
Concrete walkthrough
- Line 3:
import { existsSync, mkdirSync, rmSync, writeFileSync } from "fs";—rmSyncimported - Lines 1-185: No call to
rmSyncanywhere in the file - Line 118:
writeFileSync(join(TEST_BASE, "check.js"), ...)— createscheck.js - Line 120:
cmd: [helperPath, TEST_BASE, dirname(bunExe()), bunExe(), "-e", "console.log('test')"]— uses-einline eval, notcheck.js - No
afterAllorafterEachblock exists in the describe block
Suggested fix
Remove rmSync from the import if no cleanup is added, or (better) add an afterAll block:
afterAll(() => {
rmSync("/home/bun-test-user", { recursive: true, force: true });
rmSync("/tmp/landlock-helper", { recursive: true, force: true });
});Also remove the dead writeFileSync(join(TEST_BASE, "check.js"), ...) on line 118.
|
Hitting this in a DevContainer environment where bun run fails with CouldntReadCurrentDirectory when the container restricts access to ancestor directories. This is blocking us from using Bun in our CI/sandboxed workflows. Would love to see this merged. Happy to test a build if that helps. |
|
I made this PR instead that (imo) fixes this issue in a better way: #28782, waiting for maintainers to take it to the next step |
|
Closing: this PR's implementation lives entirely in Zig source files that have since been removed from the tree as part of the Rust migration. The change can no longer merge cleanly and the files it edits no longer exist on If the underlying issue is still present, it will need a fresh fix against the Rust implementation. |
3 participants
Summary
/or/homemay not be readable even thoughgetcwd()succeeds and the CWD itself is fully accessibleEACCESon any ancestor directory as fatal, returningnulland causingCouldntReadCurrentDirectoryEACCES/AccessDeniederrors during ancestor directory walking are handled gracefully: the directory is cached as "not found" and the walk continues to the next directory in the queueRoot cause
In
src/resolver/resolver.zig, thedirInfoCachedMaybeLogfunction walks from the filesystem root (/) down to the CWD, opening each directory withopenDirAbsoluteZ(). When any directory returnsEACCES, the code fell into the generic error handler which returnednullfor the entire operation, cascading up to a fatalCouldntReadCurrentDirectoryerror.Fix
Added a specific
EACCES/AccessDeniederror case in the directory walk that:unreachableat the end of the function withreturn nullfor safetyConfig file discovery (
package.json,tsconfig.json,bunfig.toml) in inaccessible parent directories gracefully degrades — they simply won't be found, which is correct behavior in a sandboxed environment.Test plan
test/regression/issue/028220.test.ts) using Linux Landlock to create a sandbox where only the CWD is accessiblebun bd test), fails with system bun (USE_SYSTEM_BUN=1)bun runandbun runwithrequire()both work under Landlock sandboxCloses #28220
🤖 Generated with Claude Code