Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/bun.js/api/filesystem_router.zig
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,12 @@
const path_to_use = (root_dir_path.cloneWithTrailingSlash(allocator) catch unreachable).slice();

const root_dir_info = vm.transpiler.resolver.readDirInfo(path_to_use) catch {
// Build the JS error before freeing the arena: `log` is backed by the arena allocator.
const err_value = try log.toJS(globalThis, globalThis.allocator(), "reading root directory");
origin_str.deinit();
arena.deinit();
globalThis.allocator().destroy(arena);
return globalThis.throwValue(try log.toJS(globalThis, globalThis.allocator(), "reading root directory"));
return globalThis.throwValue(err_value);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
} orelse {
origin_str.deinit();
arena.deinit();
Expand All @@ -127,10 +129,12 @@
}) catch unreachable;

router.loadRoutes(&log, root_dir_info, Resolver, &vm.transpiler.resolver, router.config.dir) catch {
// Build the JS error before freeing the arena: `log` is backed by the arena allocator.
const err_value = try log.toJS(globalThis, globalThis.allocator(), "loading routes");
origin_str.deinit();
arena.deinit();
globalThis.allocator().destroy(arena);
return globalThis.throwValue(try log.toJS(globalThis, globalThis.allocator(), "loading routes"));
return globalThis.throwValue(err_value);
};

if (try argument.get(globalThis, "origin")) |origin| {
Expand All @@ -143,10 +147,12 @@
}

if (log.errors + log.warnings > 0) {
// Build the JS error before freeing the arena: `log` is backed by the arena allocator.
const err_value = try log.toJS(globalThis, globalThis.allocator(), "loading routes");
origin_str.deinit();
arena.deinit();
globalThis.allocator().destroy(arena);
return globalThis.throwValue(try log.toJS(globalThis, globalThis.allocator(), "loading routes"));
return globalThis.throwValue(err_value);
}

var fs_router = globalThis.allocator().create(FileSystemRouter) catch unreachable;
Expand Down Expand Up @@ -246,9 +252,11 @@
.asset_prefix_path = this.router.config.asset_prefix_path,
}) catch unreachable;
router.loadRoutes(&log, root_dir_info, Resolver, &vm.transpiler.resolver, router.config.dir) catch {
// Build the JS error before freeing the arena: `log` is backed by the arena allocator.
const err_value = try log.toJS(globalThis, globalThis.allocator(), "loading routes");

Check notice on line 256 in src/bun.js/api/filesystem_router.zig

View check run for this annotation

Claude / Claude Code Review

reload(): readDirInfo catch block leaks the new arena

Pre-existing, not introduced here, but while you're normalizing arena lifetime around the `log.toJS` sites: the `readDirInfo` catch block in `reload()` (just above the `loadRoutes` catch you fixed) returns without `arena.deinit()` / `destroy(arena)`, leaking the freshly-created arena whenever `readDirInfo` errors. The adjacent `orelse` branch and the constructor's equivalent block both free it, so applying the same `const err_value = ...; arena.deinit(); destroy(arena); return throwValue(err_val
Comment thread
robobun marked this conversation as resolved.
Outdated
arena.deinit();
globalThis.allocator().destroy(arena);
return globalThis.throwValue(try log.toJS(globalThis, globalThis.allocator(), "loading routes"));
return globalThis.throwValue(err_value);
};
Comment thread
robobun marked this conversation as resolved.

this.router.deinit();
Expand Down
34 changes: 34 additions & 0 deletions test/js/bun/util/filesystem_router.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -510,3 +510,37 @@ it("MatchedRoute.params does not leak", async () => {
expect(stdout).toBe("");
expect(exitCode).toBe(0);
}, 60_000);

it("throws a clean error for invalid route filenames (no use-after-free)", async () => {
// The constructor's log is backed by an arena allocator. When route loading
// produces errors (e.g. a filename like `[foo.tsx` missing its closing bracket),
// the arena must not be freed before log.toJS() reads the messages.
// Run in a subprocess so an ASAN crash doesn't take down the test runner.
using dir = tempDir("fsr-invalid-route", {
"pages/[foo.tsx": "export default 1;",
});

const code = /* ts */ `
try {
new Bun.FileSystemRouter({
style: "nextjs",
dir: ${JSON.stringify(path.join(String(dir), "pages"))},
fileExtensions: [".tsx"],
});
console.log("no-throw");
} catch (e) {
console.log("caught:" + (e?.message ?? String(e)));
}
`;

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", code],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout.trim()).toBe("caught:Route is missing a closing bracket]");
expect(exitCode).toBe(0);
});
Loading