IPC stability bugs

Bug 21

Client proxy code casts to a derived object before it exists or after it is gone

Status
Fixed
Symptom
Undefined behavior; worst effect seen: sanitizer report only
Where seen
Bitcoin Core unit tests
Trigger
Normal use; no special event is needed
First reported
2024-11-22 (first report)
Fixed
2025-01-16
Fix
mp#121, mp#127
Shipped to Bitcoin Core
bitcoin#31740
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

Every time a ProxyClient was constructed or destroyed, the base class ProxyClientBase did a static_cast from itself to the derived ProxyClient type inside its own constructor and destructor. At those points the derived object does not exist yet, or no longer exists. That is undefined behavior, and UBSan reported it as a dynamic-type-mismatch downcast in Bitcoin Core's ipc_tests. It had no practical effect, because the generated ProxyClient classes add no state of their own. Source builds of v22 through v28 with multiprocess enabled have it. Fixed in two steps, mp#121 (constructor) and mp#127 (destructor).

What happened

Root cause

ProxyClientBase<Interface, Impl> (include/mp/proxy.h) had a self() helper, static_cast<ProxyClient<Interface>&>(*this), used to call the generated subclass's optional construct() and destroy() hooks. The constructor (include/mp/proxy-io.h) called self().construct() as its last statement, and the destructor's cleanup lambda called self().destroy() and used self().m_client. At both points the most-derived ProxyClient object is not alive, so the cast is undefined behavior under the object-lifetime rules, though nothing could go wrong in practice since the derived classes have no members of their own.

The cast has been there since the library's initial commit. Nothing reported it until Bitcoin Core built libmultiprocess with -fsanitize=undefined.

The code

Quoted from 5b8119284760^. ProxyClientBase (include/mp/proxy.h) is the base of every generated ProxyClient<Interface> class, and reaches the generated construct() and destroy() methods through a downcast:

void construct() {}
void destroy() {}

ProxyClient<Interface>& self() { return static_cast<ProxyClient<Interface>&>(*this); }

The base-class constructor in include/mp/proxy-io.h calls it as its last statement, and registers a cleanup lambda that ~ProxyClientBase runs, which calls it again:

    m_context.cleanup.emplace_front([this, destroy_connection, cleanup]{
    if (m_context.connection) {
        // ...
        self().destroy();

        // FIXME: Could just invoke removed addCleanup fn here instead of duplicating code
        m_context.connection->m_loop.sync([&]() {
            // Release client capability by move-assigning to temporary.
            {
                typename Interface::Client(std::move(self().m_client));
            }
    // ...
    });
    self().construct();
}

In the constructor the ProxyClient part of the object has not been constructed yet, and in ~ProxyClientBase it has already been destroyed, so both static_casts name an object that does not exist. That is undefined behaviour, and UBSan reports it as "downcast of address ... which does not point to an object of type 'ProxyClient'". Generated ProxyClient classes add no data members, so no memory was misread in practice.

The first fix (5b8119284760) moved the construct() call into a generated ProxyClient constructor. The second (63a39d4c9bc2) removed self(), made construct and destroy static functions taking the base object, and reverted the generator change; its src/mp/gen.cpp hunks are omitted:

--- a/include/mp/proxy.h
+++ b/include/mp/proxy.h
@@ -89,10 +91,8 @@ public:
-    void construct() {}
-    void destroy() {}
-
-    ProxyClient<Interface>& self() { return static_cast<ProxyClient<Interface>&>(*this); }
+    static void construct(Super&) {}
+    static void destroy(Super&) {}
--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -412,13 +412,13 @@ ProxyClientBase<Interface, Impl>::ProxyClientBase(typename Interface::Client cli
-        self().destroy();
+        Sub::destroy(*this);

         // FIXME: Could just invoke removed addCleanup fn here instead of duplicating code
         m_context.connection->m_loop.sync([&]() {
             // Release client capability by move-assigning to temporary.
             {
-                typename Interface::Client(std::move(self().m_client));
+                typename Interface::Client(std::move(m_client));
             }
@@ -432,6 +432,7 @@ ProxyClientBase<Interface, Impl>::ProxyClientBase(typename Interface::Client cli
+    Sub::construct(*this);
 }

Sub::construct(*this) and Sub::destroy(*this) only use the ProxyClient class's static functions and pass the base object, which is alive at both points, so no reference to the missing derived object is formed.

Fix

No test was added; the UBSan job running ipc_tests covers it.

Timeline

Lessons

What would have prevented this bug, found it sooner, or made it easier to debug. The patterns page groups these lessons across all bugs.

Pattern Built or torn down in the wrong order: A base class reaching a derived object that is not built yet or already gone

  • Prevented by a different API shape
  • Earliest detector UBSan (did not exist then)
  • Diagnosis easy
  • Fix design moderate
  • Debugging aids sanitizer output
  • Rust yes

Would Rust have prevented it? Yes. Rust has no base-class constructors or destructors that run on a partially built object; a struct is fully initialized before any method can see it, so the downcast-from-base hook cannot be written. More on Rust

Could it have been prevented?

Yes, by the shape the final fix chose. ProxyClientBase reached its generated subclass's construct() and destroy() hooks through static_cast<ProxyClient<Interface>&>(*this) from its own constructor and destructor, the two places where the derived object is by definition not alive. Hooks that a base class calls during its own lifetime must not need the derived object: mp#127 (63a39d4c9bc2) makes them static functions taking the base by reference, which removes self() and with it the possibility of the cast. The rule ("no downcast in a base constructor or destructor") is standard C++ guidance and was knowable in 2019; the cast was written because the derived classes carry no state, which is also why it never mattered in practice.

Could it have been found sooner?

Yes, by compiling the library itself with -fsanitize=undefined and running any test that constructs a proxy: every ProxyClient construction trips it, deterministically. Bitcoin Core had a UBSan job in 2019 but libmultiprocess was an external depends package it never built with sanitizers, and the library had no sanitizer CI of its own; bitcoin#30975 built multiprocess in the ASan+UBSan job and found it on the first run. The destructor site appeared only after the constructor site was fixed, because UBSan halts on the first report, which is why one bug became two PRs.

What made it hard to debug?

Nothing about the cause: the sanitizer named the line and the missing derived type. The effort was in the fix. mp#121 addressed only the constructor by having the code generator emit a forwarding ProxyClient constructor; the destructor's cleanup lambda still went through self(), and the first attempt at that (mp#126) was more invasive than wanted and was closed for mp#127, which also reverted the mp#121 generator change. Treating the first report as one site rather than asking where else self() was called cost a second CI round; a grep for the helper would have found both.

Lesson

Hooks that a base class invokes from its constructor or destructor must take the base object, never the derived one; and when a sanitizer reports a pattern, fix every use of the pattern rather than the reported line, because it stops at the first.

Classification

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

Field Value
Symptom ub
Worst effect observed sanitizer-report-only
Seen in unit-test
Scope production
Trigger Normal use (ordinary-path)
Phase any
Side client
Component proxy-io
Root cause kind lifetime
Fix kind local-fix
Detection ubsan
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see yes
Defect ever on master yes
Released source
Confidence high