CORE-629: Fix use-after-free in BRPeerManager/BRPeer #295
Conversation
| assert (0 == array_count(ctx->pongCallback)); | ||
| assert (NULL == ctx->mempoolCallback); |
There was a problem hiding this comment.
These probably aren't needed. Added them due to my unfamiliarity with this aspect of the BTC code (assert would catch if we needed to handle this).
| } | ||
|
|
There was a problem hiding this comment.
Likely the most controversial part; the spawning of a "cleanup"-like thread. Also, the dual nature of this function...
| @@ -93,7 +93,8 @@ extern "C" { | |||
| typedef enum { | |||
| BRPeerStatusDisconnected = 0, | |||
| BRPeerStatusConnecting, | |||
| BRPeerStatusConnected | |||
| BRPeerStatusConnected, | |||
| BRPeerStatusWaiting | |||
There was a problem hiding this comment.
Roll the separate waitingForNetwork flag into this.
| @@ -839,7 +839,7 @@ static void _peerDisconnected(void *info, int error) | |||
| if (manager->connectFailureCount > MAX_CONNECT_FAILURES) manager->connectFailureCount = MAX_CONNECT_FAILURES; | |||
| } | |||
|
|
|||
| if (! manager->isConnected && manager->connectFailureCount == MAX_CONNECT_FAILURES) { | |||
| if (! manager->isConnected && manager->connectFailureCount >= MAX_CONNECT_FAILURES) { | |||
There was a problem hiding this comment.
I'd seen cases where this was greater than MAX_CONNECT_FAILURES. In BRPeerManagerConnect and BRPeerManagerPublishTx, we checked for >=, mirrored that here as I had seen cases where it was greater than at this point.
| @@ -1679,7 +1673,7 @@ void BRPeerManagerDisconnect(BRPeerManager *manager) | |||
| p = manager->connectedPeers[i - 1]; | |||
| manager->connectFailureCount = MAX_CONNECT_FAILURES; // prevent futher automatic reconnect attempts | |||
| BRPeerDisconnect(p); | |||
| if (BRPeerConnectStatus(p) == BRPeerStatusConnecting) manager->peerThreadCount--; // waiting for network | |||
| while (BRPeerConnectStatus(p) == BRPeerStatusConnecting) BRPeerDisconnect(p); | |||
There was a problem hiding this comment.
This makes me leery (endless loop anyone?) but the logic should be that this only happens when the thread is being spawned. Once spawned, the BRPeer thread will set the status to connected or disconnected. In either case, this will break out (fingers crossed but also seen in testing).
michael-brd
changed the title
michael-brd
changed the title
michael-brd
force-pushed
the
bugfix/CORE-629
branch
from
6aad92b to
7fec484
Compare
michael-brd
force-pushed
the
bugfix/CORE-629
branch
from
7fec484 to
f63a229
Compare
I have some tests in
testBwm.cthat abuse the BRPeerManager (via the BRWalletManager). They call BRPeerManagerConnect and BRPeerManagerDisconnect, as well as BRPeerManagerDisconnect+BRPeerManagerFree. They also randomly set if the network is reachable or not.Using these tests, I can reliably trigger user-after-frees on an ASAN build.
To get the discussion started on how to fix, I've got this PoC branch that "fixes" the issues. Fixes in quotations because this is probably not how we want to fix it.
Some of the issues/changes:
BRPeerManagerConnectused to explicitly call_peerDisconnectedafter a race-y check against the BRPeer's thread; removed thisBRPeerDisconnect, but the BRPeer thread closes it too. We can't do that, as the file descriptor could possibly be reused. Since the BRPeer thread owns the socket (and has a close), removed thecloseinBRPeerDisconnect. Instead, just ashutdownis performed to signal our intent to close_peerDisconnected, etc.) but if the network is unreachable, no BRPeer thread is created. I removed thewaitingForNetworkBRPeer field and rolled it into the status. OnBRPeerDisconnect, if we never launched the thread (i.e. in theBRPeerStatusWaitingstate), launch a thread that does the same actions a "real" BRPeer thread would do (i.e. call disconnect, do the thread cleanup).