cgra's

What follows, should be considered EXPERIMENTAL, because:

1. I could be wrong,
2. brain fart this thick is difficult to breathe,
3. nobody else looked yet, and
4. it's not been extensively tested.

Let us proceed. We will first analyze the current situation, then have a look at the proposed solution.

The reader may first want to have a careful look at the patch contents below this article, then resume here to read the whole description.

Current situation

TRB has a bunch of (here, here, and here) out-of-memory DoS issues. This article will attempt to address one specific issue in that group, which is the vNodesDisconnected dumpster, where every disconnected CNode is put on hold for 15 minutes, after being disconnected, and before actually deallocated.

The dumpster mechanism is bad, because:

• It's not necessary (elaborated later), and
• It enables an attacker to waste the target node's RAM just by repeatedly connecting and disconnecting.

Brain Farts of Satoshi, episode ∞ - Ref Counting

Tied to the dumpster mechanism are¹: CNode's AddRef(), Release(), GetRefCount(), nRefCount, and nReleaseTime.

AddRef() and Release() calls should go in pairs, otherwise nRefCount could remain positive, indefinitely. In such cases, the node wouldn't ever get removed from vNodesDisconnected, and deallocated.

These refs are adjusted as follows:

• Increment and decrement for each peer, when beginning and ending a message processing loop in ThreadMessageHandler2() thread.
• Increment and decrement for each peer, when beginning and ending a send()/recv() handling loop in ThreadSocketHandler2() thread.
• Increment on peer's connect-time, when peer gets added to vNodes. Both, inbound and outbound.
• Decrement when disconnected node is moved from vNodes to vNodesDisconnected.
• Increment in a corner case where simultaneously connecting inbound, and outbound, to the same peer, resulting in sharing a CNode object without separate inbound/outbound connections. See next.

