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.

Sign up for GitHub

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

refactor: Improve thread safety analysis by propagating some negative capabilities #25175

Merged
merged 2 commits into from
May 20, 2022
Merged

refactor: Improve thread safety analysis by propagating some negative capabilities #25175

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a6e3c1
refactor: Propagate negative `!m_most_recent_block_mutex` capability
hebasto May 20, 2022
2b3373c
refactor: Propagate negative `!m_tx_relay_mutex` capability
hebasto May 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ struct Peer {
};

/* Initializes a TxRelay struct for this peer. Can be called at most once for a peer. */
TxRelay* SetTxRelay()
TxRelay* SetTxRelay() EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex)
{
LOCK(m_tx_relay_mutex);
Assume(!m_tx_relay);
Expand Down Expand Up @@ -472,15 +472,16 @@ class PeerManagerImpl final : public PeerManager
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void BlockChecked(const CBlock& block, const BlockValidationState& state) override
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) override;
void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) override
EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how useful it is to annotate interface functions with member mutexes. The only allowed caller can't see the mutex, as it is private.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An annotation is not for a caller, rather for Clang Thread Safety Analysis C++ extension.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(An external caller (accessing via the virtual/override) can't see the mutex, but an in-class caller could)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was just thinking that it would be incredibly unlikely or even wrong to even have an in-class caller.


/** Implement NetEventsInterface */
void InitializeNode(CNode* pnode) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex);
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex);
bool SendMessages(CNode* pto) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing)
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex);
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex);

/** Implement PeerManager */
void StartScheduledTasks(CScheduler& scheduler) override;
Expand All @@ -494,7 +495,7 @@ class PeerManagerImpl final : public PeerManager
void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) override
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex);
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex);
void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) override;

private:
Expand Down Expand Up @@ -758,7 +759,8 @@ class PeerManagerImpl final : public PeerManager
/** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */
CTransactionRef FindTxForGetData(const CNode& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main);

void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main);
void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc)
EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main);

/** Process a new block. Perform any post-processing housekeeping */
void ProcessBlock(CNode& node, const std::shared_ptr<const CBlock>& block, bool force_processing);
Expand Down Expand Up @@ -809,7 +811,8 @@ class PeerManagerImpl final : public PeerManager
*/
bool BlockRequestAllowed(const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
bool AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
void ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& inv);
void ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& inv)
EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex);

/**
* Validation logic for compact filters request handling.
Expand Down