Fix two separate data races in async levin commands
What the developer wrote
Authored by Lee Clagett
Fix two separate data races in async levin commands
What changed, and why it matters
This patch fixes two data races in Monero's asynchronous Levin network protocol handler. A data race happens when multiple threads access the same memory at the same time without proper locking, which can cause crashes, hangs, or corrupted state. The fix adds a mutex around shared callback/timer state and removes a separate 'cancel_timer' virtual method that was being called from multiple threads without synchronization. The commit title explicitly calls these 'data races'.
Apply the patch. After patching, run Monero node/daemon under ThreadSanitizer or heavy P2P load to confirm no further races in levin_protocol_handler_async. Review other async timer/callback sites for similar missing synchronization.
Security signals we found
Data race in async network response handler
Unsynchronized access to connection pointer and timer across threads
Use-after-free or double-callback risk if timer fires while response is being processed
Missing synchronization around boost::asio::steady_timer cancel/reset operations
Commit title explicitly identifies 'two separate data races'
Evidence from the diff
The change modifies contrib/epee/include/net/levin_protocol_handler_async.h. It introduces boost::mutex m_sync in anvoke_handler (the async response handler) and protects m_con and m_timer.cancel() calls with unique_lock/lock_guard. It removes the cancel_timer() virtual method and its unsynchronized boolean flags (m_cancel_timer_called, m_timer_cancelled). reset_timer() now locks before calling m_timer.cancel()/expires_after()/async_wait(). The caller in handle_recv() no longer calls cancel_timer() before handle(); it just checks response_handler existence. This addresses races where handle(), cancel(), reset_timer(), and the timer callback could concurrently access m_con and m_timer state.
Changed components
contrib/epee/include/net/levin_protocol_handler_async.hasync_protocol_handler::anvoke_handlerinvoke_response_handler_baseasync_protocol_handler::handle_recvInspect captured patch +29 / −32
diff --git a/contrib/epee/include/net/levin_protocol_handler_async.h b/contrib/epee/include/net/levin_protocol_handler_async.h
index 376f0f1..34093b5 100644
--- a/contrib/epee/include/net/levin_protocol_handler_async.h
+++ b/contrib/epee/include/net/levin_protocol_handler_async.h
@@ -26,6 +26,8 @@
#pragma once
#include <boost/asio/steady_timer.hpp>
+#include <boost/thread/lock_types.hpp>
+#include <boost/thread/mutex.hpp>
#include <boost/uuid/uuid_generators.hpp>
#include <boost/unordered_map.hpp>
@@ -195,7 +197,6 @@ public:
virtual ~invoke_response_handler_base() {}
virtual bool handle(int res, const epee::span<const uint8_t> buff, connection_context& context)=0;
virtual void cancel()=0;
- virtual bool cancel_timer()=0;
virtual bool reset_timer(bool first)=0;
};
template <class callback_t>
@@ -206,13 +207,15 @@ public:
callback_t m_cb;
const std::chrono::milliseconds m_timeout;
const int m_command;
- bool m_cancel_timer_called;
- bool m_timer_cancelled;
+ boost::mutex m_sync; // for callback
void failure(const int rc)
{
std::shared_ptr<net_utils::service_endpoint<derived_handler>> con;
+ boost::unique_lock<boost::mutex> lock{m_sync};
m_con.swap(con);
+ m_timer.cancel();
+ lock.unlock();
if (!con)
return;
@@ -230,8 +233,7 @@ public:
m_cb(std::move(cb)),
m_timeout(timeout),
m_command(command),
- m_cancel_timer_called(false),
- m_timer_cancelled(false)
+ m_sync()
{
if (!m_con)
throw std::logic_error{"Unexpected nullptr connection"};
@@ -244,44 +246,42 @@ public:
virtual bool handle(int res, const epee::span<const uint8_t> buff, typename async_protocol_handler::connection_context& context) override final
{
- if(!cancel_timer())
- return false;
-
std::shared_ptr<net_utils::service_endpoint<derived_handler>> con;
+ boost::unique_lock<boost::mutex> lock{m_sync};
m_con.swap(con);
+ m_timer.cancel();
+ lock.unlock();
+
if (con)
m_cb(res, buff, context);
- return true;
+ return bool(con);
}
virtual void cancel() override final
{
- if(cancel_timer())
- failure(LEVIN_ERROR_CONNECTION_DESTROYED);
- }
- virtual bool cancel_timer() override final
- {
- if(!m_cancel_timer_called)
- {
- m_cancel_timer_called = true;
- m_timer_cancelled = 1 == m_timer.cancel();
- }
- return m_timer_cancelled;
+ failure(LEVIN_ERROR_CONNECTION_DESTROYED);
}
+
virtual bool reset_timer(bool first) override final
{
if (m_command == connection_context::handshake_command() && !first)
return true;
- std::shared_ptr<anvoke_handler> self;
- if (!m_cancel_timer_called && (self = this->weak_from_this().lock()) && (first || m_timer.cancel() > 0))
+
+ auto self = this->weak_from_this().lock();
+ if (self)
{
- m_timer.expires_after(m_timeout);
- m_timer.async_wait([self = std::move(self)](const boost::system::error_code& ec)
+ const boost::lock_guard<boost::mutex> lock{m_sync};
+ if (first || self->m_timer.cancel() > 0)
{
- if(ec != boost::asio::error::operation_aborted)
- self->failure(LEVIN_ERROR_CONNECTION_TIMEDOUT);
- });
- return true;
+ m_timer.expires_after(m_timeout);
+ m_timer.async_wait([self = std::move(self)] (const boost::system::error_code& ec)
+ {
+ if (ec != boost::asio::error::operation_aborted)
+ self->failure(LEVIN_ERROR_CONNECTION_TIMEDOUT);
+ });
+ return true;
+ }
}
+
return false;
}
};
@@ -493,13 +493,10 @@ public:
if(!m_invoke_response_handlers.empty())
{//async call scenario
const std::shared_ptr<invoke_response_handler_base> response_handler = m_invoke_response_handlers.front().lock();
- bool timer_cancelled = false;
- if (response_handler)
- timer_cancelled = response_handler->cancel_timer();
m_invoke_response_handlers.pop_front();
invoke_response_handlers_guard.unlock();
- if(timer_cancelled)
+ if (response_handler)
response_handler->handle(m_current_head.m_return_code, buff_to_invoke, m_connection_context);
}
else
Why this scored 63/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.