Audit report AR-25

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

Report date

2026-08-20

Kind

Design

Status on master

Present

Proposed issue state

Open

Summary

IpcImpl in Bitcoin Core declares its protocol member before its process member, so C++ destroys the process object first, although protocol cleanup (the callback that waits for a spawned process) can call into it. Bitcoin Core releases its IPC clients before IpcImpl, so nothing goes wrong today. The report is accurate and calls this lifecycle hardening; swapping the two members makes destruction order match the dependency. Cosmetic fix, mergeable.

The report

In src/ipc/interfaces.cpp, IpcImpl holds std::unique_ptr<Protocol> m_protocol followed by std::unique_ptr<Process> m_process. Members are destroyed in reverse order of declaration, so m_process is destroyed before m_protocol, even though protocol cleanup can depend on the process object.

The report says no ordinary Bitcoin Core shutdown sequence has been shown to break the rule that IPC clients are released before their owner, and that a concurrent-teardown test it had earlier does not show that such caller behaviour is supported. It considered supporting clients that outlive their IPC owner and rejected it, since that would need a broader ownership contract. The patch has no test.

Proposed change

Declare and initialize the process owner before the protocol, so that protocol destruction finishes first, keeping unique ownership and the existing clients-before-owner contract:

     IpcImpl(const char* exe_name, const char* process_argv0, interfaces::Init& init)
         : m_exe_name(exe_name), m_process_argv0(process_argv0), m_init(init),
-          m_protocol(ipc::capnp::MakeCapnpProtocol(exe_name)), m_process(ipc::MakeProcess())
+          m_process(ipc::MakeProcess()), m_protocol(ipc::capnp::MakeCapnpProtocol(exe_name))
@@
     interfaces::Init& m_init;
-    std::unique_ptr<Protocol> m_protocol;
     std::unique_ptr<Process> m_process;
+    std::unique_ptr<Protocol> m_protocol;

Assessment

Accurate. On Bitcoin Core master m_protocol is declared before m_process. spawnProcess registers a cleanup callback that calls m_process->waitSpawned(pid), so a spawned-process client that outlived IpcImpl would call into a destroyed ProcessImpl. ProcessImpl has no data members, so even that would most likely be harmless in practice, and the contract requires clients to be released first.

ryanofsky, replying to the report, wrote that there is "probably not an issue here because Init object should be destroyed before ipc::Process object", that a shared_ptr approach tried in an earlier version felt less safe than a deterministic order, and that "logically it makes sense to change m_process / m_protocol field order because the process should be longer lived than IPC subsystem". No stability effect and no bug page.

Status

Present on master. Next step: the two-line reorder as a small Bitcoin Core PR. No public PR yet.

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