Restore strong captures in internal stream read callbacks to prevent UAF

[dknecht]

Motivation

• A code review found that async completion lambdas in ReadableStreamInternalController::read() and ::drainingRead() captured raw this across awaitIoLegacy(...).then(...), allowing a pending read promise to run a callback on a destroyed controller and cause a use-after-free.
• The intent is to ensure the controller/owning stream remains alive for the duration of the async read callback without changing observable stream behavior.

Description

• Added a strong reference capture ref = addRef() to the fulfillment and rejection lambdas passed to awaitIoLegacy(...).then(...) in ReadableStreamInternalController::read() so the controller is kept alive until the callback runs.
• Added the same ref = addRef() capture to the fulfillment and rejection lambdas in ReadableStreamInternalController::drainingRead() to mirror the fix for draining reads.
• The only modified file is src/workerd/api/streams/internal.c++ and the change is limited to the lambda capture lists; no behavioral logic was altered.

Testing

• Attempted to run Bazel-related build/test commands (e.g. bazel query / bazel test) but the environment blocked Bazel bootstrap download (HTTP Forbidden), so no build or tests could be executed here.
• No automated tests were run in this environment; recommend running just test or bazel test //src/workerd/... and CI checks to validate the change in a network-enabled environment.

---

Codex Task

[jasnell]

We can't land this just yet. There's a conflicting change on the internal repo that needs to clear through first.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

This PR adds ref = addRef() captures to four lambdas in ReadableStreamInternalController::read() and ::drainingRead() to prevent the owning ReadableStream from being GC'd while async I/O callbacks are pending.

Issues

1. [HIGH] Build failure: -Wunused-lambda-capture — workerd enables -Wunused-lambda-capture (.bazelrc:unix) and -Werror (CI). The ref variable is captured but never used in the lambda body, so all four lambdas will produce compiler errors. The established pattern in standard.c++ uses JSG_VISITABLE_LAMBDA which marks captures as GC-visitable (avoiding the warning). Using plain lambdas requires suppressing the warning.

2. [MEDIUM] jsg::Ref inside addFunctor is not GC-traceable — addFunctor wraps the lambda in IoOwn<kj::Own<Lambda>>, hiding the jsg::Ref from V8's GC tracer. While jsg::Ref uses CppGC reference counting (so it still prevents collection), this diverges from the pattern in standard.c++ which uses JSG_VISITABLE_LAMBDA to make refs GC-visible. Consider using JSG_VISITABLE_LAMBDA inside addFunctor for consistency with the rest of the streams codebase.

3. [LOW] No tests were run — the PR description notes that no build or tests were executed. This change is in a critical code path (stream reads). CI should validate this.

---

This review was generated by an AI assistant and may contain inaccuracies.

[ask-bonk]

[HIGH] Build will fail: workerd enables -Wunused-lambda-capture in .bazelrc and -Werror in CI. The ref variable is captured but never read in the lambda body, producing a compilation error.

The established pattern in standard.c++ for keeping the owning stream alive uses JSG_VISITABLE_LAMBDA, which lists ref in its visitable-fields tuple (making it "used" by the generated visitForGc method and also properly GC-traceable). For example, from standard.c++:4131:

auto onSuccess = JSG_VISITABLE_LAMBDA((this, ref = addRef(), preventCancel, pipeThrough), (ref), ...);

You should either:

1. Preferred: Switch to JSG_VISITABLE_LAMBDA inside addFunctor to match the standard controller pattern.
2. Minimal: Add (void)ref; at the top of each lambda body to suppress the warning.
3. Alternative: Restructure to use .attach(addRef()) on the promise (similar to WritableStreamInternalController::ensureWriting() at line 1515).

[ask-bonk]

[HIGH] Same -Wunused-lambda-capture issue here — ref is captured but never used. Will fail CI.

Minimal fix if not switching to JSG_VISITABLE_LAMBDA:

Suggested change

	[this, ref = addRef()](jsg::Lock& js,
	jsg::Value reason) -> jsg::Promise<ReadResult> {
	[this, ref = addRef()](jsg::Lock& js,
	jsg::Value reason) -> jsg::Promise<ReadResult> {
	(void)ref;

[ask-bonk]

[HIGH] Same -Wunused-lambda-capture issue — ref captured but unused.

[ask-bonk]

[HIGH] Same -Wunused-lambda-capture issue — ref captured but unused.

[ask-bonk]

I'm Bonk, and I've done a quick review of your PR.

The review has been posted to PR #6202 with 4 inline comments. Here's a summary:

1. [HIGH] Build failure: -Wunused-lambda-capture — All four ref = addRef() captures are unused in the lambda bodies. workerd enables -Wunused-lambda-capture in .bazelrc and -Werror in CI, so this will fail to compile. The established pattern in standard.c++ uses JSG_VISITABLE_LAMBDA which lists ref in its visitable-fields tuple (avoiding the warning and making the ref GC-traceable). A minimal fix would be (void)ref; in each lambda body, but the preferred approach is switching to JSG_VISITABLE_LAMBDA.

2. [MEDIUM] jsg::Ref inside addFunctor not GC-traceable — addFunctor wraps the lambda in IoOwn<kj::Own<Lambda>>, hiding the jsg::Ref from V8's GC tracer. While jsg::Ref uses CppGC reference counting (so it still prevents collection functionally), this diverges from the pattern in standard.c++.

3. [LOW] No tests run — The PR description notes Bazel was blocked by network restrictions. CI needs to validate this change.

github run