Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when BRBitcoinPeer disconnects fast #410

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cryptodev100
Copy link
Contributor

@cryptodev100 cryptodev100 commented Dec 3, 2021

Dear Blockset maintainers,

I was playing around with the great functionality of walletkit, but then I stumbled upon a strange crash.
The crash happened especially often when trying to connect to a BitcoinPeer in iPhone-airplane-mode, but it also sometimes happened when my iPhone was not in airplane-mode.
My investigations made it clear that I was dealing with a race-condition and a potential use-after-free.

Therefore, this PR fixes a suspected race-condition when a BRBitcoinPeer disconnects within a very short time.
I don't have a minimum reproducible sample, but I will try to explain how I discovered this crash:

It starts with connecting to BitCoin-peers in peer-2-peer-only-mode, when we reach btcPeerConnect within BRBitcoinPeer.c.
btcPeerConnect spawns a new thread that executes _peerThreadRoutine.
_peerThreadRoutine tries to establish a socket, when it fails to establish a socket, then it will eventually call _peerDisconnected and exit the previously created thread.

An "info-struct" gets passed to _peerDisconnected.
_peerDisconnected frees members of the info-struct and does some cleanup-stuff.
However, I suspect that _peerDisconnected might be called twice for the same info-struct.
Let me show you the two callsites of _peerDisconnected.
The first callsite is the following code-snippet, which has a suspicious locking because manager->lock gets unlocked and then a few lines later locked again within _peerDisconnected.

                if (btcPeerConnectStatus(info->peer) == BRPeerStatusDisconnected) {
                    pthread_mutex_unlock(&manager->lock);
                    _peerDisconnected(info, ENOTCONN);
                    pthread_mutex_lock(&manager->lock);
                    manager->peerThreadCount--;
                }

The second callsite is a threadCleanup-function of _peerThreadRoutine, as shown in the following code-snippet:

static void *_peerThreadRoutine(void *arg)
{
    BRBitcoinPeerContext *ctx = arg;

    pthread_cleanup_push(ctx->threadCleanup, ctx->info);

    // Do stuff, attempt to establish socket...

    if (ctx->disconnected) ctx->disconnected(ctx->info, error);
    pthread_cleanup_pop(1); // -> seems to invoke _peerDisconnected via the function-pointer ctx->threadCleanup
    return NULL;
}

Now my suspection is that those callsites are racing against each other.
In particular, I observed that _peerDisconnected got invoked with a garbage-info-struct that led to segmentation-faults within btcPeerFree.

Can you confirm that there are troubles with the multithreaded cleanup of BRBitcoinPeers?

@cryptodev100
Copy link
Contributor Author

cryptodev100 commented Dec 3, 2021

This PR might be related to breadwallet/breadwallet-core#295, but breadwallet/breadwallet-core#295 is already outdated and I only tested with new code.

@EBGToo EBGToo requested a review from voisine January 5, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants