Bugfix - Fix WorkerEntrypoint use-after-free on subrequest cancellation#6867
Open
jutaz wants to merge 1 commit into
Open
Bugfix - Fix WorkerEntrypoint use-after-free on subrequest cancellation#6867jutaz wants to merge 1 commit into
jutaz wants to merge 1 commit into
Conversation
`WorkerEntrypoint::requestImpl` is a coroutine whose KJ_DEFER at scope exit runs `incomingRequest->drain(waitUntilTasks, ...)`. The DEFER is invoked either at natural scope exit (with `this` alive) or from the coroutine's destructor. On some subrequest paths the coroutine's destructor fires after the `WorkerEntrypoint` has already been freed by its owning `Own<WorkerInterface>`: the promise chain that owns the coroutine can outlive that Own via the response body's attached task. The trigger observed in practice is a fire-and-forget `env.QUEUE.send()` followed by a 3xx response — the queue producer's loopback subrequest lands its `WorkerEntrypoint::requestImpl` coroutine in `IoContext::tasks`, and when the outer request unwinds that TaskSet the coroutine's DEFER reads `this->waitUntilTasks` from freed memory, gets a null `TaskSet&`, and faults inside `TaskSet::add`. Repro binaries either segfault (bazel test harness) or spin at 100% CPU indefinitely (standalone workerd / miniflare). Bind `waitUntilTasks` to a coroutine-frame local so the DEFER's implicit `[&]` capture pins the reference against the coroutine frame instead of `this`. `WorkerService::waitUntilTasks` itself lives for the service's lifetime, so the underlying `TaskSet` is always valid. The DEFER still reads other `this->*` members (`failOpenService`, `proxyTask`, `loggedExceptionEarlier`, `abortController`, `cfBlobJson`); those are latent UAFs on the same path but haven't been observed to crash — zeroed freed memory happens to steer the DEFER's control flow past them. A proper fix would tie the entrypoint's lifetime to the coroutine's; leaving a `TODO(cleanup)`. Includes a `wd_test` regression that exercises the exact combination (fire-and-forget `env.QUEUE.send()` + `302`) via a self-service binding, so it reproduces inside a single worker with no miniflare.
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WorkerEntrypoint::requestImplis a coroutine whose KJ_DEFER at scope exit runsincomingRequest->drain(waitUntilTasks, ...). The DEFER is invoked either at natural scope exit (withthisalive) or from the coroutine's destructor. On some subrequest paths the coroutine's destructor fires after theWorkerEntrypointhas already been freed by its owningOwn<WorkerInterface>: the promise chain that owns the coroutine can outlive that Own via the response body's attached task.The trigger observed in practice is a fire-and-forget
env.QUEUE.send()followed by a 3xx response — the queue producer's loopback subrequest lands itsWorkerEntrypoint::requestImplcoroutine inIoContext::tasks, and when the outer request unwinds that TaskSet the coroutine's DEFER readsthis->waitUntilTasksfrom freed memory, gets a nullTaskSet&, and faults insideTaskSet::add. Repro binaries either segfault (bazel test harness) or spin at 100% CPU indefinitely (standalone workerd / miniflare).Bind
waitUntilTasksto a coroutine-frame local so the DEFER's implicit[&]capture pins the reference against the coroutine frame instead ofthis.WorkerService::waitUntilTasksitself lives for the service's lifetime, so the underlyingTaskSetis always valid.The DEFER still reads other
this->*members (failOpenService,proxyTask,loggedExceptionEarlier,abortController,cfBlobJson); those are latent UAFs on the same path but haven't been observed to crash — zeroed freed memory happens to steer the DEFER's control flow past them. A proper fix would tie the entrypoint's lifetime to the coroutine's; leaving aTODO(cleanup). Happy to refactor this to address the TODO as well.Includes a
wd_testregression that exercises the exact combination (fire-and-forgetenv.QUEUE.send()+302) via a self-service binding, so it reproduces inside a single worker with no miniflare (where this was originally observed).