IPC stability bugs

Bug 31

Reading IPC fields calls a function through a dangling reference, without visible effect

Status
Fixed
Symptom
Undefined behavior; worst effect seen: nothing observed
Where seen
Never observed
Trigger
Normal use; no special event is needed
First reported
2025-06-19 (first report)
Fixed
2025-06-26
Fix
mp#184
Shipped to Bitcoin Core
bitcoin#32345
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

ReadDestEmplace, a helper libmultiprocess uses when reading fields of incoming messages, stored a reference to the function object passed to it. ReadDestTemp passed it a temporary lambda, so every field read through ReadDestTemp called that lambda through a dangling reference. This is undefined behavior in the standard's terms, but the lambda had no state and its call never touched the object, so no misbehavior was ever seen; only clang-tidy's static analyzer flagged it. Fixed in mp#184 by storing the function object by value. The code was in Bitcoin Core source releases v25 through v29, which did not ship IPC binaries.

What happened

No runtime failure was ever observed. Bitcoin Core's clang-tidy CI job on bitcoin#31802 ("Add bitcoin-{node,gui} to release binaries for IPC") printed, without failing the job:

include/mp/proxy-types.h:134:5: warning: Address of stack memory associated with temporary object of type '(lambda ...)' is still referred to by a temporary object on the stack upon returning to the caller.  This will be a dangling reference [clang-analyzer-core.StackAddressEscape]

reached from the generated foo.capnp.proxy-server.c++ through serverInvoke, PassField and ReadField. @fanquake asked (2025-06-19) for the tidy output to be fixed or suppressed so the job stayed usable.

Root cause

ReadDestEmplace in include/mp/proxy-types.h took its emplace callable as EmplaceFn&& and kept it in a reference member, EmplaceFn& m_emplace_fn. The helper ReadDestTemp<LocalType>() returned

return ReadDestEmplace{TypeList<LocalType>(), [&](auto&&... args) -> decltype(auto) {
    return LocalType{std::forward<decltype(args)>(args)...};
}};

so the returned object referred to a lambda temporary that was destroyed at the end of the return statement, and construct() later called m_emplace_fn(...) through that reference. Calling a member function through a reference to a destroyed object is undefined behavior, but this lambda captures nothing and its operator() never reads this, so in practice the call runs the lambda's code and nothing different happens. The fix's commit message says the same: it "should not be a real problem ... But taking the reference could technically be undefined behavior."

The path ran on every read through ReadDestTemp, which was used for bool vector elements in type-vector.h and by the foo-types.h test types.

The code

Quoted from 060a73926956^, include/mp/proxy-types.h. ReadDestEmplace keeps a reference to the callable it is given and calls it later from construct():

template <typename LocalType, typename EmplaceFn>
struct ReadDestEmplace
{
    ReadDestEmplace(TypeList<LocalType>, EmplaceFn&& emplace_fn) : m_emplace_fn(emplace_fn) {}
    // ...
    template <typename... Args>
    decltype(auto) construct(Args&&... args)
    {
        return m_emplace_fn(std::forward<Args>(args)...);
    }
    // ...
    EmplaceFn& m_emplace_fn;
};
// ...
template <typename LocalType>
auto ReadDestTemp()
{
    return ReadDestEmplace{TypeList<LocalType>(), [&](auto&&... args) -> decltype(auto) {
        return LocalType{std::forward<decltype(args)>(args)...};
    }};
}

The lambda in ReadDestTemp is a temporary that is destroyed when the return statement ends, but the ReadDestEmplace returned to the caller still refers to it. A caller such as ReadField(..., ReadDestTemp<std::string>()) then calls construct(), which invokes the destroyed lambda through m_emplace_fn. That is undefined behaviour, and clang-tidy's StackAddressEscape check reports it. The lambda has no captures and its call operator reads no state, so the call ran the right code in practice.

The fix (060a73926956) stores the callable by value and drops the unused [&] capture (the .clang-tidy change enabling the check is omitted):

