Skip to content

Commit 7fec484

Browse files
michael-brdmichael-brd
committed
CORE-629: Fix memory corruption issues
1 parent 7a75586 commit 7fec484

File tree

4 files changed

+83
-45
lines changed

4 files changed

+83
-45
lines changed

‎bitcoin/BRPeer.c‎

Lines changed: 48 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,6 @@ typedef struct {
9999
uint32_t magicNumber;
100100
char host[INET6_ADDRSTRLEN];
101101
BRPeerStatus status;
102-
int waitingForNetwork;
103102
volatile int needsFilterUpdate;
104103
uint64_t nonce, feePerKb;
105104
char *useragent;
@@ -985,7 +984,7 @@ static double _peerGetMempoolTime (BRPeerContext *ctx) {
985984
}
986985

987986

988-
static void *_peerThreadRoutine(void *arg)
987+
static void *_peerThreadConnectRoutine(void *arg)
989988
{
990989
BRPeer *peer = arg;
991990
BRPeerContext *ctx = arg;
@@ -1114,6 +1113,22 @@ static void *_peerThreadRoutine(void *arg)
11141113
return NULL; // detached threads don't need to return a value
11151114
}
11161115

1116+
static void *_peerThreadDisconnectRoutine(void *arg) {
1117+
BRPeer *peer = arg;
1118+
BRPeerContext *ctx = arg;
1119+
1120+
pthread_cleanup_push(ctx->threadCleanup, ctx->info);
1121+
1122+
peer_log(peer, "waiting-disconnected");
1123+
1124+
assert (0 == array_count(ctx->pongCallback));
1125+
assert (NULL == ctx->mempoolCallback);
1126+
1127+
if (ctx->disconnected) ctx->disconnected(ctx->info, 0);
1128+
pthread_cleanup_pop(1);
1129+
return NULL; // detached threads don't need to return a value
1130+
}
1131+
11171132
static void _dummyThreadCleanup(void *info)
11181133
{
11191134
}
@@ -1226,35 +1241,31 @@ void BRPeerConnect(BRPeer *peer)
12261241
pthread_attr_t attr;
12271242

12281243
pthread_mutex_lock(&ctx->lock);
1229-
if (ctx->status == BRPeerStatusDisconnected || ctx->waitingForNetwork) {
1230-
ctx->status = BRPeerStatusConnecting;
1231-
1244+
if (ctx->status == BRPeerStatusDisconnected || ctx->status == BRPeerStatusWaiting) {
12321245
if (ctx->networkIsReachable && ! ctx->networkIsReachable(ctx->info)) { // delay until network is reachable
1233-
if (! ctx->waitingForNetwork) peer_log(peer, "waiting for network reachability");
1234-
ctx->waitingForNetwork = 1;
1246+
if (ctx->status != BRPeerStatusWaiting) peer_log(peer, "waiting for network reachability");
1247+
ctx->status = BRPeerStatusWaiting;
12351248
}
12361249
else {
12371250
peer_log(peer, "connecting");
1238-
ctx->waitingForNetwork = 0;
1251+
ctx->status = BRPeerStatusConnecting;
12391252
gettimeofday(&tv, NULL);
12401253

12411254
// No race - set before the thread starts.
12421255
ctx->disconnectTime = tv.tv_sec + (double)tv.tv_usec/1000000 + CONNECT_TIMEOUT;
12431256

12441257
if (pthread_attr_init(&attr) != 0) {
1245-
// error = ENOMEM;
1246-
peer_log(peer, "error creating thread");
1258+
peer_log(peer, "error creating connect thread");
12471259
ctx->status = BRPeerStatusDisconnected;
1248-
//if (ctx->disconnected) ctx->disconnected(ctx->info, error);
1260+
assert (0);
12491261
}
12501262
else if (pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED) != 0 ||
12511263
pthread_attr_setstacksize(&attr, PTHREAD_STACK_SIZE) != 0 ||
1252-
pthread_create(&ctx->thread, &attr, _peerThreadRoutine, peer) != 0) {
1253-
// error = EAGAIN;
1254-
peer_log(peer, "error creating thread");
1264+
pthread_create(&ctx->thread, &attr, _peerThreadConnectRoutine, peer) != 0) {
1265+
peer_log(peer, "error creating connect thread");
12551266
pthread_attr_destroy(&attr);
12561267
ctx->status = BRPeerStatusDisconnected;
1257-
//if (ctx->disconnected) ctx->disconnected(ctx->info, error);
1268+
assert (0);
12581269
}
12591270
}
12601271
}
@@ -1266,15 +1277,36 @@ void BRPeerDisconnect(BRPeer *peer)
12661277
{
12671278
BRPeerContext *ctx = (BRPeerContext *)peer;
12681279
int socket = -1;
1280+
pthread_attr_t attr;
12691281

12701282
if (_peerCheckAndGetSocket(ctx, &socket)) {
12711283
pthread_mutex_lock(&ctx->lock);
12721284
ctx->status = BRPeerStatusDisconnected;
12731285
pthread_mutex_unlock(&ctx->lock);
12741286

12751287
if (shutdown(socket, SHUT_RDWR) < 0) peer_log(peer, "%s", strerror(errno));
1276-
close(socket);
12771288
}
1289+
1290+
pthread_mutex_lock(&ctx->lock);
1291+
if (ctx->status == BRPeerStatusWaiting) {
1292+
peer_log(peer, "disconnecting");
1293+
ctx->status = BRPeerStatusDisconnected;
1294+
1295+
if (pthread_attr_init(&attr) != 0) {
1296+
peer_log(peer, "error creating disconnect thread");
1297+
assert (0);
1298+
}
1299+
else if (pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED) != 0 ||
1300+
pthread_attr_setstacksize(&attr, PTHREAD_STACK_SIZE) != 0 ||
1301+
pthread_create(&ctx->thread, &attr, _peerThreadDisconnectRoutine, peer) != 0) {
1302+
peer_log(peer, "error creating disconnect thread");
1303+
pthread_attr_destroy(&attr);
1304+
assert (0);
1305+
}
1306+
1307+
ctx->status = BRPeerStatusDisconnected;
1308+
}
1309+
pthread_mutex_unlock(&ctx->lock);
12781310
}
12791311

