fix use-after-free in ContainerClient decodeJsonResponse()

[anonrig]

Added some basic tests with the help of AI

[ask-bonk]

Good — @capnp-cpp//src/capnp/compat:json is a transitive dependency of the :container-client target. The include will resolve correctly.

Now let me check if there's a potential issue: the capnp/compat/json.h and capnp/message.h includes are now in the header, which means any translation unit that includes container-client.h will also pull these in. Let me check if these are already available via the header's existing deps or if they need to be listed explicitly in the BUILD:

The :container-client target already listed @capnp-cpp//src/capnp/compat:http-over-capnp which likely transitively includes capnp message types, and the json dep works transitively through //src/workerd/io. However, the test target only lists :container-client and @capnp-cpp//src/kj as deps. It doesn't list @capnp-cpp//src/capnp/compat:json directly. But since :container-client has it as a transitive dep, it should be fine for Bazel's C++ compilation (headers + link).

The fix is correct and clean. Let me review the full change one more time for any subtle issues:

1. Memory safety fix: Correct. The old code returned a Builder (a view/pointer) into a destroyed MallocMessageBuilder. The new code returns a kj::Own<MallocMessageBuilder> that the caller keeps alive.

2. Template in header: Necessary since the function is now used in both container-client.c++ and the test file. The inline nature of templates avoids ODR violations.

3. API change: The callers now get kj::Own<capnp::MallocMessageBuilder> and call message->getRoot<T>() themselves. This is a clean, straightforward pattern.

4. Tests: Cover all 3 response types used in the codebase. The tests are regression tests that would catch the UAF under ASAN.

5. No behavioral changes: The fix only changes internal memory management — no user-visible behavior changes, so no compat flag is needed.

This is a straightforward, well-executed UAF fix. The code is correct, tests are appropriate, and there are no issues to flag.

LGTM

github run