HTMLRewriter: fix use-after-free when GC sweeps an abandoned transform before its stream sink controller

[robobun]

Fixes an ASAN heap-use-after-free found by fuzzing on current main (new in the window that added the HTMLRewriter SinkHandle/SourceHandle streaming path, #36733; older builds throw ERR_STREAM_CANNOT_PIPE before reaching it).

Repro

function once() {
  const rs = new ReadableStream({
    pull(c) { c.enqueue(new TextEncoder().encode("<p>x</p>")); return new Promise(() => {}); },
  });
  new HTMLRewriter().on("p", { element(e) {} }).transform(new Response(rs));
}
for (let round = 0; round < 20; round++) {
  for (let i = 0; i < 200; i++) once();
  await Bun.sleep(0);
  Bun.gc(true);
}
console.log("done");

On an ASAN build this aborts within the first few GC rounds:

SUMMARY: AddressSanitizer: heap-use-after-free src/runtime/webcore/Sink.rs:575:55 in JSSink<RewriterPipe>::js_controller_detached

ACCESS is HTMLRewriterSink__controllerDetached called from ~JSReadableHTMLRewriterSinkController; FREE is drop<Box<RewriterPipe>> in the JSHTMLRewriterTransform cell finalizer.

Cause

transform() over a plain JS ReadableStream takes the JS-pump fallback: assignToStream creates a JSReadableHTMLRewriterSinkController whose m_sinkPtr points at the RewriterPipe raw, and the controller destructor dispatches __controllerDetached and __finalize into that pointer unless something nulled it first. The pipe itself is owned by the JSHTMLRewriterTransform cell, whose finalizer frees the Box.

When the rewrite is abandoned mid-stream (input never closes, output dropped), the Transform cell, the input stream, the pump, and the controller all become garbage in the same GC cycle, and JSC sweeps those cells in unspecified order. Transform cell first means the controller destructor then reads (and on the matching path writes) the freed pipe.

The suspension hand-off had the same shape: a suspended transform abandoned with a JS-pump input could have the deferred abandon_suspension task free the pipe while the controller destructor still held the raw pointer, in either order.

Fix

RewriterPipe now tracks its owners in a small claim count: the Transform cell (from init until its finalizer), the JS-pump controller (from assign_to_stream until __controllerDetached, which fires exactly once on every detach path including the destructor), and a parked suspension's reaction or abandon task. Whoever releases the last claim frees the Box.

A controller-origin release never frees inline because the releasing C++ frame keeps calling into the allocation after it returns (the destructor calls __finalize next; the close/end host fns call __close/__endWithSink on the saved pointer), so that case defers the free to an event-loop task, reusing the existing DeferredDerefTask used by the abandon path.

JsSinkType gains a default no-op controller_detached hook so the generic js_controller_detached can notify the sink; behavior for every other sink type is unchanged.

Verification

• Unfixed debug ASAN build: repro aborts with the stack above, 3/3 runs; the new test fails the same way.
• Fixed build: repro prints done, 5/5 runs; new test passes.
• test/js/workerd/html-rewriter-leak.test.ts (includes the suspension/abandon lifetime tests), html-rewriter.test.js, html-rewriter-end-error.test.ts, html-rewriter-doctype.test.ts, htmlrewriter-additional-bugs.test.ts: all pass (154 tests).
• test/js/web/fetch/body-stream.test.ts (9086) and test/js/bun/util/filesink.test.ts (57) pass, covering the shared sink-controller machinery.

The new test is skipIf(!isASAN): the stale read is only observable under ASAN.

---

no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/workerd/html-rewriter-leak.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 69968418-9a72-4a0b-90af-01166379246e

📥 Commits

Reviewing files that changed from the base of the PR and between 3da737d and 0487f46.

📒 Files selected for processing (3)

• src/runtime/api/NativePromiseContext.rs
• src/runtime/api/html_rewriter.rs
• src/runtime/webcore/Sink.rs

Walkthrough

Changes

The change adds explicit RewriterPipe ownership claims for transform cells, JS-pump controllers, and suspended handlers. It adds deferred pipe destruction, controller-detachment callbacks, and an ASAN regression test.

HTMLRewriter pipe lifetime

Layer / File(s)	Summary
Deferred pipe task handling src/jsc/bindings/NativePromiseContext.h, src/runtime/api/NativePromiseContext.rs	Adds the HTMLRewriterPipeFree task tag and dispatches deferred RewriterPipe destruction.
Pipe ownership lifecycle src/runtime/api/html_rewriter.rs, src/runtime/webcore/Sink.rs	Tracks claims held by transforms, controllers, and suspended handlers. Releases claims during finalization, detachment, abandonment, and promise reactions.
Abandoned transform validation test/js/workerd/html-rewriter-leak.test.ts	Adds ASAN-only coverage for abandoned transforms backed by never-closing streams.

Possibly related PRs

• oven-sh/bun#36733: Introduces related RewriterPipe suspension and deferred-cleanup mechanisms.
• oven-sh/bun#36783: Also changes Sink.rs lifecycle handling for controller detachment.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the HTMLRewriter use-after-free fix caused by garbage collection of an abandoned transform.
Description check	✅ Passed	The description explains the cause, fix, reproduction, and verification, although it does not use the template headings.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 4

🤖 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 `@src/runtime/api/html_rewriter.rs`:
- Around line 782-789: Update finalize so release_claim() is evaluated before
the shutdown check, using the combined release-or-shutdown condition. Preserve
the existing Box::into_raw leak path when either condition succeeds, including
during shutdown, and only drop the controller when both conditions are false.

In `@src/runtime/api/NativePromiseContext.rs`:
- Around line 243-247: Update the SAFETY comment above the match containing
HTMLRewriterSuspension and HTMLRewriterPipeFree to describe the actual ownership
invariant: the scheduling site holds the pipe claim, that claim is the last one,
and no cell, controller, or task can dispatch into the allocation. Replace the
inaccurate intrusive-refcounted-pointer statement while preserving the existing
safety details for other tags.
- Around line 66-67: Add a compile-time assertion near Tag::COUNT that verifies
the maximum tag value (Tag::COUNT - 1) fits within DeferredDerefTask::TAG_MASK,
causing compilation to fail if additional tags exceed the packed-pointer
capacity.

In `@src/runtime/webcore/Sink.rs`:
- Around line 296-302: Update the trait documentation for controller_detached to
explicitly state that implementations must not free the sink allocation inline
during the callback; defer any last-owner deallocation until after the caller
returns. Keep the existing ownership-release guidance and callback behavior
unchanged.

🪄 Autofix

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: eacc88a5-a79c-4484-998b-985afb8f6a9e

📥 Commits

Reviewing files that changed from the base of the PR and between 45eda51 and 3da737d.

📒 Files selected for processing (5)

• src/jsc/bindings/NativePromiseContext.h
• src/runtime/api/NativePromiseContext.rs
• src/runtime/api/html_rewriter.rs
• src/runtime/webcore/Sink.rs
• test/js/workerd/html-rewriter-leak.test.ts

[robobun]

^{Updated 12:08 AM PT - Aug 7th, 2026}

@robobun, your commit 0487f46 is building: #89973

[claude]

The earlier shutdown-path finding is fixed (4ed8406 drops the is_shutting_down() gate so finalize always runs release_claim() and leaks under a live claim instead of freeing). No further issues found, but this is a manual claim-count over three GC-swept owners with deferred frees — worth a human pass on the ownership reasoning.

What was reviewed:

• Claim acquire/release pairing: init(1) + assign_to_stream + begin_suspension vs. finalize / release_pump_claim / abandon_suspension / on_handler_{resolve,reject} — balanced on every path traced.
• All four C++ __controllerDetached call sites (close/end host fns, controller destructor, detach()) keep using the saved pointer afterward, which is why release_pump_claim defers the last-owner free — verified against generate-jssink.ts.
• assign_to_stream error path: detach_ptr reaches detach() → __controllerDetached → release_pump_claim, so the pre-acquired pump claim is released.

Extended reasoning...

Overview

Fixes a heap-use-after-free in the HTMLRewriter streaming path where the JSHTMLRewriterTransform cell and the JS-pump JSReadableHTMLRewriterSinkController become garbage in the same GC cycle and sweep in unspecified order; the controller destructor dispatches into the RewriterPipe that the Transform cell's finalizer already freed. The fix replaces single-owner Box drop with a small claim count (Transform cell, pump controller, parked suspension) where the last releaser frees, and controller-origin last-owner frees are deferred to the event loop because the C++ frame keeps using m_sinkPtr after __controllerDetached returns. Touches html_rewriter.rs (claim machinery), Sink.rs (new default controller_detached hook on JsSinkType), NativePromiseContext.{rs,h} (new HTMLRewriterPipeFree deferred-task tag), and adds an ASAN-gated regression test.

Security risks

None identified. This is internal lifetime management with no user-facing surface, input parsing, or trust-boundary changes. The failure mode being fixed is a UAF (memory-safety), not an auth/injection concern.

Level of scrutiny

High. Per REVIEW.md this is squarely in the "most-blocked category" — native memory safety, GC-swept cell finalization ordering, manual owner counting with unsafe frees, and a new hook on shared JsSinkType infrastructure that every sink implements (default no-op, so other sinks are behaviorally unchanged). The reasoning is subtle: correctness depends on __controllerDetached firing exactly once on every detach path (verified against the four generate-jssink.ts call sites), on DeferredDerefTask not draining synchronously inside the dispatching frame, and on the suspension reaction/abandon task each releasing the begin_suspension claim exactly once.

Other factors

My previous inline finding (the is_shutting_down() short-circuit in finalize that bypassed release_claim() and reintroduced the same UAF on worker termination) was addressed in 4ed8406 — finalize now unconditionally decrements and leaks (Box::into_raw) when other claims remain, matching the leak-on-shutdown convention already used by DeferredDerefTask::schedule. The CodeRabbit nits (compile-time Tag::COUNT <= TAG_MASK+1 assert, SAFETY comment for the non-refcounted tags, no-inline-free contract on the trait doc) are all present in the current diff. The comment-cop warnings are all resolved. The new test is skipIf(!isASAN) because the stale read is only observable under ASAN; the PR description reports it fails 3/3 on unfixed and passes 5/5 on fixed builds, plus 154 existing HTMLRewriter tests and the shared sink-controller suites pass. Given the change reworks ownership across GC cells, a human reviewer should confirm the claim-count invariants independently rather than rely on this trace.