container: DockerPort should hold a strong ref to ContainerClient (mirror DockerProcessHandle)

[Lougarou]

Context. This is a defensive-hardening change that came out of a private security report to Cloudflare, which was reviewed and closed as Informative — there is no reachable production trigger for a crash — and the triager encouraged opening the fix and its ASAN regression test here as a maintenance improvement, and flagging the dead ActorContainer::containerClient member along with it. No exploitable path is claimed.

ContainerClient::DockerPort holds its owner as a bare ContainerClient& and dereferences it (containerClient.network, containerClient.byteStreamFactory) after the co_await points in connect() (parse the ingress address, connect to the sidecar, read the CONNECT status). The sibling capability server DockerProcessHandle — created from exec() in the same file — holds its owner as a kj::Own<ContainerClient> via containerClient.addRef() and even attaches a ref to its background task, precisely so it can outlive teardown races. DockerPort does not.

Today DockerPort's bare reference is safe only because of an unenforced coupling: nothing drops the last ContainerClient reference while a connect() is suspended. In the normal actor-teardown paths the JS-held Port capability is dropped first, which cancels the in-flight connect() before the ContainerClient is freed (container.capnp sets a file-level $Cxx.allowCancellation, so the cancellation does unwind the coroutine). Nothing in the type system enforces that ordering, though. Any future path that frees the ContainerClient without first cancelling an in-flight connect() — for example wiring up the currently-dead ActorContainer::containerClient member (see below), or introducing a co_await between actor destruction and the container-client drop — resumes connect() into freed memory.

This PR makes DockerPort hold containerClient.addRef() (a kj::Own<ContainerClient>), exactly like DockerProcessHandle. It removes the dependency on teardown ordering, is a no-op in the normal case, and does not create a reference cycle: getTcpPort() hands the DockerPort to the RPC layer via results.setPort() and ContainerClient keeps no reference back to it, so the new edge is released when the client drops the Port capability (identical to how DockerProcessHandle already behaves).

This fix is independent of the exact body of connect(): it holds the reference structurally, so it stays correct under refactors that relocate the connect logic (e.g. into a ContainerClient helper) as long as DockerPort keeps borrowing ContainerClient.

Regression test

container-client-test.c++ adds KJ_TEST("DockerPort connect() use-after-free on ContainerClient freed mid-call"). It vends a DockerPort via getTcpPort, drives connect() to its first suspension point (a SuspendNetwork whose parseAddress parks on a fulfiller), drops the only strong ContainerClient reference, then resumes connect(). Under ASAN, with a bare ContainerClient& the test aborts with a heap-use-after-free in DockerPort::connect() (.resume) (freed by ~ContainerClient() when the last Container::Client capability reference is dropped mid-call); with the addRef() fix it passes. The test runs no JavaScript, so it exercises only the kj/capnp capability-lifetime path.

A one-line test-only accessor, ContainerClient::setSidecarIngressHostPortForTest, lets the test drive connect() without a live sidecar container.

bazel test //src/workerd/server:container-client-test --config=asan --test_output=all

Separate observation: the dead ActorContainer::containerClient member

While tracing the container lifetime for this fix, ActorContainer::containerClient (server.c++, declared kj::Maybe<kj::Own<ContainerClient>>) appears to be dead: it is never assigned a value anywhere in server.c++; the containerClient = kj::none; statements are no-ops on an always-empty Maybe, and the comments around them describe ownership that is not wired up. The live container-client references are the raw ContainerClient* entries in the containerClients HashMap and the Own handed to the actor. Flagging it because it is exactly the kind of member that, if "fixed" to actually own a ContainerClient without also giving DockerPort a strong ref, would make the connect() use-after-free above reachable. Happy to split this into a separate issue/PR if the team prefers.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[Lougarou]

I have read the CLA Document and I hereby sign the CLA