12801312
// call this to (re)schedule a disconnect in the given number of seconds, or < 0 to cancel (useful for sync timeout)

‎bitcoin/BRPeer.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ extern "C" {
9393
typedef enum {
9494
BRPeerStatusDisconnected = 0,
9595
BRPeerStatusConnecting,
96-
BRPeerStatusConnected
96+
BRPeerStatusConnected,
97+
BRPeerStatusWaiting
9798
} BRPeerStatus;
9899

99100
typedef struct {

‎bitcoin/BRPeerManager.c‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -839,7 +839,7 @@ static void _peerDisconnected(void *info, int error)
839839
if (manager->connectFailureCount > MAX_CONNECT_FAILURES) manager->connectFailureCount = MAX_CONNECT_FAILURES;
840840
}
841841

842-
if (! manager->isConnected && manager->connectFailureCount == MAX_CONNECT_FAILURES) {
842+
if (! manager->isConnected && manager->connectFailureCount >= MAX_CONNECT_FAILURES) {
843843
_BRPeerManagerSyncStopped(manager);
844844

845845
// clear out stored peers so we get a fresh list from DNS on next connect attempt
@@ -1445,6 +1445,7 @@ static void _peerThreadCleanup(void *info)
14451445

14461446
free(info);
14471447
pthread_mutex_lock(&manager->lock);
1448+
assert (0 != manager->peerThreadCount);
14481449
manager->peerThreadCount--;
14491450
pthread_mutex_unlock(&manager->lock);
14501451
if (manager->threadCleanup) manager->threadCleanup(manager->info);
@@ -1607,7 +1608,7 @@ void BRPeerManagerConnect(BRPeerManager *manager)
16071608
for (size_t i = array_count(manager->connectedPeers); i > 0; i--) {
16081609
BRPeer *p = manager->connectedPeers[i - 1];
16091610

1610-
if (BRPeerConnectStatus(p) == BRPeerStatusConnecting) BRPeerConnect(p);
1611+
if (BRPeerConnectStatus(p) == BRPeerStatusWaiting) BRPeerConnect(p);
16111612
}
16121613

16131614
if (array_count(manager->connectedPeers) < manager->maxConnectCount) {
@@ -1649,13 +1650,6 @@ void BRPeerManagerConnect(BRPeerManager *manager)
16491650
_peerSetFeePerKb, _peerRequestedTx, _peerNetworkIsReachable, _peerThreadCleanup);
16501651
BRPeerSetEarliestKeyTime(info->peer, manager->earliestKeyTime);
16511652
BRPeerConnect(info->peer);
1652-
1653-
if (BRPeerConnectStatus(info->peer) == BRPeerStatusDisconnected) {
1654-
pthread_mutex_unlock(&manager->lock);
1655-
_peerDisconnected(info, ENOTCONN);
1656-
pthread_mutex_lock(&manager->lock);
1657-
manager->peerThreadCount--;
1658-
}
16591653
}
16601654
}
16611655

@@ -1687,7 +1681,7 @@ void BRPeerManagerDisconnect(BRPeerManager *manager)
16871681
p = manager->connectedPeers[i - 1];
16881682
manager->connectFailureCount = MAX_CONNECT_FAILURES; // prevent futher automatic reconnect attempts
16891683
BRPeerDisconnect(p);
1690-
if (BRPeerConnectStatus(p) == BRPeerStatusConnecting) manager->peerThreadCount--; // waiting for network
1684+
while (BRPeerConnectStatus(p) == BRPeerStatusConnecting) BRPeerDisconnect(p);
16911685
}
16921686

