Audit report AR-13

Reported privately in an independent security review, 2026-08-07.

Report date

2026-08-07

Kind

Hardening

Status on master

Present

Proposed issue state

Open

Summary

ReadDestUpdate::construct destroys the caller's output object before constructing its replacement in the same storage. If the replacement's constructor throws, the object is left destroyed, and it is destroyed a second time when the caller's stack unwinds. The mechanism is real, but on Bitcoin Core master it takes an allocation failure to trigger; the one realistic trigger (a malformed transaction reply) exists only on the unmerged multiprocess branch. Hardening with a cheap fix.

The report

ReadDestUpdate in include/mp/proxy-types.h is the read destination used when a value is read into an existing object. Its construct method does:

m_value.~Value();
new (&m_value) Value(args...);

If the placement-new constructor throws, the old value has already been destroyed and no new value exists. Normal unwinding later runs the destructor of m_value again, a double destroy that for a heap-owning type is a double free. The report says that built-in primitive replacements usually do not throw, but that custom messages, pair and tuple reconstruction, validating constructors and allocation failures make the generic template reachable, and that a hostile decoded value could trigger it.

The reproduction is a standalone C++ program, not a test against the library. It copies the destroy-then-placement-new pattern into a small ReadDestUpdateModel and uses it on an OwnedBuffer type that holds a malloced buffer and whose constructor throws for the input 31337. AddressSanitizer reports attempting double-free.

Proposed change

Construct a temporary first and assign it only after construction succeeds; never destroy caller-owned output storage before the replacement is complete:

     template <typename... Args>
     Value& construct(Args&&... args)
     {
-        m_value.~Value();
-        new (&m_value) Value(std::forward<Args>(args)...);
+        Value replacement(std::forward<Args>(args)...);
+        m_value = std::move(replacement);
         return m_value;
     }

It also suggests testing throwing output constructors under ASan.

Assessment

The code is unchanged on libmultiprocess master. ReadDestUpdate is used for client results, output and in/out parameters, and for reads through non-null pointers and shared_ptr. When the constructor throws, clientInvoke catches the exception and rethrows it on the calling thread, and unwinding then destroys the already-destroyed object. So the report's C++ reasoning is correct.

The set of constructors that can actually throw here is smaller than the report suggests. Most of the library's ReadField functions finish validation before calling construct (the integer range check throws first), or read through update() or ReadDestEmplace instead. What remains:

• std::string(const char*, size_t) in type-string.h, which throws only bad_alloc.
• Bitcoin Core's overload for types with a deserializing constructor in src/ipc/capnp/common-types.h (return read_dest.construct(::deserialize, wrapper);). The constructor unserializes and throws on truncated or malformed bytes. No IPC method on Bitcoin Core master has such a type as a by-value result or output parameter: the mining interface uses CTransactionRef and CBlock, which take other paths. On the unmerged multiprocess branch (bitcoin#10102), createBumpTransaction and signBumpTransaction take a CMutableTransaction& in/out parameter, so a wallet process replying with malformed transaction bytes would trigger a double destroy in the GUI or node.

The report does not name a concrete call path for the other cases it lists. Reaching this takes a peer sending bytes its own serializer would never produce, or an allocation failure. Under Bitcoin Core's trust model such a peer already has unauthenticated RPC access; the case would matter more if wallet processes are ever treated as less trusted than the node.

The proposed patch requires Value to be move-assignable. That needs checking for every type that goes through ReadDestUpdate, and non-assignable types may need a fallback.

Related: AR-11 (compound outputs updated in place) and AR-14 (exceptions while building a request).

Reproduction. Not yet attempted against Bitcoin Core master.

Status

Present on master. Next step: a small libmultiprocess change that builds the replacement first and move-assigns it, with a test using a throwing constructor. Worth making; no issue open.

Reported privately in an independent security review on 2026-08-07.