From 7dbf76d0da5c78b8e987ce3fe1bf25781b02b82e Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Mon, 9 Oct 2017 16:46:42 +0100 Subject: [PATCH] Fix an object lifetime bug in net load tests The commands handler must not be destroyed before the config object, or we'll be accessing freed memory. An earlier attempt at using boost::shared_ptr to control object lifetime turned out to be very invasive, though would be a better solution in theory. --- contrib/epee/include/net/levin_base.h | 1 + contrib/epee/include/net/levin_client_async.h | 12 +++++++++--- .../epee/include/net/levin_protocol_handler.h | 2 ++ .../include/net/levin_protocol_handler_async.h | 16 ++++++++++++++-- contrib/epee/tests/src/net/test_net.h | 2 +- src/p2p/net_node.inl | 2 +- tests/net_load_tests/clt.cpp | 9 +++++---- tests/net_load_tests/net_load_tests.h | 5 +++++ tests/net_load_tests/srv.cpp | 4 ++-- .../epee_levin_protocol_handler_async.cpp | 8 +++++--- 10 files changed, 45 insertions(+), 16 deletions(-) diff --git a/contrib/epee/include/net/levin_base.h b/contrib/epee/include/net/levin_base.h index d630bff19..7d060f5ef 100644 --- a/contrib/epee/include/net/levin_base.h +++ b/contrib/epee/include/net/levin_base.h @@ -87,6 +87,7 @@ namespace levin virtual void on_connection_new(t_connection_context& context){}; virtual void on_connection_close(t_connection_context& context){}; + virtual ~levin_commands_handler(){} }; #define LEVIN_OK 0 diff --git a/contrib/epee/include/net/levin_client_async.h b/contrib/epee/include/net/levin_client_async.h index 337d345c4..6c8f9bcb3 100644 --- a/contrib/epee/include/net/levin_client_async.h +++ b/contrib/epee/include/net/levin_client_async.h @@ -52,6 +52,7 @@ namespace levin class levin_client_async { levin_commands_handler* m_pcommands_handler; + void (*commands_handler_destroy)(levin_commands_handler*); volatile uint32_t m_is_stop; volatile uint32_t m_threads_count; ::critical_section m_send_lock; @@ -85,9 +86,9 @@ namespace levin ::critical_section m_connection_lock; net_utils::blocked_mode_client m_transport; public: - levin_client_async():m_pcommands_handler(NULL), m_is_stop(0), m_threads_count(0), m_invoke_data_ready(0), m_invoke_is_active(0) + levin_client_async():m_pcommands_handler(NULL), commands_handler_destroy(NULL), m_is_stop(0), m_threads_count(0), m_invoke_data_ready(0), m_invoke_is_active(0) {} - levin_client_async(const levin_client_async& /*v*/):m_pcommands_handler(NULL), m_is_stop(0), m_threads_count(0), m_invoke_data_ready(0), m_invoke_is_active(0) + levin_client_async(const levin_client_async& /*v*/):m_pcommands_handler(NULL), commands_handler_destroy(NULL), m_is_stop(0), m_threads_count(0), m_invoke_data_ready(0), m_invoke_is_active(0) {} ~levin_client_async() { @@ -97,11 +98,16 @@ namespace levin while(boost::interprocess::ipcdetail::atomic_read32(&m_threads_count)) ::Sleep(100); + + set_handler(NULL); } - void set_handler(levin_commands_handler* phandler) + void set_handler(levin_commands_handler* phandler, void (*destroy)(levin_commands_handler*) = NULL) { + if (commands_handler_destroy && m_pcommands_handler) + (*commands_handler_destroy)(m_pcommands_handler); m_pcommands_handler = phandler; + m_pcommands_handler_destroy = destroy; } bool connect(uint32_t ip, uint32_t port, uint32_t timeout) diff --git a/contrib/epee/include/net/levin_protocol_handler.h b/contrib/epee/include/net/levin_protocol_handler.h index fbc9727e2..b3a75bedc 100644 --- a/contrib/epee/include/net/levin_protocol_handler.h +++ b/contrib/epee/include/net/levin_protocol_handler.h @@ -43,6 +43,8 @@ namespace levin struct protocl_handler_config { levin_commands_handler* m_pcommands_handler; + void (*m_pcommands_handler_destroy)(levin_commands_handler*); + ~protocl_handler_config() { if (m_pcommands_handler && m_pcommands_handler_destroy) (*m_pcommands_handler_destroy)(m_pcommands_handler); } }; template diff --git a/contrib/epee/include/net/levin_protocol_handler_async.h b/contrib/epee/include/net/levin_protocol_handler_async.h index 779f4e78f..7ad6d198b 100644 --- a/contrib/epee/include/net/levin_protocol_handler_async.h +++ b/contrib/epee/include/net/levin_protocol_handler_async.h @@ -72,9 +72,11 @@ class async_protocol_handler_config friend class async_protocol_handler; + levin_commands_handler* m_pcommands_handler; + void (*m_pcommands_handler_destroy)(levin_commands_handler*); + public: typedef t_connection_context connection_context; - levin_commands_handler* m_pcommands_handler; uint64_t m_max_packet_size; uint64_t m_invoke_timeout; @@ -91,8 +93,9 @@ public: template bool for_connection(const boost::uuids::uuid &connection_id, callback_t cb); size_t get_connections_count(); + void set_handler(levin_commands_handler* handler, void (*destroy)(levin_commands_handler*) = NULL); - async_protocol_handler_config():m_pcommands_handler(NULL), m_max_packet_size(LEVIN_DEFAULT_MAX_PACKET_SIZE) + async_protocol_handler_config():m_pcommands_handler(NULL), m_pcommands_handler_destroy(NULL), m_max_packet_size(LEVIN_DEFAULT_MAX_PACKET_SIZE) {} void del_out_connections(size_t count); }; @@ -832,6 +835,15 @@ size_t async_protocol_handler_config::get_connections_coun } //------------------------------------------------------------------------------------------ template +void async_protocol_handler_config::set_handler(levin_commands_handler* handler, void (*destroy)(levin_commands_handler*)) +{ + if (m_pcommands_handler && m_pcommands_handler_destroy) + (*m_pcommands_handler_destroy)(m_pcommands_handler); + m_pcommands_handler = handler; + m_pcommands_handler_destroy = destroy; +} +//------------------------------------------------------------------------------------------ +template int async_protocol_handler_config::notify(int command, const std::string& in_buff, boost::uuids::uuid connection_id) { async_protocol_handler* aph; diff --git a/contrib/epee/tests/src/net/test_net.h b/contrib/epee/tests/src/net/test_net.h index 5b21036bb..2e1b1e5fd 100644 --- a/contrib/epee/tests/src/net/test_net.h +++ b/contrib/epee/tests/src/net/test_net.h @@ -155,7 +155,7 @@ namespace tests bool init(const std::string& bind_port = "", const std::string& bind_ip = "0.0.0.0") { - m_net_server.get_config_object().m_pcommands_handler = this; + m_net_server.get_config_object().set_handler(this); m_net_server.get_config_object().m_invoke_timeout = 1000; LOG_PRINT_L0("Binding on " << bind_ip << ":" << bind_port); return m_net_server.init_server(bind_port, bind_ip); diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl index 6162d649b..ee3ff160a 100644 --- a/src/p2p/net_node.inl +++ b/src/p2p/net_node.inl @@ -560,7 +560,7 @@ namespace nodetool //configure self m_net_server.set_threads_prefix("P2P"); - m_net_server.get_config_object().m_pcommands_handler = this; + m_net_server.get_config_object().set_handler(this); m_net_server.get_config_object().m_invoke_timeout = P2P_DEFAULT_INVOKE_TIMEOUT; m_net_server.set_connection_filter(this); diff --git a/tests/net_load_tests/clt.cpp b/tests/net_load_tests/clt.cpp index 376d7ee53..c930a5b57 100644 --- a/tests/net_load_tests/clt.cpp +++ b/tests/net_load_tests/clt.cpp @@ -193,7 +193,7 @@ namespace { m_thread_count = (std::max)(min_thread_count, boost::thread::hardware_concurrency() / 2); - m_tcp_server.get_config_object().m_pcommands_handler = &m_commands_handler; + m_tcp_server.get_config_object().set_handler(&m_commands_handler); m_tcp_server.get_config_object().m_invoke_timeout = CONNECTION_TIMEOUT; ASSERT_TRUE(m_tcp_server.init_server(clt_port, "127.0.0.1")); @@ -238,9 +238,10 @@ namespace static void TearDownTestCase() { // Stop server - test_levin_commands_handler commands_handler; - test_tcp_server tcp_server(epee::net_utils::e_connection_type_NET); - tcp_server.get_config_object().m_pcommands_handler = &commands_handler; + test_levin_commands_handler *commands_handler_ptr = new test_levin_commands_handler(); + test_levin_commands_handler &commands_handler = *commands_handler_ptr; + test_tcp_server tcp_server(epee::net_utils::e_connection_type_RPC); + tcp_server.get_config_object().set_handler(commands_handler_ptr, [](epee::levin::levin_commands_handler *handler)->void { delete handler; }); tcp_server.get_config_object().m_invoke_timeout = CONNECTION_TIMEOUT; if (!tcp_server.init_server(clt_port, "127.0.0.1")) return; diff --git a/tests/net_load_tests/net_load_tests.h b/tests/net_load_tests/net_load_tests.h index f74282683..ce9d8b6fe 100644 --- a/tests/net_load_tests/net_load_tests.h +++ b/tests/net_load_tests/net_load_tests.h @@ -151,6 +151,11 @@ namespace net_load_tests bool handle_new_connection(const boost::uuids::uuid& connection_id, bool ignore_close_fails = false) { size_t idx = m_next_opened_conn_idx.fetch_add(1, std::memory_order_relaxed); + if (idx >= m_connections.size()) + { + LOG_PRINT_L0("ERROR: connections overflow"); + exit(1); + } m_connections[idx] = connection_id; size_t prev_connection_count = m_opened_connection_count.fetch_add(1, std::memory_order_relaxed); diff --git a/tests/net_load_tests/srv.cpp b/tests/net_load_tests/srv.cpp index e6dee1639..f0b0889ea 100644 --- a/tests/net_load_tests/srv.cpp +++ b/tests/net_load_tests/srv.cpp @@ -224,8 +224,8 @@ int main(int argc, char** argv) if (!tcp_server.init_server(srv_port, "127.0.0.1")) return 1; - srv_levin_commands_handler commands_handler(tcp_server); - tcp_server.get_config_object().m_pcommands_handler = &commands_handler; + srv_levin_commands_handler *commands_handler = new srv_levin_commands_handler(tcp_server); + tcp_server.get_config_object().set_handler(commands_handler, [](epee::levin::levin_commands_handler *handler) { delete handler; }); tcp_server.get_config_object().m_invoke_timeout = 10000; //tcp_server.get_config_object().m_max_packet_size = max_packet_size; diff --git a/tests/unit_tests/epee_levin_protocol_handler_async.cpp b/tests/unit_tests/epee_levin_protocol_handler_async.cpp index d2aa31555..c749c2531 100644 --- a/tests/unit_tests/epee_levin_protocol_handler_async.cpp +++ b/tests/unit_tests/epee_levin_protocol_handler_async.cpp @@ -187,9 +187,11 @@ namespace typedef std::unique_ptr test_connection_ptr; - async_protocol_handler_test() + async_protocol_handler_test(): + m_pcommands_handler(new test_levin_commands_handler()), + m_commands_handler(*m_pcommands_handler) { - m_handler_config.m_pcommands_handler = &m_commands_handler; + m_handler_config.set_handler(m_pcommands_handler, [](epee::levin::levin_commands_handler *handler) { delete handler; }); m_handler_config.m_invoke_timeout = invoke_timeout; m_handler_config.m_max_packet_size = max_packet_size; } @@ -212,7 +214,7 @@ namespace protected: boost::asio::io_service m_io_service; test_levin_protocol_handler_config m_handler_config; - test_levin_commands_handler m_commands_handler; + test_levin_commands_handler *m_pcommands_handler, &m_commands_handler; }; class positive_test_connection_to_levin_protocol_handler_calls : public async_protocol_handler_test