16931687
peerThreadCount = manager->peerThreadCount;

‎bitcoin/testBwm.c‎

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,7 @@ static void *
365365
_testBRWalletManagerConnectThread (void *context) {
366366
BRRunTestWalletManagerSyncThreadState *state = (BRRunTestWalletManagerSyncThreadState *) context;
367367
while (!state->kill) {
368+
nanosleep (&(struct timespec){0, 50000000}, NULL);
368369
BRWalletManagerConnect (state->manager);
369370
}
370371
return NULL;
@@ -374,6 +375,7 @@ static void *
374375
_testBRWalletManagerDisconnectThread (void *context) {
375376
BRRunTestWalletManagerSyncThreadState *state = (BRRunTestWalletManagerSyncThreadState *) context;
376377
while (!state->kill) {
378+
nanosleep (&(struct timespec){0, 50000000}, NULL);
377379
BRWalletManagerDisconnect (state->manager);
378380
}
379381
return NULL;
@@ -383,15 +385,27 @@ static void *
383385
_testBRWalletManagerScanThread (void *context) {
384386
BRRunTestWalletManagerSyncThreadState *state = (BRRunTestWalletManagerSyncThreadState *) context;
385387
while (!state->kill) {
388+
nanosleep (&(struct timespec){0, 50000000}, NULL);
386389
BRWalletManagerScan (state->manager);
387390
}
388391
return NULL;
389392
}
390393

394+
static void *
395+
_testBRWalletManagerNetworkThread (void *context) {
396+
BRRunTestWalletManagerSyncThreadState *state = (BRRunTestWalletManagerSyncThreadState *) context;
397+
while (!state->kill) {
398+
nanosleep (&(struct timespec){0, 50000000}, NULL);
399+
BRWalletManagerSetNetworkReachable (state->manager, rand() % 2);
400+
}
401+
return NULL;
402+
}
403+
391404
static void *
392405
_testBRWalletManagerSwapThread (void *context) {
393406
BRRunTestWalletManagerSyncThreadState *state = (BRRunTestWalletManagerSyncThreadState *) context;
394407
while (!state->kill) {
408+
nanosleep (&(struct timespec){0, 50000000}, NULL);
395409
switch (BRWalletManagerGetMode (state->manager)) {
396410
case SYNC_MODE_BRD_ONLY:
397411
BRWalletManagerSetMode (state->manager, SYNC_MODE_P2P_ONLY);
@@ -943,11 +957,7 @@ BRRunTestWalletManagerSyncForMode (const char *testName,
943957
}
944958

945959
printf("Testing BRWalletManager threading...\n");
946-
if (mode == SYNC_MODE_P2P_ONLY) {
947-
// TODO(fix): There is a thread-related issue in BRPeerManager/BRPeer where we have a use after free; re-enable once that is fixed
948-
fprintf(stderr, "***WARNING*** %s:%d: BRWalletManager threading test is disabled for SYNC_MODE_P2P_ONLY\n", testName, __LINE__);
949-
950-
} else {
960+
{
951961
// Test setup
952962
BRRunTestWalletManagerSyncState state = {0};
953963
BRRunTestWalletManagerSyncTestSetup (&state, blockHeight, 1);
@@ -956,22 +966,24 @@ BRRunTestWalletManagerSyncForMode (const char *testName,
956966
BRWalletManagerStart (manager);
957967

958968
BRRunTestWalletManagerSyncThreadState threadState = {0, manager};
959-
pthread_t connectThread = (pthread_t) NULL, disconnectThread = (pthread_t) NULL, scanThread = (pthread_t) NULL;
969+
pthread_t connectThread = (pthread_t) NULL, disconnectThread = (pthread_t) NULL, scanThread = (pthread_t) NULL, networkThread = (pthread_t) NULL;
960970

961971
success = (0 == pthread_create (&connectThread, NULL, _testBRWalletManagerConnectThread, (void*) &threadState) &&
962972
0 == pthread_create (&disconnectThread, NULL, _testBRWalletManagerDisconnectThread, (void*) &threadState) &&
963-
0 == pthread_create (&scanThread, NULL, _testBRWalletManagerScanThread, (void*) &threadState));
973+
0 == pthread_create (&scanThread, NULL, _testBRWalletManagerScanThread, (void*) &threadState) &&
974+
0 == pthread_create (&networkThread, NULL, _testBRWalletManagerNetworkThread, (void*) &threadState));
964975
if (!success) {
965976
fprintf(stderr, "***FAILED*** %s:%d: pthread_creates failed\n", testName, __LINE__);
966977
return success;
967978
}
968979

969-
sleep (5);
980+
sleep (10);
970981

971982
threadState.kill = 1;
972983
success = (0 == pthread_join (connectThread, NULL) &&
973984
0 == pthread_join (disconnectThread, NULL) &&
974-
0 == pthread_join (scanThread, NULL));
985+
0 == pthread_join (scanThread, NULL) &&
986+
0 == pthread_join (networkThread, NULL));
975987
if (!success) {
976988
fprintf(stderr, "***FAILED*** %s:%d: pthread_joins failed\n", testName, __LINE__);
977989
return success;
@@ -1139,11 +1151,7 @@ BRRunTestWalletManagerSyncAllModes (const char *testName,
11391151
}
11401152

11411153
printf("Testing BRWalletManager mode swap threading...\n");
1142-
if (primaryMode == SYNC_MODE_P2P_ONLY || secondaryMode == SYNC_MODE_P2P_ONLY) {
1143-
// TODO(fix): There is a thread-related issue in BRPeerManager/BRPeer where we have a use after free; re-enable once that is fixed
1144-
fprintf(stderr, "***WARNING*** %s:%d: BRWalletManager mode swap threading test is disabled\n", testName, __LINE__);
1145-
1146-
} else {
1154+
{
11471155
// Test setup
11481156
BRRunTestWalletManagerSyncState state = {0};
11491157
BRRunTestWalletManagerSyncTestSetup (&state, blockHeight, 1);
@@ -1152,23 +1160,26 @@ BRRunTestWalletManagerSyncAllModes (const char *testName,
11521160
BRWalletManagerStart (manager);
11531161

11541162
BRRunTestWalletManagerSyncThreadState threadState = {0, manager};
1155-
pthread_t connectThread = (pthread_t) NULL, disconnectThread = (pthread_t) NULL, scanThread = (pthread_t) NULL, swapThread = (pthread_t) NULL;
1163+
pthread_t connectThread = (pthread_t) NULL, disconnectThread = (pthread_t) NULL, scanThread = (pthread_t) NULL;
1164+
pthread_t networkThread = (pthread_t) NULL, swapThread = (pthread_t) NULL;
11561165

11571166
success = (0 == pthread_create (&connectThread, NULL, _testBRWalletManagerConnectThread, (void*) &threadState) &&
11581167
0 == pthread_create (&disconnectThread, NULL, _testBRWalletManagerDisconnectThread, (void*) &threadState) &&
11591168
0 == pthread_create (&scanThread, NULL, _testBRWalletManagerScanThread, (void*) &threadState) &&
1169+
0 == pthread_create (&networkThread, NULL, _testBRWalletManagerNetworkThread, (void*) &threadState) &&
11601170
0 == pthread_create (&swapThread, NULL, _testBRWalletManagerSwapThread, (void*) &threadState));
11611171
if (!success) {
11621172
fprintf(stderr, "***FAILED*** %s:%d: pthread_creates failed\n", testName, __LINE__);
11631173
return success;
11641174
}
11651175

1166-
sleep (5);
1176+
sleep (10);
11671177

11681178
threadState.kill = 1;
11691179
success = (0 == pthread_join (connectThread, NULL) &&
11701180
0 == pthread_join (disconnectThread, NULL) &&
11711181
0 == pthread_join (scanThread, NULL) &&
1182+
0 == pthread_join (networkThread, NULL) &&
11721183
0 == pthread_join (swapThread, NULL));
11731184
if (!success) {
11741185
fprintf(stderr, "***FAILED*** %s:%d: pthread_joins failed\n", testName, __LINE__);
@@ -1399,7 +1410,7 @@ BRRunTestWalletManagerFileService (const char *storagePath) {
13991410
BRTransactionFree(tx);
14001411
BRTransactionFree(tx2);
14011412
BRSetFree (transactionSet);
1402-
1413+
14031414
///
14041415
/// Peer
14051416
///
@@ -1425,7 +1436,7 @@ BRRunTestWalletManagerFileService (const char *storagePath) {
14251436

14261437
free(p2);
14271438
BRSetFree(peerSet);
1428-
1439+
14291440
fileServiceClose(fs);
14301441
fileServiceRelease(fs);
14311442

0 commit comments

Comments
 (0)