Bug 23

Scripts passed to the node over IPC are decoded in the wrong format

Status

Fixed

Symptom

Crash; worst effect seen: crash

Where seen

Branch-build

Trigger

Normal use; no special event is needed

First reported

2024-12-02 (first report)

Fixed

2025-01-27

Fix

mp#135, bitcoin#31740

Shipped to Bitcoin Core

bitcoin#31740

In released binaries

No

Defect ever on master

Yes

Summary

A CScript passed to bitcoin-node over IPC, for example the coinbase script given to Mining::createNewBlock, was decoded wrongly. libmultiprocess converts each C++ type to and from a Cap'n Proto field with a pair of overloaded functions, one to build the field and one to read it. For scripts the sender wrote raw bytes, but there was no matching read function, so the receiver parsed the bytes with Bitcoin serialization, which expects a length prefix first. @Sjors reported it as a crash while testing a Stratum v2 template provider, and @maflcko reproduced it under valgrind as a memory error in SpanReader::read. The read is bounds-checked, so a release build most likely threw an exception from the server method or decoded a wrong script rather than segfaulting; the exact failure in a normal build was not recorded. No release was affected: v28 had no mining IPC interface and v29 has the fix. Fixed in mp#135 by adding a read function that matches the raw-bytes build function, shipped in bitcoin#31740.

What happened

@Sjors hit a crash in bitcoin-node while testing his Template Provider branch against the Mining interface and filed mp#122 ("Possible crash when (not) writing the compact size of the script/prevector", 2024-12-02), pointing at a comment on an issue in his fork of Bitcoin Core. @maflcko reproduced it the same day by adding mining->createNewBlock({}) to the bitcoin-mine sample client from bitcoin#30437 ("ipc: add bitcoin-mine test program", then still a draft), and posted a valgrind trace with an error in SpanReader::read below CScript::Unserialize. ryanofsky diagnosed it within the hour.

Root cause

Bitcoin Core's src/ipc/capnp/common-types.h had a CustomBuildField overload that sent any type convertible to a byte span as a raw capnp::Data field. It was added for GCSFilter::ElementSet and later used for CScript and others, and it had no matching CustomReadField; its own comment said so. On the receiving side, overload resolution fell through to the lower-priority generic overload for types with an Unserialize method, which expects Bitcoin's serialization format: a CompactSize length followed by the script bytes. The reader took the first script byte as the length. SpanReader::read checks bounds, so a length past the end of the data throws SpanReader::read(): end of data, and a shorter length decodes a wrong script.

CScript was the first type sent over IPC that is both convertible to a byte span and has Unserialize. No merged code called createNewBlock with a script over IPC, so only an external client or the draft bitcoin-mine could hit it. Earlier users of the build overload were types whose generic read path happened to be compatible.

The code

The defect is in Bitcoin Core's src/ipc/capnp/common-types.h, quoted from 2221c8814d7^, the Bitcoin Core commit that replaced it with the libmultiprocess fix. The sending side writes any byte-span-like value, such as a CScript, as its raw bytes:

template <typename LocalType, typename Value, typename Output>
void CustomBuildField(TypeList<LocalType>, Priority<2>, InvokeContext& invoke_context, Value&& value, Output&& output)
requires
    (std::is_same_v<decltype(output.get()), ::capnp::Data::Builder>) &&
    (std::convertible_to<Value, std::span<const std::byte>> ||
     std::convertible_to<Value, std::span<const char>> ||
     std::convertible_to<Value, std::span<const unsigned char>> ||
     std::convertible_to<Value, std::span<const signed char>>)
{
    auto data = std::span{value};
    auto result = output.init(data.size());
    memcpy(result.begin(), data.data(), data.size());
}

There is no matching CustomReadField, so the receiving side picks the best remaining overload, which in the same file is the one for any type with an Unserialize method:

template <typename LocalType, typename Input, typename ReadDest>
decltype(auto) CustomReadField(TypeList<LocalType>, Priority<1>, InvokeContext& invoke_context, Input&& input, ReadDest&& read_dest)
requires Unserializable<LocalType, DataStream> && (!ipc::capnp::Deserializable<LocalType>)
{
    return read_dest.update([&](auto& value) {
        // ...
        SpanReader stream({data.begin(), data.end()});
        auto wrapper{ipc::capnp::Wrap(stream)};
        value.Unserialize(wrapper);
    });
}

CScript::Unserialize expects Bitcoin's serialization format, a CompactSize length followed by the bytes, so it reads the script's first byte as the length. A length past the end of the buffer throws SpanReader::read(): end of data, and a shorter one silently produces a different, truncated script.

The fix (7d59b8d14c86) moves the build overload into libmultiprocess as include/mp/type-data.h, limited to types that can also be constructed from a byte range, and adds the matching reader (concept definitions and the unchanged build overload omitted):

+template <typename LocalType, typename Input, typename ReadDest>
+decltype(auto) CustomReadField(TypeList<LocalType>, Priority<2>, InvokeContext& invoke_context, Input&& input, ReadDest&& read_dest)
+requires (std::is_same_v<decltype(input.get()), ::capnp::Data::Reader> && IsByteSpan<LocalType>)
+{
+    using ByteType = decltype(std::span{std::declval<LocalType>().begin(), std::declval<LocalType>().end()})::element_type;
+    const kj::byte *begin{input.get().begin()}, *end{input.get().end()};
+    return read_dest.construct(reinterpret_cast<const ByteType*>(begin), reinterpret_cast<const ByteType*>(end));
+}

Both overloads now have the same IsByteSpan<LocalType> constraint and Priority<2>, so any type written as raw bytes is also read back as raw bytes, and the Priority<1> Unserialize reader is never chosen for it.

Fix

The fix commit 7d59b8d14c86, part of mp#135 ("refactor: proxy-types.h API cleanup"), moves the overload into libmultiprocess as include/mp/type-data.h. It restricts the Data build overload to types that are both convertible to and constructible from a byte span, and adds a matching Priority<2> CustomReadField that constructs the value from the raw bytes, so both sides always use the same encoding. bitcoin#31740, the update that brought the fix into Bitcoin Core, adds a passScript test case to src/test/ipc_test.cpp.

@maflcko argued that mismatched build and read overloads should fail to compile rather than be fixed one type at a time; ryanofsky answered that the problem came from this one unpaired overload, and pairing it is what landed.

Timeline

• 2024-09-25: the unpaired span CustomBuildField reaches Bitcoin Core master in 06882f84017f ("multiprocess: Add serialization code for vector"), merged as part of bitcoin#30510 ("multiprocess: Add IPC wrapper for Mining interface").
• 2024-12-02: mp#122 filed by @Sjors; reproduced by @maflcko.
• 2025-01-27: fix merged in mp#135, closing mp#122.
• 2025-01-29: shipped to Bitcoin Core in bitcoin#31740 ("depends: Update libmultiprocess library before converting to subtree") with the regression test.
• Releases: none affected.

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.