IPC stability bugs

Bug 14

Separate GUI process crashes when broadcasting a PSBT

Status
Fixed
Symptom
Crash; worst effect seen: crash
Where seen
Branch-build
Trigger
Normal use; no special event is needed
First reported
2021-11-12 (first report)
Fixed
2021-11-16
Fix
bitcoin#23499
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

In the multiprocess GUI, where bitcoin-gui runs in its own process and talks to the node over IPC, clicking "Broadcast" in the PSBT operations dialog crashed bitcoin-gui with a null pointer dereference. The dialog called a node-internal function through node().context(), which is null when the node runs in another process. Released binaries (v22.0 included) contain the call but cannot reach it, because their GUI runs in the same process as the node; only the out-of-process bitcoin-gui of the unmerged multiprocess branches crashed. Fixed on master by bitcoin#23499 ("multiprocess: Add interfaces::Node::broadCastTransaction method") ahead of that feature.

What happened

ryanofsky hit the crash while testing the multiprocess GUI of the bitcoin#10102 ("Multiprocess bitcoin") branch and opened bitcoin#23499 with the fix on 2021-11-12. No CI failure or issue was filed. The same day he opened bitcoin#23497, which moved node code into the node:: namespace to make GUI access to node and wallet internals easier to see; its dialog change needed a using node::BroadcastTransaction declaration that was dropped after bitcoin#23499 merged.

Root cause

PSBTOperationsDialog::broadcastTransaction in src/qt/psbtoperationsdialog.cpp bypassed the interfaces::Node interface and called the node-internal function directly:

TransactionError error = BroadcastTransaction(
    *m_client_model->node().context(), tx, err_string, DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK(), /* relay */ true, /* await_callback */ false);

In-process, Node::context() returns the live NodeContext, the struct that holds the node's internal state. In bitcoin-gui, interfaces::Node is an IPC proxy (ProxyClient<Node>) that forwards method calls to the node process; it has no NodeContext and returns a null pointer, so the call dereferenced null. The call dates from the commit that created the dialog (a6cb0b0c29d3, "[gui] PSBT Operations Dialog (sign & broadcast)", merged in bitcoin#18027). Any direct use of NodeContext from GUI code is a latent crash of this kind once the GUI runs out of process.

The code

Quoted from 0e0f4fdd8922^. PSBTOperationsDialog::broadcastTransaction in src/qt/psbtoperationsdialog.cpp calls the node-internal BroadcastTransaction with the node's NodeContext:

CTransactionRef tx = MakeTransactionRef(mtx);
std::string err_string;
TransactionError error = BroadcastTransaction(
    *m_client_model->node().context(), tx, err_string, DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK(), /* relay */ true, /* await_callback */ false);

interfaces::Node in src/interfaces/node.h only has a context when the node runs in the same process; the default implementation returns null:

//! Get and set internal node context. Useful for testing, but not
//! accessible across processes.
virtual NodeContext* context() { return nullptr; }

NodeImpl overrides context() to return the live NodeContext, but in bitcoin-gui node() is an IPC proxy that keeps the default, so *m_client_model->node().context() dereferences a null pointer when the user clicks Broadcast.

The fix adds a broadcastTransaction method to interfaces::Node and calls it from the dialog (0e0f4fdd8922; the declaration in src/interfaces/node.h is omitted):

--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -261,6 +261,10 @@ public:
+    TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, std::string& err_string) override
+    {
+        return BroadcastTransaction(*m_context, std::move(tx), err_string, max_tx_fee, /*relay=*/ true, /*wait_callback=*/ false);
+    }
--- a/src/qt/psbtoperationsdialog.cpp
+++ b/src/qt/psbtoperationsdialog.cpp
@@ -110,8 +110,8 @@ void PSBTOperationsDialog::broadcastTransaction()
-    TransactionError error = BroadcastTransaction(
-        *m_client_model->node().context(), tx, err_string, DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK(), /* relay */ true, /* await_callback */ false);
+    TransactionError error =
+        m_client_model->node().broadcastTransaction(tx, DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK(), err_string);

The dialog now goes through the Node interface, so across processes the call is forwarded to NodeImpl in the node process, which has a real m_context to broadcast with.

Fix

Commit 0e0f4fdd8922 adds interfaces::Node::broadcastTransaction(tx, max_tx_fee, err_string) (src/interfaces/node.h, implemented in NodeImpl in src/node/interfaces.cpp) and makes the dialog call it, so the broadcast is forwarded to the node process like other node operations (+10/-2).

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 in-process interface trusting what now comes from another process: An interface that only works inside one process

  • Prevented by a different API shape, static analysis
  • Earliest detector static analysis (did not exist then)
  • Diagnosis easy
  • Fix design easy
  • Rust partly

Would Rust have prevented it? Partly. The null NodeContext dereference would be an Option unwrap panic instead of a segfault, but the design flaw, a test-only escape hatch on the production interface, is a visibility choice that Rust's module privacy could enforce only if someone made it. More on Rust

Could it have been prevented?

Yes. interfaces::Node::context() exists for tests and is documented "not accessible across processes", but it is a public method on the same interface GUI code uses, so a dialog author in 2020 could reach the node's internals in one call and nothing objected. The fix (0e0f4fdd8922) is the API-shape answer: give Node a broadcastTransaction method and route the dialog through it. A shape that made the shortcut impossible, context() available only from a test helper rather than the interface, would have forced that at the time the dialog was written. Short of that, a lint that fails on context() or node:: symbols in src/qt/ is mechanical; bitcoin#23497, opened the same day, moved node code into a node:: namespace so exactly these calls become visible as using declarations in GUI files.

Could it have been found sooner?

Yes, by a grep. Every direct use of NodeContext from GUI code is a null dereference once the GUI is in its own process, and a review checklist for GUI PRs ("does this call anything not on interfaces::Node or Wallet") would have flagged the dialog in bitcoin#18027. No CI ran bitcoin-gui out of process, so the call sat for seventeen months until ryanofsky clicked Broadcast in the bitcoin#10102 GUI; a functional test driving the separate-process GUI would only have caught it if it exercised the PSBT dialog, which the GUI tests did not.

What made it hard to debug?

Nothing. A crash on one button with a null NodeContext points at the one line that dereferences it, and the fix took an afternoon. The cost is the discovery, one dialog at a time, of each place the in-process build let GUI code bypass the interface.

Lesson

An interface that also exposes an "escape hatch" to the implementation will be used through the hatch; keep test-only access off the production interface, and enforce with a lint that the client side of a process boundary calls only the interface.

Classification

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

Field Value
Symptom crash
Worst effect observed crash
Seen in branch-build
Scope production
Trigger Normal use (ordinary-path)
Phase steady-state
Side client
Component bitcoin-ipc
Root cause kind assumption-violated
Fix kind local-fix
Detection manual-test
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released source
Confidence high