What the developer wrote
Authored by Lee *!* Clagett
Fix DNS resolve UB
What changed, and why it matters
This patch fixes a threading bug in Monero's peer-discovery code. The old code launched background threads that wrote into a local variable (`dns_results`) owned by the main function. If the main function finished or the variable went out of scope while a thread was still running, the thread would write to freed memory. That is undefined behavior and could crash the node or, in theory, be abused to corrupt memory. The fix moves the shared data into a heap-allocated structure kept alive with a smart pointer and protects it with a mutex, so threads can finish safely even if the main code no longer waits for them.
Treat this as a stability and potential security fix. Nodes should upgrade to a build containing this commit. Operators running Monero daemons, especially on musl or other small-stack libc builds where the timeout path is more likely, should prioritize the update. No immediate workaround is described in the commit.
Security signals we found
Use-after-free/undefined behavior in multi-threaded DNS resolution
Stack-allocated shared data captured by reference in detached worker threads
Thread interruption and timeout handling that assumed out-of-scope variables were still valid
Race condition between worker threads writing results and main thread reading them
Memory corruption potential in P2P bootstrap path
Evidence from the diff
The commit refactors DNS seed-node resolution in src/p2p/net_node.inl. Previously, worker lambdas captured &dns_results (a stack-allocated std::vector) and &addr_str (a loop reference). The main thread then interrupt()ed and tried to join threads on timeout. If a thread outlived the vector’s scope, it wrote to dangling storage, producing use-after-free / undefined behavior. The patch introduces a frame_t containing dns_results and a boost::mutex, allocated as std::shared_ptr. Threads capture a std::weak_ptr and lock the frame before writing. On timeout, the main thread now detach()es instead of interrupting, removing the unsafe scope assumption. The change also removes the thread_interrupted catch path and adds a mutex guard around final result iteration.
Changed components
src/p2p/net_node.inlMonero P2P seed node DNS resolutionBoost.Thread worker pool for DNS lookupsInspect captured patch +20 / −23
diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl
index 82ba991..c88b96e 100644
--- a/src/p2p/net_node.inl
+++ b/src/p2p/net_node.inl
@@ -764,8 +764,14 @@ namespace nodetool
// TODO: at some point add IPv6 support, but that won't be relevant
// for some time yet.
- std::vector<std::vector<std::string>> dns_results;
- dns_results.resize(m_seed_nodes_list.size());
+ struct frame_t
+ {
+ std::vector<std::vector<std::string>> dns_results;
+ boost::mutex sync;
+ };
+
+ const auto frame = std::make_shared<frame_t>();
+ frame->dns_results.resize(m_seed_nodes_list.size());
// some libc implementation provide only a very small stack
// for threads, e.g. musl only gives +- 80kb, which is not
@@ -776,32 +782,22 @@ namespace nodetool
std::list<boost::thread> dns_threads;
uint64_t result_index = 0;
+ const std::weak_ptr<frame_t> frame_weak{frame};
for (const std::string& addr_str : m_seed_nodes_list)
{
- boost::thread th = boost::thread(thread_attributes, [=, &dns_results, &addr_str]
+ boost::thread th = boost::thread(thread_attributes, [frame_weak, addr_str, result_index]
{
MDEBUG("dns_threads[" << result_index << "] created for: " << addr_str);
// TODO: care about dnssec avail/valid
bool avail, valid;
- std::vector<std::string> addr_list;
-
- try
- {
- addr_list = tools::DNSResolver::instance().get_ipv4(addr_str, avail, valid);
- MDEBUG("dns_threads[" << result_index << "] DNS resolve done");
- boost::this_thread::interruption_point();
- }
- catch(const boost::thread_interrupted&)
+ std::vector<std::string> addr_list = tools::DNSResolver::instance().get_ipv4(addr_str, avail, valid);
+ MINFO("dns_threads[" << result_index << "] addr_str: " << addr_str << " number of results: " << addr_list.size());
+ const auto frame = frame_weak.lock();
+ if (frame)
{
- // thread interruption request
- // even if we now have results, finish thread without setting
- // result variables, which are now out of scope in main thread
- MWARNING("dns_threads[" << result_index << "] interrupted");
- return;
+ const boost::lock_guard<boost::mutex> lock{frame->sync};
+ frame->dns_results.at(result_index) = std::move(addr_list);
}
-
- MINFO("dns_threads[" << result_index << "] addr_str: " << addr_str << " number of results: " << addr_list.size());
- dns_results[result_index] = addr_list;
});
dns_threads.push_back(std::move(th));
@@ -815,14 +811,15 @@ namespace nodetool
{
if (! th.try_join_until(deadline))
{
- MWARNING("dns_threads[" << i << "] timed out, sending interrupt");
- th.interrupt();
+ MWARNING("dns_threads[" << i << "] timed out");
+ th.detach();
}
++i;
}
i = 0;
- for (const auto& result : dns_results)
+ const boost::lock_guard<boost::mutex> lock{frame->sync};
+ for (const auto& result : frame->dns_results)
{
MDEBUG("DNS lookup for " << m_seed_nodes_list[i] << ": " << result.size() << " results");
// if no results for node, thread's lookup likely timed out
Why this scored 54/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.