Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
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
28 changes: 15 additions & 13 deletions src/bundler/bundle_v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6826,19 +6826,21 @@ pub mod bv2_impl {
.text;
let loader = this.graph.input_files.items_loader()[source_index as usize];
if this.should_add_watcher(source_path) {
// const generic `CLONE_FILE_PATH = isWindows`
// matches `cfg!(windows)` at compile time.
let _ = this
.bun_watcher_mut()
.unwrap()
.add_file::<{ cfg!(windows) }>(
parse_result.watcher_data.fd,
source_path,
bun_wyhash::hash(source_path) as u32,
bun_watcher::Loader(loader as u8),
parse_result.watcher_data.dir_fd,
None,
);
// CLONE_FILE_PATH=true: `source_path` is not guaranteed to
// be process-lifetime storage — `dupe_alloc` allocates the
// disjoint `text\0pretty\0` case (e.g. every `ssr:`-prefixed
// SSR-graph path) from the per-bundle arena, which is freed
// when the bundle completes. The watchlist outlives the
// bundle, so it must own its copy of the path (the watcher
// dedups by hash, so this is one copy per watched file).
let _ = this.bun_watcher_mut().unwrap().add_file::<true>(
parse_result.watcher_data.fd,
source_path,
bun_wyhash::hash(source_path) as u32,
bun_watcher::Loader(loader as u8),
parse_result.watcher_data.dir_fd,
None,
);
}
}
}
Expand Down
17 changes: 14 additions & 3 deletions src/runtime/bake/DevServer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4503,13 +4503,24 @@ pub(super) fn finalize_bundle(
dev.incremental_result.html_routes_soft_affected.clear();
ctx.gts.clear();

for index in &dev.incremental_result.client_components_affected {
// `trace_dependencies` appends to `client_components_affected` when it
// visits another client component boundary, so iterating a slice of the
// Vec here is iterator invalidation: the push reallocates the buffer and
// the stale iterator walks freed memory (whose bytes are promptly reused
// by `framework_routes_affected` pushes), producing garbage file indexes
// that crash `trace_dependencies` with an index-out-of-bounds. Iterate by
// index against the live Vec instead — entries appended mid-loop already
// had their `gts` bit set when pushed, so re-visiting them is a no-op.
let mut i = 0;
while i < dev.incremental_result.client_components_affected.len() {
let index = dev.incremental_result.client_components_affected[i];
dev.server_graph.trace_dependencies(
*index,
index,
ctx.gts,
incremental_graph::TraceDependencyGoal::NoStop,
*index,
index,
)?;
i += 1;
}

for request in &dev.incremental_result.framework_routes_affected {
Expand Down
37 changes: 33 additions & 4 deletions src/runtime/bake/FrameworkRouter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1385,7 +1385,10 @@
}

pub fn print(&self, rel_path: &[u8]) {
let cursor_at = self.cursor_at as usize;
// `cursor_at` may still be the `empty()` sentinel (`u32::MAX`) when the
// producer recorded a message without a cursor position. Clamp to the
// path length so the slicing below cannot go out of bounds.
let cursor_at = (self.cursor_at as usize).min(rel_path.len());
let cursor_len = self.cursor_len as usize;
let after = &rel_path[cursor_at..];
Output::err_generic(
Expand Down Expand Up @@ -1639,8 +1642,25 @@
.parse(rel_path, ext, &mut log, t.allow_layouts, arena_state);
let parsed = match parse_result {
Err(_) => {
log.cursor_at +=
u32::try_from(abs_root_len - root_len).expect("int cast");
// Rebase the cursor from `rel_path` coordinates
// (where `parse` recorded it) onto
// `full_rel_path` (what `print` receives).
// `rel_path` starts one byte before the
// `full_rel_path` suffix it mirrors — the
// leading '/' — so the offset is one less than
// the root-length delta. Error paths that never
// called `log.fail()` (e.g. OOM) leave the
// `u32::MAX` sentinel; keep it pinned there so
// `print` clamps to the end of the path.
if log.cursor_at != u32::MAX {
log.cursor_at = log
.cursor_at
.saturating_add(
u32::try_from(abs_root_len - root_len)
.expect("int cast"),
)
.saturating_sub(1);
}
ctx.on_router_syntax_error(full_rel_path, log)?;
arena_state.reset_retain_with_limit(8 * 1024 * 1024);
continue 'outer;
Expand Down Expand Up @@ -1673,7 +1693,16 @@
}

if param_count > 64 {
log.write(format_args!("Pattern cannot have more than 64 param"));
// Highlight the whole path: unlike `fail()` callers
// inside `parse`, there is no narrower span to point
// at. Leaving the cursor unset (the `empty()`
// sentinel) previously made `TinyLog::print` slice
// `rel_path[u32::MAX..]` and crash the dev server.
let _ = log.fail(
format_args!("Pattern cannot have more than 64 param"),
0,
full_rel_path.len(),
);

Check warning on line 1705 in src/runtime/bake/FrameworkRouter.rs

View check run for this annotation

Claude / Claude Code Review

Comments contain bug-history narrative (CLAUDE.md convention)

nit: per root CLAUDE.md ("Comments carry only durable non-obvious content … no bug history, no 'we use X because Y was broken' — that belongs in the PR description"), the past-tense crash narrative here ("previously made `TinyLog::print` slice `rel_path[u32::MAX..]` and crash the dev server") and the freed-memory-reuse middle of the DevServer.rs:4506-4513 comment ("the push reallocates the buffer and the stale iterator walks freed memory … producing garbage file indexes that crash `trace_depende
Comment thread
robobun marked this conversation as resolved.
Outdated
ctx.on_router_syntax_error(full_rel_path, log)?;
arena_state.reset_retain_with_limit(8 * 1024 * 1024);
continue 'outer;
Expand Down
62 changes: 62 additions & 0 deletions test/bake/dev/bundle.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -814,3 +814,65 @@ devTest("barrel optimization: two import statements from the same barrel (#28886
await c.expectMessage("got: ALPHA BETA");
},
});

// Regression: editing a file shared by several client component boundaries
// crashed the dev server. `finalizeBundle` iterates
// `incremental_result.client_components_affected` while `trace_dependencies`
// re-appends every visited boundary to that same list (the trace bits were
// just cleared), so with 3+ boundaries the Vec reallocates mid-iteration and
// the stale iterator reads freed memory — caught as a use-after-free under
// ASAN, and as `index out of bounds: the len is N but the index is N` in
// release once the freed block is reused by `framework_routes_affected`.
devTest("hot update touching multiple client component boundaries does not crash", {
framework: {
...minimalFramework,
serverComponents: {
...minimalFramework.serverComponents!,
separateSSRGraph: true,
},
},
files: {
"routes/index.ts": `
import * as A from '../components/A';
import * as B from '../components/B';
import * as C from '../components/C';
import * as D from '../components/D';
export default function (req, meta) {
// Client component exports are reference proxies on the server; only
// their existence is observable here.
return new Response('page: ' + [typeof A.a, typeof B.b, typeof C.c, typeof D.d].join(','));
}
`,
"components/A.ts": `
"use client";
import { shared } from './shared';
export const a = 'A' + shared;
`,
"components/B.ts": `
"use client";
import { shared } from './shared';
export const b = 'B' + shared;
`,
"components/C.ts": `
"use client";
import { shared } from './shared';
export const c = 'C' + shared;
`,
"components/D.ts": `
"use client";
import { shared } from './shared';
export const d = 'D' + shared;
`,
"components/shared.ts": `export const shared = '1';`,
},
async test(dev) {
await dev.fetch("/").equals("page: object,object,object,object");
// Invalidate every client component boundary in a single hot update.
await dev.write("components/shared.ts", `export const shared = '2';`);
await dev.fetch("/").equals("page: object,object,object,object");
// A second round exercises the watcher entries recorded during the
// previous (SSR-duplicated) bundle.
await dev.write("components/shared.ts", `export const shared = '3';`);
await dev.fetch("/").equals("page: object,object,object,object");
},
});
77 changes: 76 additions & 1 deletion test/bake/framework-router.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { frameworkRouterInternals } from "bun:internal-for-testing";
import { describe, expect, test } from "bun:test";
import { tempDirWithFiles } from "harness";
import { bunEnv, bunExe, isWindows, tempDirWithFiles } from "harness";
import path from "path";

const { parseRoutePattern, FrameworkRouter } = frameworkRouterInternals;
Expand Down Expand Up @@ -133,3 +133,78 @@ test("discovers from filesystem paths", () => {
],
});
});

// Regression: printing a route syntax error crashed the dev server instead of
// reporting it. The 65+ parameter check recorded its message without a cursor
// position, leaving `TinyLog.cursor_at` at the `u32::MAX` sentinel, and
// `TinyLog.print` sliced `rel_path[cursor_at..]` with it.
// The 65-directory fixture exceeds Windows' MAX_PATH; the code under test is
// platform-independent.
test.skipIf(isWindows)("dev server reports route syntax errors without crashing", async () => {
const manyParams = Array.from({ length: 65 }, (_, i) => `[p${i}]`).join("/");
const dir = tempDirWithFiles("fsr-syntax-error", {
"server.ts": `
export function render(req, meta) {
return meta.pageModule.default(req, meta);
}
`,
"main.ts": `
export default {
port: 0,
development: true,
app: {
framework: {
fileSystemRouterTypes: [
{ root: "routes", style: "nextjs-pages", serverEntryPoint: "./server.ts" },
],
},
},
};
`,
"routes/index.tsx": `export default () => new Response("ok");`,
"routes/[bad.tsx": `export default () => new Response("bad");`,
[`routes/${manyParams}/index.tsx`]: `export default () => new Response("deep");`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "main.ts"],
cwd: dir,
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});

// Drain stderr concurrently with the stdout scan below so neither pipe
// can fill up and block the child.
const stderrPromise = proc.stderr.text();

// The route scan runs during server startup, so "Started development
// server" only prints after both syntax errors were reported. Without the
// fix the process dies while printing the 65-param error.
let stdout = "";
let url: string | undefined;
const decoder = new TextDecoder();
for await (const chunk of proc.stdout) {
stdout += decoder.decode(chunk, { stream: true });
url = stdout.match(/Started development server: (http:\/\/\S+)/)?.[1];
if (url) break;
}
expect(url).toBeDefined();

// The server must still respond after reporting the errors.
const res = await fetch(url!);
expect(await res.text()).toBe("ok");

proc.kill();
const [stderr] = await Promise.all([stderrPromise, proc.exited]);
// The caret/underline must line up with the offending "[" — column
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// 8 for the `error: "` prefix plus 7 for `routes/`.
expect(stderr).toContain(
'error: "routes/[bad.tsx" is not a valid route\n' +
" ".repeat(8 + 7) +
"-------\n" +
" ".repeat(8 + 7) +
'Missing "]" to match this route parameter',
);
expect(stderr).toContain("Pattern cannot have more than 64 param");
});
Loading