In ConnectNode(), an unbalanced AddRef() is theoretically possible: If new inbound peer was connected after the FindNode() call in OpenNetworkConnection(), and before the FindNode() call in ConnectNode(), and also happened to be the same peer our node was about to connect to (ie. ConnectNode()'s parameter addrConnect). Here, the inbound and outbound connection attempts would end up peculiarly sharing the same CNode object.

Otherwise, starting from the last FindNode() call, nothing prevents a peer from creating multiple connections to our node, and if we had an outbound connection already to the peer, we could end up with separate inbound and outbound connections to the same (unlike above, not a shared CNode object).

In the dumpster filling section is an expression that never triggers. The ref count increments for new connections (inbound, outbound) are only balanced with their equivalent decrements after triggering the expression, which is impossible!

Therefore, ref counts are only ever productively read by the check: whether to already delete a disconnected node, or not.

Ref counting is mostly pointless

Because the ref count reading happens in the ThreadSocketHandler2() thread, it's entirely pointless to adjust ref counts in the send()/recv handler (increment, decrement). The ref count reading will never happen at the same time -- both are in different positions of the same thread.

Adjusting the ref count, when peer is being added to, or removed from vNodes (new inbound, new outbound; dumpster load), is similarly pointless, but serves one distinct purpose: Prevent a theoretical risk of a dangling pointer dereference on two rows:

• Patch line 57, a redundant timestamp assignment, and
• patch line 177, an outbound-node-flag assignment, which could easily reside in a thread-safe position.

The last remaining, and within the scope of this patch, fundamentally the only, useful place to adjust the ref count is the ThreadMessageHandler2()'s loop. It signals the deletion code in ThreadSocketHandler2() thread that whether the message handler thread is still actively handling the peer data, or not.

The 15 min dumpster cooldown of disconnected nodes doesn't add to the ref counting anything else, but a loose, extra protection against the marginal dangling pointer dereference risk in OpenNetworkConnection(), at patch line 177: The second FindNode() call could find an existing peer, which the ThreadSocketHandler2() could delete immediately after - now we have a dangling pnode pointer.

Proposed changes

Removals: redundant outbound validation, unbalanced ref count increment, redundant assignment

Repeatedly validating outbound connections (patch lines 164, 165, 28, and 32) is useless, and for the FindNode() check especially, because the acceptance of inbound connections from a single peer IP address is not limited to one only.

The AddRef() by the second FindNode() position is not balanced anywhere, and could cause a stale peer to stick till node restart.

Also, assignment at patch line 57 is already done in CNode constructor, so is redundant. And besides, it marginally risks a dangling pointer dereference, as previously noted.

For these reasons we may remove lines from ConnectNode(), marked by patch lines from 28 to 41, and line 57.

Simplified outbound connection init

We may simplify the OpenNetworkConnection() function to return void, instead of bool, because the return value isn't used anywhere, and it further helps cleaning up the whole function.

The outbound-node-flag assignment from OpenNetworkConnection() should be moved to happen before the new peer gets added to vNodes in ConnectNode(), to avoid one of the marginal risks of a dangling pointer dereference noted earlier.

ConnectNode() would convey it's meaning more accurately, if it didn't return a CNode pointer. Specifically, we don't want to do anything with a node object, when outside of cs_vNodes/ref count guard, to avoid further dangling pointers. So we change the return type to void.

ConnectNode() parameter nTimeout is always 0, so we bolt the value down.

These changes are highlighted by patch lines: 16, 17, 25, 26, 53, 56-62, 155-180, 218 and 219.

Reduce the ref counting to required minimum

Because the AddRef() / Release() / GetRefCount() only make sense in coordinating the stale peer deletion moment, between ThreadMessageHandler2() and ThreadSocketHandler2(), we remove all the other uses:

• From ConnectNode() outbound connection initialization,
• From ThreadSocketHandler2() send()/recv() loop (before loop, after loop), and
• From ThreadSocketHandler2() inbound connection initialization
• From ThreadSocketHandler2() dumpster-loading connection ref count decrement

AddRef()'s nTimeout parameter is never used, we may remove it.

The nReleaseTime will not be used, and must go (patch lines 90, 227, 235 and 250-255). We previously fixed the corner case it was loosely originally protecting -- nReleaseTime won't bed missed.

Because ref count is never 0 at deletion check time, we may remove the expression that never triggers.

Tow away the dumpster

The vNodesDisconnected can go entirely. We will keep the stale nodes in vNodes instead, and they won't stay there for long.

None of the TRY_CRITICAL_BLOCK's were necessary, because:

• The whole dumpster-loading and dumpster-unloading section is surrounded by the cs_vNodes lock
• Either an outer cs_vNodes has us covered, where the other threads may hold these locks (ThreadRCPServer() and ThreadBitcoinMiner(), which only are concerned with relaying either user's transactions, or freshly self-mined blocks), or
• ThreadMessageHandler2() is either sleeping, waiting for cs_vNodes, or signalling own use with an increased ref count

We keep the zero ref count condition, under which the final deallocation may happen, like before.

The outbound/inbound flag check is pointless, because no node may show up here that has both flags false. The only three places where CNode constructor is called, are:

• New inbound peer connection, where fInbound will become true,
• new outbound peer connection, where fNetworkNode will become true, and
• a global pnodeLocalHost object allocation in StartNode()², which is a redundant object, doing nothing of importance.

The 1) removal from vNodes, and 2) CNode deallocation, will happen together, under required conditions. Closing the TCP socket and other cleanup should happen without delay, even if the final deletion had to wait until ThreadMessageHandler2() is done with it's current iteration.

This whole dumpster hauling change is highlighted by the patch lines 70, and 78-118.

Finally, ThreadMessageHandler2() and ThreadSocketHandler2() threads could remain off-sync for a while so that the deletion section didn't immediately coincide with the message handler's sleeping period. For this reason, we're probably better off not letting already disconnected nodes to unnecessarily get locked down by the ref count guard (patch lines 188-198).

In conclusion

Simple as that! Right?

The patch

The patch file and my corresponding signature.

1. See bitcoin/src/net.h, declaration of class CNode [↩]
2. See: bitcoin/src/net.cpp [↩]