From d6f8588d1ab8195c72a24359d0e67f9f6c0f11b2 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 8 Jul 2026 13:03:00 -0400 Subject: [PATCH] proxy: fix BuildList to use non-const iteration for interface types BuildList turns a C++ container into a capnp list. It looped over the container with `for (const auto& elem : value)`, so every element was const. That breaks when the elements are interface pointers: to send one to the capnp server the proxy code has to take ownership away from the unique_ptr by calling unique_ptr::release(), and that cannot be called on a const object. So the code failed to compile, and returning a list of interface pointers stopped working after #277. Fix this by looping with a plain iterator and passing each element to BuildField as non-const, only moving elements out of the container when the container itself is a temporary. See the code comment for details. In Bitcoin Core, interfaces::Node::listExternalSigners() returns a vector> over IPC and hits this code path. Add a `listBars` method to the test FooInterface that returns a vector>, plus a test that calls it over IPC, so this is covered and does not break again. Co-Authored-by: Sjors Provoost Co-Authored-By: Claude Opus 4.8 --- include/mp/proxy-types.h | 25 ++++++++++++++++++++++--- include/mp/type-map.h | 2 ++ src/mp/gen.cpp | 1 + test/mp/test/foo-types.h | 1 + test/mp/test/foo.capnp | 6 ++++++ test/mp/test/foo.h | 24 ++++++++++++++++++++++++ test/mp/test/test.cpp | 10 ++++++++++ 7 files changed, 66 insertions(+), 3 deletions(-) diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index 02e8eefb..3a5dd3d8 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -285,9 +285,28 @@ void BuildList(TypeList, InvokeContext& invoke_context, Output&& outp { auto list = output.init(value.size()); size_t i = 0; - for (const auto& elem : value) { - BuildField(TypeList(), invoke_context, ListOutput(list, i), elem); - ++i; + // Iterate with an explicit iterator rather than a range-for loop so the + // value category of `*it` is passed through to BuildField unchanged. This + // matters for two reasons: + // + // - Elements must be passed as non-const so that BuildField can move out of + // them, e.g. calling unique_ptr::release() to transfer ownership of an + // interface pointer to the capnp server. + // + // - For proxy containers like std::vector, `*it` is a prvalue proxy + // object rather than a reference. A range-for loop would bind it to a + // named variable and demote it to an lvalue; passing `*it` directly + // preserves the prvalue-ness. + // + // Only move out of elements when `value` itself is an rvalue container that + // is about to be destroyed. When it is an lvalue reference owned by the + // caller, pass elements as lvalues so BuildField does not move from them. + for (auto it = value.begin(); it != value.end(); ++it, ++i) { + if constexpr (std::is_lvalue_reference_v) { + BuildField(TypeList(), invoke_context, ListOutput(list, i), *it); + } else { + BuildField(TypeList(), invoke_context, ListOutput(list, i), std::move(*it)); + } } } diff --git a/include/mp/type-map.h b/include/mp/type-map.h index e00a7813..ccfefb87 100644 --- a/include/mp/type-map.h +++ b/include/mp/type-map.h @@ -9,6 +9,8 @@ #include #include +#include + namespace mp { template void CustomBuildField(TypeList>, diff --git a/src/mp/gen.cpp b/src/mp/gen.cpp index 7733eba6..8d733a91 100644 --- a/src/mp/gen.cpp +++ b/src/mp/gen.cpp @@ -329,6 +329,7 @@ static void Generate(kj::StringPtr src_prefix, cpp_client << "#include \n"; cpp_client << "#include \n"; cpp_client << "#include \n"; + cpp_client << "#include \n"; cpp_client << "#include \n"; cpp_client << "#include \n"; cpp_client << "#include \n"; diff --git a/test/mp/test/foo-types.h b/test/mp/test/foo-types.h index 1bc6c523..ee12f490 100644 --- a/test/mp/test/foo-types.h +++ b/test/mp/test/foo-types.h @@ -39,6 +39,7 @@ struct ExtendedCallback; // IWYU pragma: export struct FooCallback; // IWYU pragma: export struct FooFn; // IWYU pragma: export struct FooInterface; // IWYU pragma: export +struct BarInterface; // IWYU pragma: export } // namespace messages template diff --git a/test/mp/test/foo.capnp b/test/mp/test/foo.capnp index 18df85aa..11bfd9c5 100644 --- a/test/mp/test/foo.capnp +++ b/test/mp/test/foo.capnp @@ -37,6 +37,7 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") { callFnAsync @18 (context :Proxy.Context) -> (); callIntFnAsync @21 (context :Proxy.Context, arg :Int32) -> (result :Int32); passDataPointers @22 (arg :List(Data)) -> (result :List(Data)); + listBars @25 (context :Proxy.Context, n :Int32) -> (result :List(BarInterface)); } interface FooInit $Proxy.wrap("mp::test::FooInit") { @@ -52,6 +53,11 @@ interface ExtendedCallback extends(FooCallback) $Proxy.wrap("mp::test::ExtendedC callExtended @0 (context :Proxy.Context, arg :Int32) -> (result :Int32); } +interface BarInterface $Proxy.wrap("mp::test::Bar") { + destroy @0 (context :Proxy.Context) -> (); + value @1 (context :Proxy.Context) -> (result :Int32); +} + interface FooFn $Proxy.wrap("ProxyCallback>") { destroy @0 (context :Proxy.Context) -> (); call @1 (context :Proxy.Context) -> (result :Int32); diff --git a/test/mp/test/foo.h b/test/mp/test/foo.h index 01566c58..8fc34eee 100644 --- a/test/mp/test/foo.h +++ b/test/mp/test/foo.h @@ -71,6 +71,23 @@ class FooInit { }; +//! A second, arbitrary interface used to test returning a +//! list of interface objects. +class Bar +{ +public: + virtual ~Bar() = default; + virtual int value() = 0; +}; + +//! Concrete Bar that returns a fixed value, used by listBars tests. +class SimpleBar : public Bar +{ +public: + explicit SimpleBar(int value) : m_value(value) {} + int value() override { return m_value; } + int m_value; +}; class FooImplementation { public: @@ -96,6 +113,13 @@ class FooImplementation double passDouble(double value) { return value; } int passFn(std::function fn) { return fn(); } std::vector passDataPointers(std::vector values) { return values; } + std::vector> listBars(int n) + { + std::vector> result; + result.reserve(n); + for (int i = 0; i < n; ++i) result.push_back(std::make_unique(i)); + return result; + } std::shared_ptr m_callback; void callFn() { assert(m_fn); m_fn(); } void callFnAsync() { assert(m_fn); m_fn(); } diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp index 7af1365e..400aff73 100644 --- a/test/mp/test/test.cpp +++ b/test/mp/test/test.cpp @@ -272,6 +272,16 @@ KJ_TEST("Call FooInterface methods") KJ_REQUIRE(data_out[0] != nullptr); KJ_EXPECT(*data_out[0] == *data_in[0]); KJ_EXPECT(!data_out[1]); + + // Test returning vector> from server. This exercises + // BuildList with interface element types, which requires non-const iteration + // so unique_ptr::release() can transfer ownership to the proxy server. + std::vector> bars{foo->listBars(3)}; + KJ_REQUIRE(bars.size() == 3u); + for (int i = 0; i < 3; ++i) { + KJ_REQUIRE(bars[i] != nullptr); + KJ_EXPECT(bars[i]->value() == i); + } } KJ_TEST("Call IPC method after client connection is closed")