Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 5:09 PM PT - Jul 28th, 2026
❌ @robobun, your commit 0c1ccf7 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36214That installs a local version of the PR into your bun-36214 --bun |
|
Found 7 issues this PR may fix:
🤖 Generated with Claude Code |
|
Re the find-issues suggestions: not adding |
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughChangesThe concurrent task tag scheme reserves Concurrent task sentinel
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/internal/concurrent-task-sentinel.test.ts`:
- Around line 15-19: The regression coverage in
test/internal/concurrent-task-sentinel.test.ts lines 15-19 and 45-52 must verify
both sentinel contracts: add a direct assertion that the Access task tag is no
longer 0, and add release/alternate-mode coverage that reaches the targeted
dispatch panic rather than only the event-loop debug_assert!; update the tests
around the existing access round-trip and concurrent-task sentinel cases without
changing their current coverage.
- Line 20: Update the missing-file assertion in the access test to verify that
the rejected error has code ENOENT, rather than only asserting that any error is
thrown. Preserve the existing nonexistent path and asynchronous rejection flow.
🪄 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: 1612c33a-9b0c-428d-8fcf-e0f1431cf3fa
📒 Files selected for processing (6)
src/event_loop/ConcurrentTask.rssrc/js/internal-for-testing.tssrc/jsc/event_loop.rssrc/runtime/dispatch.rssrc/runtime/dispatch_js2native.rstest/internal/concurrent-task-sentinel.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the test-assertion feedback from my earlier review was applied in 896562c.
What was reviewed:
- Tag renumbering is transparent: grepped all of
src/for numericTaskTagreferences — everyTaskableimpl andNodeFSFunctionEnum::task_tag()use namedtask_tag::*constants; the onlyCOUNTconsumer is the compile-time exhaustiveness assert (correctly bumped 96→97). - Hot-path impact: the new
run_taskarm is a cold panic after all real arms; the drain-site check isdebug_assert!(release no-op). - Test hook is gated behind
bun:internal-for-testing; the leakedConcurrentTaskbox in the hook is irrelevant since dispatch panics on the next drain.
Extended reasoning...
Overview
Reserves TaskTag(0) as task_tag::INVALID so an all-zeros Task (the bit pattern of ConcurrentTask::default(), and what a UAF'd inline concurrent_task field reads back as) produces a labelled panic instead of dispatching as whichever real type happened to be tag 0 (fs.access, which segfaulted at self.result with self = null). Real tags now start at 1 via the tags! macro seed change; a debug_assert! at the concurrent-drain site catches it one step earlier with the node address, and run_task gains an explicit INVALID panic arm. A test-only enqueueZeroedConcurrentTaskForTesting hook is exposed via bun:internal-for-testing, and a spawned-child test asserts the panic message and non-segfault.
Security risks
None. Diagnostic-only; no user-facing surface, no untrusted input handling. The test hook is behind BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING and only reachable via bun:internal-for-testing.
Level of scrutiny
This touches the event-loop dispatch hot path, which normally warrants high scrutiny — but the actual changes there are (a) a compile-time constant shift in a macro seed, (b) one new match arm that panics, and (c) a debug_assert!. None alter behavior for any valid tag. I verified via grep that nothing in src/ compares against or constructs numeric tag values directly; every producer goes through named task_tag::* constants (including the NodeFSFunctionEnum::task_tag() const-fn table at node_fs.rs:10166), so the +1 shift is invisible to all of them. The COUNT == 97 compile-time assert is the only other COUNT consumer.
Other factors
My prior inline comment (on .resolves.toBeNull() coupling to a Node-compat divergence, and the bare .rejects.toThrow()) was addressed in 896562c — the test now does a plain await access(...) and asserts { code: "ENOENT" }. All comment-cop and CodeRabbit threads are resolved. The PR is explicitly diagnostic-only (author declined to add Fixes # lines for the suspected underlying UAFs, which is correct). The test asserts the shared "zeroed ConcurrentTask" prefix so debug lanes exercise the drain-site debug_assert! and release lanes exercise the run_task panic — both new paths covered across the CI matrix.
There was a problem hiding this comment.
Thanks for applying the test-assertion changes in 896562c — that resolves my earlier note. I didn't find any further issues, but deferring to a human since this renumbers the TaskTag space and adds an arm to run_task, which is the core event-loop dispatch hot path.
What was reviewed:
- Tag renumbering: grepped for hardcoded
TaskTag(N)andtask_tag::COUNTconsumers — only the newINVALIDand the const-assert (updated 96→97) reference numeric values;NodeFSFunctionEnum::task_tag()and allTaskableimpls use named constants. debug_assert!at drain site compiles out in release; the newINVALIDpanic arm is cold and precedes the existing wildcard, so valid-tag dispatch is unchanged.suppress_reporting()in the test hook is reachable only viabun:internal-for-testing(feature-flag gated), matching the crash-handler doc's guard requirement.
Extended reasoning...
Overview
This PR reserves TaskTag(0) as a sentinel (task_tag::INVALID) so that a zeroed ConcurrentTask — the bit pattern produced by Default and, more importantly, by a UAF of an inline concurrent_task field — panics with a diagnostic message instead of dispatching as whichever real task type happened to be value 0 (previously Access, which segfaulted at self.result with a null self). Files touched: src/event_loop/ConcurrentTask.rs (macro start index 0→1, INVALID const, Default doc), src/jsc/event_loop.rs (debug_assert at drain + test-only enqueue hook), src/runtime/dispatch.rs (INVALID panic arm + COUNT assert bump), src/runtime/dispatch_js2native.rs (re-export), src/js/internal-for-testing.ts (JS binding), and a new test file.
Security risks
None identified. The change is diagnostic-only; it doesn't parse untrusted input, touch auth/crypto, or expose new user-facing surface. The test hook is gated behind bun:internal-for-testing (requires BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING=1), and it deliberately crashes the process — not a data-exposure vector.
Level of scrutiny
High — run_task is the ~96-arm hot dispatch that every completed work-pool task flows through, and tick_concurrent_with_count is the MPSC drain. The changes themselves are minimal (a debug_assert that compiles out in release, a cold panic arm, and a macro seed-value bump), but the tag renumbering shifts every real tag by +1. I verified via grep that nothing in src/ references a numeric TaskTag value other than the new INVALID = TaskTag(0), that NodeFSFunctionEnum::task_tag() maps through named task_tag::* constants, and that task_tag::COUNT has exactly one consumer (the const-assert in dispatch.rs, updated 96→97). Task carries a raw pointer so there's no serialization/ABI concern across builds.
Other factors
All prior review threads are resolved: the comment-cop flags were trimmed or justified as short rustdoc; both CodeRabbit findings were withdrawn after the author explained the shared "zeroed ConcurrentTask" prefix covers debug and release lanes; my earlier note on .resolves.toBeNull() coupling and bare toThrow() was applied in 896562c (now plain await access(...) + .rejects.toMatchObject({ code: "ENOENT" })). The test spawns a subprocess, drains pipes concurrently, asserts the panic text and absence of segfault markers, and asserts non-zero exit last. The 30s per-test timeout is reasonable given the crash handler symbolicates a full backtrace under debug+ASAN. Deferring only because this is core event-loop infrastructure and REVIEW.md's guidance on critical code paths applies; I have no correctness concerns.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/dispatch.rs (1)
470-476: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude a remediation step in the panic message.
The message identifies the failure and likely causes, but not what to fix. Tell callers to keep the inline owner alive until drain and initialize
.taskbefore enqueueing.Proposed fix
- "zeroed ConcurrentTask (tag=INVALID, ptr={:p}): use-after-free of an inline \ - concurrent_task field, or enqueue of a default-initialised ConcurrentTask", + "zeroed ConcurrentTask (tag=INVALID, ptr={:p}): an inline concurrent_task \ + owner was freed while queued, or a default-initialised ConcurrentTask was \ + enqueued; keep the owner alive until drain and initialize .task before enqueueing",As per coding guidelines, error messages must identify the resource, rejected value, constraint, cause, and remedy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/dispatch.rs` around lines 470 - 476, Update the panic message in the task_tag::INVALID branch to retain the existing failure details and explicitly instruct callers to keep the inline owner alive until drain and initialize .task before enqueueing.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/dispatch.rs`:
- Around line 470-476: Update the panic message in the task_tag::INVALID branch
to retain the existing failure details and explicitly instruct callers to keep
the inline owner alive until drain and initialize .task before enqueueing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 177a596d-e2be-4af9-a57f-31a6d4678ad4
📒 Files selected for processing (4)
src/event_loop/ConcurrentTask.rssrc/jsc/event_loop.rssrc/runtime/dispatch.rstest/internal/concurrent-task-sentinel.test.ts
There was a problem hiding this comment.
No issues found, but deferring to a human: this renumbers all ~96 TaskTag values and adds a panic arm on the event-loop dispatch hot path. The change looks correct and my earlier feedback on the access() smoke test was applied in 896562c, but a maintainer should confirm the sentinel-tripwire approach (vs. root-causing the suspected UAF) and the "nothing references hard-coded tag values" claim.
What was reviewed:
tags!macro seed 0→1 andCOUNTbump — all consumers (Taskableimpls,NodeFSFunctionEnum::task_tag(),run_taskarms) use namedtask_tag::*constants; grep found no numeric-literal tag references.run_task's newINVALIDarm sits before the wildcard and shares the "zeroed ConcurrentTask" prefix with the drain-sitedebug_assert!, so the test's.toContaincovers both debug and release paths.- Test-only hook: heap-allocates with
auto_delete: trueand callssuppress_reporting()so the deliberate panic doesn't upload a crash report; gated behindbun:internal-for-testing.
Extended reasoning...
Overview
Reserves TaskTag(0) as a sentinel (task_tag::INVALID) so an all-zeros Task — the bit pattern of ConcurrentTask::default(), and what the drain loop observes when an inline concurrent_task field's owner is UAF'd while queued — panics with a labelled message instead of dispatching as Access with self = null. Touches: the tags! macro seed in src/event_loop/ConcurrentTask.rs (0→1), a new match arm in src/runtime/dispatch.rs::run_task, a debug_assert! in src/jsc/event_loop.rs::tick_concurrent_with_count, a test-only enqueue hook wired through internal-for-testing.ts / dispatch_js2native.rs, and a new test file.
Security risks
None. The new hook is test-only (bun:internal-for-testing, gated by BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING) and deliberately crashes the calling process; it exposes no new attack surface to userland.
Level of scrutiny
High. run_task and tick_concurrent_with_count are on the per-task hot path of the JS event loop; the tag-constant table is load-bearing for every async dispatch in the runtime. The change itself is small (macro seed, one match arm, one debug_assert!), and the compile-time task_tag::COUNT == 97 assert plus symbolic-constant usage everywhere makes the renumber mechanically safe — I grepped for TaskTag(0 / numeric tag comparisons and found none outside this PR. But renumbering a 96-variant discriminant that fans out across bun_runtime, bun_jsc, bun_event_loop, and every Taskable impl warrants a maintainer's eyes.
Other factors
- The PR is explicitly diagnostic-only ("does not fix that UAF"). REVIEW.md's "fix bugs at the layer that owns the violated invariant" applies to the underlying UAF, but this change doesn't paper over it — it converts UB into a loud, labelled crash, which is a strict improvement for a bug that didn't reproduce in 80+ local runs. Whether to land the tripwire before the root-cause fix is a maintainer call.
- My prior inline comment (couple
access()resolve value, baretoThrow()) was addressed in 896562c; the test now uses a plainawait access(...)and.rejects.toMatchObject({ code: "ENOENT" }). - The comment-cop bot flagged doc comments repeatedly; the author trimmed once and pushed back (correctly) on the remaining 2-3-line rustdoc — those threads are all resolved.
- CodeRabbit's two findings were withdrawn after the author's replies; both reasonings check out (the panic test is the tag-0 contract check; debug/release both hit the shared "zeroed ConcurrentTask" prefix).
- Test spawns a child that panics under ASAN with backtrace symbolication (hence the 30s per-test timeout);
suppress_reporting()was added in 2399661 to keep the deliberate crash out of CI's crash-report set.
|
Found the producer: the freed node is |
A ConcurrentTask::default() produces an all-zeros .task field, which is also what the consumer observes when an inline concurrent_task field's owner is freed while the node is still linked in the concurrent queue. Before this change, tag value 0 was task_tag::Access, so such a node dispatched as AsyncFSTask<_, Access, _>::run_from_js_thread with a null self pointer and segfaulted at the self.result field offset (0x48 on aarch64), which is what we saw on Windows 11 aarch64 in bun-serve-html.test.ts. Reserve value 0 as task_tag::INVALID and shift the real tags to start at 1. run_task matches it with a panic that says what happened, and a debug_assert at the concurrent-queue drain site flags it one step earlier with the ConcurrentTask address. This does not fix the UAF itself; it turns the bug class into a labelled crash so the next occurrence is diagnosable on any platform.
The coredump-upload CI lanes flag any new core file as a failure; the test hook is for triggering the INVALID panic on purpose, so zero the RLIMIT_CORE and suppress the report upload before enqueueing.
f08142e to
0c1ccf7
Compare
There was a problem hiding this comment.
Thanks for applying the earlier test feedback. I didn't find any bugs, but this renumbers every task tag and adds a panic arm to the hot run_task dispatch — core event-loop infrastructure — so it's worth a human confirming the sentinel/panic design and the "nothing references hard-coded tag values" claim before merge.
Reviewed: the tags! macro shift (0→1 start, COUNT semantics), the new INVALID arm placement in run_task, the debug_assert! at the drain site, and the test-only enqueue hook (heap-allocated, auto_delete: true, suppress_reporting() before the deliberate panic). Grepped src/ for literal TaskTag(N) / numeric tag comparisons — only the new INVALID definition matches. The access() smoke test now avoids the null/undefined coupling and asserts ENOENT.
Extended reasoning...
Overview
Reserves TaskTag(0) as task_tag::INVALID so a zeroed ConcurrentTask (the bit pattern seen when an inline concurrent_task field's owner is freed while still queued) panics with a labelled message instead of dispatching as fs.access with self = null. Touches the tag-generating macro in src/event_loop/ConcurrentTask.rs, adds a match arm + COUNT bump in src/runtime/dispatch.rs, a debug_assert! in tick_concurrent_with_count, and a test-only hook in src/jsc/event_loop.rs wired through internal-for-testing.ts and dispatch_js2native.rs. New test in test/internal/.
Security risks
None. No user-facing surface; the only new entry point is gated behind bun:internal-for-testing + BUN_FEATURE_FLAG_INTERNAL_FOR_TESTING, and it heap-allocates its own node with auto_delete: true so the drain path frees it (though the process panics before that matters).
Level of scrutiny
High — run_task and tick_concurrent_with_count are the per-task hot path for every async operation. The change itself is mechanically small (two 0u8 → 1u8 in the macro, one new match arm, one debug_assert), and I verified via grep that no code matches on numeric TaskTag values or constructs TaskTag(N) literals outside the new INVALID constant. But renumbering all ~96 tags is the kind of thing where a maintainer should sanity-check that no serialized/persisted tag values exist anywhere (I found none, but I don't have full visibility into e.g. IPC or cache formats).
Other factors
- My earlier inline feedback (decouple the
access()smoke from thenull-vs-undefinedresolve value; assertENOENTinstead of bare.toThrow()) was applied in 293a00e / 896562c and is reflected in the current diff. - The PR is explicitly diagnostic-only; the actual UAF producer is being fixed separately in #36247. A human should decide whether to land this independently or fold it into that fix.
- The PR body's own evidence shows
release without fix: all passedfor the new test, which suggests the test may not fail against a release binary lacking the hook (the child would throw "is not a function" rather than segfault, which happens to also satisfy thenot.toContain("unreachable")assertion but should fail thetoContain("zeroed ConcurrentTask")one — worth a second look at how that lane ran). - All comment-cop and CodeRabbit threads are resolved.
|
CI status: The remaining reds on build 84317 are unrelated to this diff:
None touch the task-tag dispatch path. Ready for review. |
…ed (#36247) ## What `test/js/bun/http/bun-serve-html.test.ts` segfaults on `windows-aarch64` after #36175 landed (builds 84162, 84194; one earlier sighting in 83933): ``` panic(main thread): Segmentation fault at address 0x48 Features: ... dev_server(14) ... ``` Symbolicated in #36214 as `AsyncFSTask<Access>::run_from_js_thread` with `self = null`, i.e. a zeroed `ConcurrentTask` was dispatched. ## Cause `DevServer.watcher_atomics.events[*].concurrent_task` is the intrusive MPSC node the watcher thread links into `EventLoop.concurrent_tasks` when it submits a hot-reload event. It was an inline field of `DevServer`, so `server.stop()` → `drop(Box<DevServer>)` freed it while it was still linked. The next `tick_concurrent` then read `.next`/`.task`/`.auto_delete` from freed memory. ASAN on Linux confirms: ``` heap-use-after-free: ConcurrentTask::get_next (unbounded_queue.rs) ← BatchIterator::next ← EventLoop::tick_concurrent_with_count freed by: Box<DevServer>::drop ← NewServer::deinit_if_we_can ← NewServer::stop ← dispose_from_js (using server) ``` On release builds the freed block reads back as zeros, so the copied `Task` is `{tag: 0, ptr: null}`; tag 0 is `task_tag::Access`, whose `run_from_js_thread` loads `self.result` at offset `0x48`. The bug is latent and platform-agnostic. #36175 exposed it because the CI runner now spawns the napi addon prebuild in the background while serial tests run; that writes under the watched project root, so the `jsx-runtime` DevServers in this test file now reliably receive a hot-reload event between the last `await fetch` and `using server` disposal. ## Fix `watcher_atomics` is now a `NonNull<WatcherAtomics>` owned via `bun_core::heap::into_raw`, so the allocation can outlive `DevServer` and every queued pointer keeps allocation-root provenance. `watcher_acquire_event`, `watcher_release_and_submit_event` and `recycle_event_from_dev_server` take `*mut Self` and derive the returned `*mut HotReloadEvent` (and the linked `concurrent_task` node) from that root pointer via raw place projections rather than from a `&mut WatcherAtomics` reborrow. `Drop for DevServer` reads `next_event` after `Watcher::shutdown` has serialised out the watcher thread (which guarantees it is stable): - `DONE`: nothing is queued; clear and `heap::destroy` as before. - otherwise: a `concurrent_task` is still linked (or its `Task` is already in the drain FIFO). Null `owner` on every event and leave the allocation alive. `HotReloadEvent::run` checks `owner.is_null()` first; when set it reclaims the allocation via the new `atomics` backref and returns without touching the dead `DevServer`. The `# Safety` contracts on `run` and the `BakeHotReloadEvent` dispatch arm are updated to describe the null-owner case. ## Test `test/js/bun/http/bun-serve-html-hot-reload-drop.test.ts` creates a development server, bundles once so `app.js` is watched, synchronously rewrites `app.js`, spins briefly without yielding so the watcher thread can enqueue, disposes the server, then yields. Ten iterations. In a separate file because the React-bundling cases in `bun-serve-html.test.ts` already exceed the default per-test timeout under a debug+ASAN build on `main`. <details><summary>fail-before (debug+ASAN, src/ at main)</summary> ``` ==25521==ERROR: AddressSanitizer: heap-use-after-free on address 0x79315e4743e8 READ of size 8 at 0x79315e4743e8 thread T0 #2 <ConcurrentTask as Node>::get_next unbounded_queue.rs:82 #3 BatchIterator<ConcurrentTask>::next unbounded_queue.rs:135 #4 EventLoop::tick_concurrent_with_count event_loop.rs:507 0x79315e4743e8 is located 488 bytes inside of 16512-byte region freed by thread T0 here: #9 Box<DevServer>::drop #12 NewServer<false,true>::deinit_if_we_can mod.rs:1770 #13 NewServer<false,true>::stop mod.rs:1665 #14 NewServer<false,true>::dispose_from_js server_body.rs:2584 ``` </details> Passes with the fix in ~2.4s under debug+ASAN (also on a local `windows-aarch64` debug build, where the original `bun-serve-html.test.ts` is now 19/19); `test/bake/deinitialization.test.ts` still green. Supersedes the producer half of #36214 (which adds a sentinel for the same zeroed-task symptom). <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 4 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 failed, 2 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/http/bun-serve-html.test.ts test/js/bun/http/bun-serve-html-hot-reload-drop.test.ts bun test v1.4.0 (5f6622f) test/js/bun/http/bun-serve-html.test.ts: waitForServer /tmp/html-css-js_ObkyZk { "/": "/tmp/html-css-js_ObkyZk/index.html", "/dashboard": "/tmp/html-css-js_ObkyZk/dashboard.html", } [0.12ms] bundle index.html 1.09 KB [0.05ms] bundle dashboard.html 1.27 KB (pass) serve html [630.67ms] waitForServer /tmp/bun-serve-html-txt_5C6B7a { "/": "/tmp/bun-serve-html-txt_5C6B7a/index.html", } [0.15ms] bundle index.html 0.40 KB HASH efbnbska (pass) serve plugins > basic plugin [556.20ms] waitForServer /tmp/html-css-js-failing-plugin_OPRhwb { "/": "/tmp/html-css-js-failing-plugin_OPRhwb/index.html", } error: Plugin failed intentionally at /tmp/html-css-js-failing-plugin_OPRhwb/styles.css:0 error: Plugin failed intentionally at /tmp/html-css-js-failing-plugin_OPRhwb/styles.css:0 (pass) serve plugins > serve html with failing plugin [491.35ms] waitForServer /tmp/html-css-js-empty-plugins_biqnN6 { "/": "/tmp/htm ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (96ff7ec) test/js/bun/http/bun-serve-html.test.ts: waitForServer /tmp/html-css-js_ZmgkBG { "/": "/tmp/html-css-js_ZmgkBG/index.html", "/dashboard": "/tmp/html-css-js_ZmgkBG/dashboard.html", } [0.00ms] bundle index.html 1.09 KB [0.00ms] bundle dashboard.html 1.27 KB (pass) serve html [25.17ms] waitForServer /tmp/bun-serve-html-txt_uWNogT { "/": "/tmp/bun-serve-html-txt_uWNogT/index.html", } [0.00ms] bundle index.html 0.40 KB HASH efbnbska (pass) serve plugins > basic plugin [17.25ms] waitForServer /tmp/html-css-js-failing-plugin_Kd1a8p { "/": "/tmp/html-css-js-failing-plugin_Kd1a8p/index.html", } error: Plugin failed intentionally at /tmp/html-css-js-failing-plugin_Kd1a8p/styles.css:0 error: Plugin failed intentionally at /tmp/html-css-js-failing-plugin_Kd1a8p/styles.css:0 (pass) serve plugins > serve html with failing plugin [16.33ms] waitForServer /tmp/html-css-js-empty-plugins_Ecz7qJ { "/": "/tmp/html-css-js-empty-plugins_Ecz7qJ/index.html", } [0.00ms] bundle index.html 0.71 KB (pass) serve plugins > empty plugin array [13.23ms] Waiting for server waitForServer /tmp/html-css-js-concurrent-plugins_l7wQg5 { "/": "/tmp/ ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 2 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/http/bun-serve-html.test.ts test/js/bun/http/bun-serve-html-hot-reload-drop.test.ts bun test v1.4.0 (5f6622f) test/js/bun/http/bun-serve-html.test.ts: waitForServer /tmp/html-css-js_CpXhx4 { "/": "/tmp/html-css-js_CpXhx4/index.html", "/dashboard": "/tmp/html-css-js_CpXhx4/dashboard.html", } [0.09ms] bundle index.html 1.09 KB [0.05ms] bundle dashboard.html 1.27 KB (pass) serve html [588.64ms] waitForServer /tmp/bun-serve-html-txt_rjZL4e { "/": "/tmp/bun-serve-html-txt_rjZL4e/index.html", } [0.15ms] bundle index.html 0.40 KB HASH efbnbska (pass) serve plugins > basic plugin [552.11ms] waitForServer /tmp/html-css-js-failing-plugin_D8NJ2O { "/": "/tmp/html-css-js-failing-plugin_D8NJ2O/index.html", } error: Plugin failed intentionally at /tmp/html-css-js-failing-plugin_D8NJ2O/styles.css:0 error: Plugin failed intentionally at /tmp/html-css-js-failing-plugin_D8NJ2O/styles.css:0 (pass) serve plugins > serve html with failing plugin [505.55ms] waitForServer /tmp/html-css-js-empty-plugins_vK0DVI { "/": "/tmp/htm ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 673ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/7] gen bake.{client,server,error}.js -> bake.client.js, bake.server.js, bake.error.js [2/7] gen generated_host_exports.rs generated_host_exports.rs: 94 exports (host=3, lazy=10, generic=81, rust=0); 240 extern-C blocks audited [2/7] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/bake/DevServer.rs | 63 +++- src/runtime/bake/dev_server/lifecycle.rs | 12 +- src/runtime/bake/dev_server/mod.rs | 401 +++++++++++---------- src/runtime/dispatch.rs | 10 +- .../http/bun-serve-html-hot-reload-drop.test.ts | 82 +++++ test/js/bun/http/bun-serve-html.test.ts | 10 +- 6 files changed, 374 insertions(+), 204 deletions(-) ``` </details> **gate history** · 3 passed · 2 rejected · iteration 4 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/bake/DevServer.rs 10 13 0 src/runtime/bake/dev_server/lifecycle.rs 5 9 0 src/runtime/bake/dev_server/mod.rs 13 12 0 src/runtime/dispatch.rs 3 2 0 test/js/bun/http/bun-serve-html-hot-reload-drop.test.ts 1 5 0 test/js/bun/http/bun-serve-html.test.ts 6 9 0 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-28, 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. |
2 participants
What
ConcurrentTask::default()produces an all-zeros.taskfield, and that is also what the consumer reads when an inlineconcurrent_taskfield's owner is freed while the node is still linked in the concurrent queue. Until now tag value0wastask_tag::Access, so such a node dispatched asAsyncFSTask<_, Access, _>::run_from_js_threadwithself = nulland segfaulted at theself.resultfield offset.This reserves
TaskTag(0)astask_tag::INVALID(real tags now start at 1) and:run_taskpanics on it with a message that says what happeneddebug_assert!at the concurrent-queue drain site catches it one step earlier with the node addressNothing references hard-coded tag values; every
Taskableimpl andNodeFSFunctionEnum::task_tag()use the namedtask_tag::*constants, so the shift is transparent.Why
Seen on Windows 11 aarch64 in build 83933 (
test/js/bun/http/bun-serve-html.test.ts):Symbolicating the trace against the profile PDB:
The faulting instruction is
ldur q0, [x0, #0x48]withx0 = 0. Therun_taskjump table confirms onlytask_tag::Access(tag0) reaches that call site, so the dispatchedTaskwas literally{tag: 0, ptr: null}, which is the all-zeros bit pattern ofConcurrentTask::default().Every enqueue site writes
.taskimmediately before pushing, and the MPSC queue's release/acquire chain checks out, so the zeroed value came from a use-after-free of an inlineconcurrent_taskfield (suspects:DevServer.watcher_atomics.events[*].concurrent_task,FetchTasklet.concurrent_task, or the HTML-bundle abort path). Did not reproduce locally in 80+ runs.This does not fix that UAF; it turns the whole class into a labelled crash so the next occurrence is diagnosable on any platform instead of looking like a spurious null deref in
fs.access.Test
test/internal/concurrent-task-sentinel.test.tsenqueues a zeroedConcurrentTaskvia a test-only hook and asserts the child panics withzeroed ConcurrentTaskrather than segfaulting, and thatfs.promises.access(the former tag-0 type) still dispatches.[review] gate passed · iteration 1 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file