From 3fd69b643c9d2ec71337ebc35c6f54461042d480 Mon Sep 17 00:00:00 2001 From: cryptocode Date: Fri, 26 Apr 2019 13:42:56 +0200 Subject: [PATCH] Protect udp_channel members with a mutex (#1930) * Protect udp_channel members with a mutex * Remove return in void function --- nano/core_test/network.cpp | 16 ++++----- nano/core_test/node.cpp | 4 +-- nano/core_test/peer_container.cpp | 6 ++-- nano/node/json_handler.cpp | 5 +-- nano/node/transport/udp.cpp | 6 ++-- nano/node/transport/udp.hpp | 60 +++++++++++++++++++++++++++---- nano/qt/qt.cpp | 5 +-- 7 files changed, 76 insertions(+), 26 deletions(-) diff --git a/nano/core_test/network.cpp b/nano/core_test/network.cpp index 3b5f70cb..b7afaf3f 100644 --- a/nano/core_test/network.cpp +++ b/nano/core_test/network.cpp @@ -92,9 +92,9 @@ TEST (network, send_node_id_handshake) ASSERT_EQ (1, system.nodes[0]->network.size ()); ASSERT_EQ (1, node1->network.size ()); auto list1 (system.nodes[0]->network.udp_channels.list (1)); - ASSERT_EQ (node1->network.endpoint (), list1[0]->endpoint); + ASSERT_EQ (node1->network.endpoint (), list1[0]->get_endpoint ()); auto list2 (node1->network.udp_channels.list (1)); - ASSERT_EQ (system.nodes[0]->network.endpoint (), list2[0]->endpoint); + ASSERT_EQ (system.nodes[0]->network.endpoint (), list2[0]->get_endpoint ()); node1->stop (); } @@ -120,14 +120,14 @@ TEST (network, last_contacted) auto channel2 (system.nodes[0]->network.udp_channels.channel (nano::endpoint (boost::asio::ip::address_v6::loopback (), 24001))); ASSERT_NE (nullptr, channel2); // Make sure last_contact gets updated on receiving a non-handshake message - auto timestamp_before_keepalive = channel2->last_packet_received; + auto timestamp_before_keepalive = channel2->get_last_packet_received (); node1->network.send_keepalive (channel1); while (system.nodes[0]->stats.count (nano::stat::type::message, nano::stat::detail::keepalive, nano::stat::dir::in) < 2) { ASSERT_NO_ERROR (system.poll ()); } ASSERT_EQ (system.nodes[0]->network.size (), 1); - auto timestamp_after_keepalive = channel2->last_packet_received; + auto timestamp_after_keepalive = channel2->get_last_packet_received (); ASSERT_GT (timestamp_after_keepalive, timestamp_before_keepalive); node1->stop (); @@ -2014,11 +2014,11 @@ TEST (network, replace_port) auto channel (system.nodes[0]->network.udp_channels.insert (nano::endpoint (node1->network.endpoint ().address (), 23000), nano::protocol_version)); if (channel) { - channel->node_id = node1->node_id.pub; + channel->set_node_id (node1->node_id.pub); } } auto peers_list (system.nodes[0]->network.udp_channels.list (std::numeric_limits::max ())); - ASSERT_EQ (peers_list[0]->node_id.get (), node1->node_id.pub); + ASSERT_EQ (peers_list[0]->get_node_id ().get (), node1->node_id.pub); nano::transport::channel_udp channel (system.nodes[0]->network.udp_channels, node1->network.endpoint ()); system.nodes[0]->network.send_keepalive (channel); system.deadline_set (5s); @@ -2033,9 +2033,9 @@ TEST (network, replace_port) } ASSERT_EQ (system.nodes[0]->network.udp_channels.size (), 1); auto list1 (system.nodes[0]->network.udp_channels.list (1)); - ASSERT_EQ (node1->network.endpoint (), list1[0]->endpoint); + ASSERT_EQ (node1->network.endpoint (), list1[0]->get_endpoint ()); auto list2 (node1->network.udp_channels.list (1)); - ASSERT_EQ (system.nodes[0]->network.endpoint (), list2[0]->endpoint); + ASSERT_EQ (system.nodes[0]->network.endpoint (), list2[0]->get_endpoint ()); // Remove correct peer (same node ID) system.nodes[0]->network.udp_channels.clean_node_id (nano::endpoint (node1->network.endpoint ().address (), 23000), node1->node_id.pub); ASSERT_EQ (system.nodes[0]->network.udp_channels.size (), 0); diff --git a/nano/core_test/node.cpp b/nano/core_test/node.cpp index 9d33a77e..101a14e5 100644 --- a/nano/core_test/node.cpp +++ b/nano/core_test/node.cpp @@ -2396,10 +2396,10 @@ TEST (node, peers) // Confirm that the peers match with the endpoints we are expecting ASSERT_EQ (1, system.nodes.front ()->network.size ()); auto list1 (system.nodes[0]->network.udp_channels.list (2)); - ASSERT_EQ (system.nodes[1]->network.endpoint (), list1[0]->endpoint); + ASSERT_EQ (system.nodes[1]->network.endpoint (), list1[0]->get_endpoint ()); ASSERT_EQ (1, node->network.size ()); auto list2 (system.nodes[1]->network.udp_channels.list (2)); - ASSERT_EQ (system.nodes[0]->network.endpoint (), list2[0]->endpoint); + ASSERT_EQ (system.nodes[0]->network.endpoint (), list2[0]->get_endpoint ()); // Stop the peer node and check that it is removed from the store system.nodes.front ()->stop (); diff --git a/nano/core_test/peer_container.cpp b/nano/core_test/peer_container.cpp index 5903baa0..2de62bfe 100644 --- a/nano/core_test/peer_container.cpp +++ b/nano/core_test/peer_container.cpp @@ -58,11 +58,11 @@ TEST (peer_container, split) nano::endpoint endpoint2 (boost::asio::ip::address_v6::loopback (), 101); auto channel1 (system.nodes[0]->network.udp_channels.insert (endpoint1, 0)); ASSERT_NE (nullptr, channel1); - channel1->last_packet_received = now - std::chrono::seconds (1); + channel1->set_last_packet_received (now - std::chrono::seconds (1)); system.nodes[0]->network.udp_channels.modify (channel1); auto channel2 (system.nodes[0]->network.udp_channels.insert (endpoint2, 0)); ASSERT_NE (nullptr, channel2); - channel2->last_packet_received = now + std::chrono::seconds (1); + channel2->set_last_packet_received (now + std::chrono::seconds (1)); system.nodes[0]->network.udp_channels.modify (channel2); ASSERT_EQ (2, system.nodes[0]->network.size ()); ASSERT_EQ (2, system.nodes[0]->network.udp_channels.size ()); @@ -70,7 +70,7 @@ TEST (peer_container, split) ASSERT_EQ (1, system.nodes[0]->network.size ()); ASSERT_EQ (1, system.nodes[0]->network.udp_channels.size ()); auto list (system.nodes[0]->network.udp_channels.list (1)); - ASSERT_EQ (endpoint2, list[0]->endpoint); + ASSERT_EQ (endpoint2, list[0]->get_endpoint ()); } TEST (udp_channels, fill_random_clear) diff --git a/nano/node/json_handler.cpp b/nano/node/json_handler.cpp index f6edce26..613bc511 100644 --- a/nano/node/json_handler.cpp +++ b/nano/node/json_handler.cpp @@ -2431,9 +2431,10 @@ void nano::json_handler::peers () { boost::property_tree::ptree pending_tree; pending_tree.put ("protocol_version", std::to_string (channel->network_version)); - if (channel->node_id.is_initialized ()) + auto node_id_l (channel->get_node_id ()); + if (node_id_l.is_initialized ()) { - pending_tree.put ("node_id", channel->node_id.get ().to_account ()); + pending_tree.put ("node_id", node_id_l.get ().to_account ()); } else { diff --git a/nano/node/transport/udp.cpp b/nano/node/transport/udp.cpp index 5e62da7c..cf76ca28 100644 --- a/nano/node/transport/udp.cpp +++ b/nano/node/transport/udp.cpp @@ -323,7 +323,7 @@ nano::endpoint nano::transport::udp_channels::tcp_peer () { result = i->endpoint (); channels.get ().modify (i, [](channel_udp_wrapper & wrapper_a) { - wrapper_a.channel->last_tcp_attempt = std::chrono::steady_clock::now (); + wrapper_a.channel->set_last_tcp_attempt (std::chrono::steady_clock::now ()); }); i = n; } @@ -493,7 +493,7 @@ public: auto channel (node.network.udp_channels.insert (endpoint, message_a.header.version_using)); if (channel) { - channel->node_id = message_a.response->first; + channel->set_node_id (message_a.response->first); } } } @@ -517,7 +517,7 @@ public: auto channel (node.network.udp_channels.channel (endpoint)); if (channel) { - channel->last_packet_received = std::chrono::steady_clock::now (); + channel->set_last_packet_received (std::chrono::steady_clock::now ()); node.network.udp_channels.modify (channel); node.process_message (message_a, channel); } diff --git a/nano/node/transport/udp.hpp b/nano/node/transport/udp.hpp index 7c17ed09..7ee6b31c 100644 --- a/nano/node/transport/udp.hpp +++ b/nano/node/transport/udp.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -25,10 +26,56 @@ namespace transport { return &channels == &other_a.channels && endpoint == other_a.endpoint; } + + nano::endpoint get_endpoint () const + { + std::lock_guard lk (channel_mutex); + return endpoint; + } + + std::chrono::steady_clock::time_point get_last_tcp_attempt () const + { + std::lock_guard lk (channel_mutex); + return last_tcp_attempt; + } + + void set_last_tcp_attempt (std::chrono::steady_clock::time_point const time_a) + { + std::lock_guard lk (channel_mutex); + last_tcp_attempt = time_a; + } + + std::chrono::steady_clock::time_point get_last_packet_received () const + { + std::lock_guard lk (channel_mutex); + return last_packet_received; + } + + void set_last_packet_received (std::chrono::steady_clock::time_point const time_a) + { + std::lock_guard lk (channel_mutex); + last_packet_received = time_a; + } + + boost::optional get_node_id () const + { + std::lock_guard lk (channel_mutex); + return node_id; + } + + void set_node_id (nano::account node_id_a) + { + std::lock_guard lk (channel_mutex); + node_id = node_id_a; + } + + unsigned network_version{ nano::protocol_version }; + + private: + mutable std::mutex channel_mutex; nano::endpoint endpoint; std::chrono::steady_clock::time_point last_tcp_attempt{ std::chrono::steady_clock::time_point () }; std::chrono::steady_clock::time_point last_packet_received{ std::chrono::steady_clock::time_point () }; - unsigned network_version{ nano::protocol_version }; boost::optional node_id{ boost::none }; private: @@ -108,15 +155,15 @@ namespace transport std::shared_ptr channel; nano::endpoint endpoint () const { - return channel->endpoint; + return channel->get_endpoint (); } std::chrono::steady_clock::time_point last_packet_received () const { - return channel->last_packet_received; + return channel->get_last_packet_received (); } std::chrono::steady_clock::time_point last_tcp_attempt () const { - return channel->last_tcp_attempt; + return channel->get_last_tcp_attempt (); } boost::asio::ip::address ip_address () const { @@ -124,9 +171,10 @@ namespace transport } nano::account node_id () const { - if (channel->node_id.is_initialized ()) + auto node_id_l (channel->get_node_id ()); + if (node_id_l.is_initialized ()) { - return channel->node_id.get (); + return node_id_l.get (); } else { diff --git a/nano/qt/qt.cpp b/nano/qt/qt.cpp index c4ab4828..0325852a 100644 --- a/nano/qt/qt.cpp +++ b/nano/qt/qt.cpp @@ -1942,9 +1942,10 @@ void nano_qt::advanced_actions::refresh_peers () version->setData (QVariant (channel->network_version), Qt::DisplayRole); items.push_back (version); QString node_id (""); - if (channel->node_id.is_initialized ()) + auto node_id_l (channel->get_node_id ()); + if (node_id_l.is_initialized ()) { - node_id = channel->node_id.get ().to_account ().c_str (); + node_id = node_id_l.get ().to_account ().c_str (); } items.push_back (new QStandardItem (node_id)); peers_model->appendRow (items);