From 06ebdc886fcb4ca22f695bafe0956cff6d70a250 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Fri, 20 May 2022 05:31:34 +1000 Subject: [PATCH 1/4] net/net_processing: add missing thread safety annotations --- src/net.h | 2 +- src/net_processing.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/net.h b/src/net.h index 7156a2259f8..064c448264c 100644 --- a/src/net.h +++ b/src/net.h @@ -368,7 +368,7 @@ public: RecursiveMutex cs_vProcessMsg; std::list vProcessMsg GUARDED_BY(cs_vProcessMsg); - size_t nProcessQueueSize{0}; + size_t nProcessQueueSize GUARDED_BY(cs_vProcessMsg){0}; RecursiveMutex cs_sendProcessing; diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b83034463d4..fff7f86d781 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -289,7 +289,7 @@ struct Peer { * non-wtxid-relay peers, wtxid for wtxid-relay peers). We use the * mempool to sort transactions in dependency order before relay, so * this does not have to be sorted. */ - std::set m_tx_inventory_to_send; + std::set m_tx_inventory_to_send GUARDED_BY(m_tx_inventory_mutex); /** Whether the peer has requested us to send our complete mempool. Only * permitted if the peer has NetPermissionFlags::Mempool. See BIP35. */ bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false}; @@ -648,7 +648,7 @@ private: std::atomic m_best_height{-1}; /** Next time to check for stale tip */ - std::chrono::seconds m_stale_tip_check_time{0s}; + std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s}; /** Whether this node is running in -blocksonly mode */ const bool m_ignore_incoming_txs; @@ -657,7 +657,7 @@ private: /** Whether we've completed initial sync yet, for determining when to turn * on extra block-relay-only peers. */ - bool m_initial_sync_finished{false}; + bool m_initial_sync_finished GUARDED_BY(cs_main){false}; /** Protects m_peer_map. This mutex must not be locked while holding a lock * on any of the mutexes inside a Peer object. */ From bbec32c9ad2fe213314db9d39aa1eacff2e0bc23 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Sat, 5 Mar 2022 04:09:35 +1000 Subject: [PATCH 2/4] net: mark TransportSerializer/m_serializer as const The (V1)TransportSerializer instance CNode::m_serializer is used from multiple threads via PushMessage without protection by a mutex. This is only thread safe because the class does not have any mutable state, so document that by marking the methods and the object as "const". --- src/net.cpp | 3 ++- src/net.h | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index c2e45898e80..cf0c9aef7ba 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -797,7 +797,8 @@ CNetMessage V1TransportDeserializer::GetMessage(const std::chrono::microseconds return msg; } -void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector& header) { +void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector& header) const +{ // create dbl-sha256 checksum uint256 hash = Hash(msg.data); diff --git a/src/net.h b/src/net.h index 064c448264c..97389314b2b 100644 --- a/src/net.h +++ b/src/net.h @@ -325,13 +325,13 @@ public: class TransportSerializer { public: // prepare message for transport (header construction, error-correction computation, payload encryption, etc.) - virtual void prepareForTransport(CSerializedNetMsg& msg, std::vector& header) = 0; + virtual void prepareForTransport(CSerializedNetMsg& msg, std::vector& header) const = 0; virtual ~TransportSerializer() {} }; -class V1TransportSerializer : public TransportSerializer { +class V1TransportSerializer : public TransportSerializer { public: - void prepareForTransport(CSerializedNetMsg& msg, std::vector& header) override; + void prepareForTransport(CSerializedNetMsg& msg, std::vector& header) const override; }; /** Information about a peer */ @@ -342,7 +342,7 @@ class CNode public: std::unique_ptr m_deserializer; - std::unique_ptr m_serializer; + std::unique_ptr m_serializer; NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; From ef26f2f421071986a3878a1a94b0149ae8e16fcd Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Fri, 20 May 2022 05:37:54 +1000 Subject: [PATCH 3/4] net: mark CNode unique_ptr members as const Dereferencing a unique_ptr is not necessarily thread safe. The reason these are safe is because their values are set at construction and do not change later; so mark them as const and set them via the initializer list to guarantee that. --- src/net.cpp | 7 +++---- src/net.h | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index cf0c9aef7ba..535a5ce8e2d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2723,7 +2723,9 @@ CNode::CNode(NodeId idIn, ConnectionType conn_type_in, bool inbound_onion, std::unique_ptr&& i2p_sam_session) - : m_sock{sock}, + : m_deserializer{std::make_unique(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))}, + m_serializer{std::make_unique(V1TransportSerializer())}, + m_sock{sock}, m_connected{GetTime()}, addr{addrIn}, addrBind{addrBindIn}, @@ -2746,9 +2748,6 @@ CNode::CNode(NodeId idIn, } else { LogPrint(BCLog::NET, "Added connection peer=%d\n", id); } - - m_deserializer = std::make_unique(V1TransportDeserializer(Params(), id, SER_NETWORK, INIT_PROTO_VERSION)); - m_serializer = std::make_unique(V1TransportSerializer()); } bool CConnman::NodeFullyConnected(const CNode* pnode) diff --git a/src/net.h b/src/net.h index 97389314b2b..d062b8a9ffb 100644 --- a/src/net.h +++ b/src/net.h @@ -341,8 +341,8 @@ class CNode friend struct ConnmanTestMsg; public: - std::unique_ptr m_deserializer; - std::unique_ptr m_serializer; + const std::unique_ptr m_deserializer; // Used only by SocketHandler thread + const std::unique_ptr m_serializer; NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; From 9816dc96b77fd9780bb97891cd45a1b9798db8f5 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Sat, 5 Mar 2022 04:44:58 +1000 Subject: [PATCH 4/4] net: note CNode members that are treated as const m_permissionFlags and m_prefer_evict are treated as const -- they're only set immediately after construction before any other thread has access to the object, and not changed again afterwards. As such they don't need to be marked atomic or guarded by a mutex; though it would probably be better to actually mark them as const... --- src/net.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/net.h b/src/net.h index d062b8a9ffb..abdfef85f8e 100644 --- a/src/net.h +++ b/src/net.h @@ -344,7 +344,7 @@ public: const std::unique_ptr m_deserializer; // Used only by SocketHandler thread const std::unique_ptr m_serializer; - NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; + NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester /** * Socket used for communication with the node. @@ -393,7 +393,7 @@ public: * from the wire. This cleaned string can safely be logged or displayed. */ std::string cleanSubVer GUARDED_BY(m_subver_mutex){}; - bool m_prefer_evict{false}; // This peer is preferred for eviction. + bool m_prefer_evict{false}; // This peer is preferred for eviction. (treated as const) bool HasPermission(NetPermissionFlags permission) const { return NetPermissions::HasFlag(m_permissionFlags, permission); }