wallet2: fix data races in refresh error handlers
What the developer wrote
Authored by Thomas
wallet2: fix data races in refresh error handlers
What changed, and why it matters
This patch adds two calls to wait for background worker threads to finish before handling certain refresh errors in the Monero wallet. Without these waits, the wallet could read or modify shared data while background threads are still running, causing a 'data race.' Data races can lead to crashes, corrupted wallet state, or unpredictable behavior during synchronization. The patch is small and clearly aimed at making error handling safer, but it does not by itself prove that an attacker can reliably exploit the race from the outside.
Apply the patch. After applying, review other exception exit paths in refresh() to ensure waiter.wait() or equivalent synchronization is present before any shared-state access. Consider running ThreadSanitizer on wallet refresh scenarios involving daemon errors or hash chain resets.
Security signals we found
Data race in multi-threaded wallet refresh error path
Missing thread synchronization before shared-state mutation
Potential use of shared blockchain state while background workers are active
Crash/corruption risk during daemon synchronization errors
Evidence from the diff
In wallet2::refresh(), two exception handlers—one for out_of_hashchain_bounds_error and one for the generic std::exception during block parsing—now call waiter.wait() before touching shared state (m_blockchain, exception/error flags, etc.). The refresh() function uses a waiter object to manage parallel block download/parse tasks. If an exception is thrown while workers are still active, the main thread previously proceeded to reset the hash chain or set exception/error state without synchronizing with those workers. This creates data races on wallet state. The fix synchronizes the threads before any shared-state mutation in the error paths.
Changed components
src/wallet/wallet2.cppwallet2::refresh()refresh error handlerswaiter synchronization objectInspect captured patch +2 / −0
diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp
index 91a5420..fbcc8cf 100644
--- a/src/wallet/wallet2.cpp
+++ b/src/wallet/wallet2.cpp
@@ -4177,6 +4177,7 @@ void wallet2::refresh(bool trusted_daemon, uint64_t start_height, uint64_t & blo
}
catch (const tools::error::out_of_hashchain_bounds_error&)
{
+ waiter.wait();
MINFO("Daemon claims next refresh block is out of hash chain bounds, resetting hash chain");
uint64_t stop_height = m_blockchain.offset();
std::vector<crypto::hash> tip(m_blockchain.size() - m_blockchain.offset());
@@ -4200,6 +4201,7 @@ void wallet2::refresh(bool trusted_daemon, uint64_t start_height, uint64_t & blo
}
catch (const std::exception &e)
{
+ waiter.wait();
MERROR("Error parsing blocks: " << e.what());
exception = std::current_exception();
error = true;
Why this scored 40/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.