@@ -92,7 +92,7 @@ struct StructField
-    ReadDestEmplace(TypeList<LocalType>, EmplaceFn&& emplace_fn) : m_emplace_fn(emplace_fn) {}
+    ReadDestEmplace(TypeList<LocalType>, EmplaceFn emplace_fn) : m_emplace_fn(std::move(emplace_fn)) {}
@@ -123,7 +123,7 @@ struct ReadDestEmplace
-    EmplaceFn& m_emplace_fn;
+    EmplaceFn m_emplace_fn;
 };
@@ -131,7 +131,7 @@ struct ReadDestEmplace
-    return ReadDestEmplace{TypeList<LocalType>(), [&](auto&&... args) -> decltype(auto) {
+    return ReadDestEmplace{TypeList<LocalType>(), [](auto&&... args) -> decltype(auto) {
         return LocalType{std::forward<decltype(args)>(args)...};
     }};

Each ReadDestEmplace now owns a copy of its callable, so the callable lives as long as the object that calls it, whatever the lifetime of the lambda it was built from.

Fix

The fix commit 060a73926956 takes EmplaceFn by value and stores it by value (m_emplace_fn(std::move(emplace_fn))), and drops the unneeded [&] capture from the ReadDestTemp lambda (+3/-3 in proxy-types.h). The same commit enables clang-analyzer-core.StackAddressEscape in .clang-tidy. mp#184 ("Add CI jobs and fix clang-tidy and iwyu errors") also added libmultiprocess's own clang-tidy and IWYU CI jobs and fixed other tidy warnings; the three bugprone-use-after-move warnings it addressed were false positives (no object was actually moved from before use).

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 An object outliving what it points to: A stored reference to a temporary

  • Prevented by a design rule, a review checklist item
  • Earliest detector static analysis (did not exist then)
  • Diagnosis easy
  • Fix design easy
  • Debugging aids other
  • Rust yes

Would Rust have prevented it? Yes. A struct that borrows a temporary created in the same return statement cannot be returned; the borrow checker rejects it, so the callable would be stored by value from the start. More on Rust

Could it have been prevented?

Yes, by a rule that is common in C++ style guides and that the fix applies: a class that stores a callable it will invoke later stores it by value, never as a reference member bound from a constructor parameter, because the constructor cannot know whether its argument is a temporary. ReadDestEmplace took EmplaceFn&& and kept EmplaceFn& m_emplace_fn; the only caller that mattered, ReadDestTemp, passed a lambda that died at the end of its return statement (70d108bc6086, mp#81). The reference member and the temporary were introduced in the same commit, so a review item "does any reference member outlive what it was bound to?" would have caught it on the day. The fix (060a73926956) is three lines: take by value, store by value, drop the unused [&] capture.

Could it have been found sooner?

Yes, by the check that did find it, clang-analyzer's StackAddressEscape, which reports exactly this shape without running anything. Neither project ran it on this code in 2022: libmultiprocess had no clang-tidy job at all until mp#184 added one as part of this fix, and Bitcoin Core's tidy job did not compile the library until bitcoin#31802 ran it over the src/ipc/libmultiprocess subtree in June 2025, when fanquake noticed the warning in the output of a passing job. No runtime check could have found it: the lambda has no captures and its call operator never reads this, so ASan and UBSan see no bad access, and nothing ever misbehaved in the three years the code ran.

What made it hard to debug?

Nothing. The warning names the header line and the mechanism ("still referred to by a temporary object on the stack upon returning to the caller"), and the fix commit was written within a day of the report. The only effort was in classifying it: the same tidy run produced several bugprone-use-after-move warnings that turned out to be false positives, and this one had to be recognized as a real, if harmless, defect rather than suppressed with them. The commit body records that judgement ("should not be a real problem ... could technically be undefined behavior"), which is what a future reader needs to know.

Lesson

Store callables and other constructor arguments by value unless the class is documented as a non-owning view; and run the static analyzer on library code from the first commit, because a static check is the only kind that reports undefined behavior that happens to work.

Notes

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 none
Seen in never
Scope production
Trigger Normal use (ordinary-path)
Phase any
Side both
Component proxy-types
Root cause kind lifetime
Fix kind local-fix
Detection clang-tidy
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released source
Confidence low