TxDownloadManagerImpl retains a reference to PeerManagerImpl::m_rng, which is non-thread-safe and guarded by g_msgproc_mutex.
BlockConnected runs on the validation background thread while holding only m_tx_download_mutex. Reconsidering an orphan with multiple announcers could therefore use m_rng concurrently with message processing.
Regression introduced in #35986
Added a regression test on second commit, can remove it from the PR if deemed superfluous.
This is a Project Loupe find.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36077.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko, hodlinator, sedited |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
@fanquake this needs a 32.x tag as it's not in a release yet
Added a regression test on second commit, can remove it from the PR if deemed superfluous.
Seems fine to drop. I guess an end-to-end sanitizer test would be nicer, but I guess that won't work because tsan can't detect this UB for some reason? Reminds me of #29625 (review), which was only visible via pattern-init-ubsan: #29625 (comment)
TxDownloadManagerImpl retains a reference to PeerManagerImpl::m_rng,
which is non-thread-safe and guarded by g_msgproc_mutex.
BlockConnected runs on the validation background thread while holding
only m_tx_download_mutex. Reconsidering an orphan with multiple
announcers could therefore use m_rng concurrently with message
processing.
Regression introduced in 9cc7dc50bdc9867d079ab7a111d39487a4566767
129 | @@ -128,7 +130,12 @@ class TxDownloadManagerImpl { 130 | return *m_lazy_recent_confirmed_transactions; 131 | } 132 | 133 | - TxDownloadManagerImpl(const TxDownloadOptions& options) : m_opts{options}, m_orphanage{MakeTxOrphanage()}, m_txrequest{options.m_deterministic_txrequest} {} 134 | + TxDownloadManagerImpl(const TxDownloadOptions& options) :
nit: according to clang-format, this : goes on the next line? Also, you forgot to actually remove the (now unused/dead) rng ref to avoid the bug being re-introduced in the futre?
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 6b6594f7da..34958d5e90 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2139,3 +2139,3 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman,
m_mempool(pool),
- m_txdownloadman(node::TxDownloadOptions{pool, m_rng, opts.deterministic_rng}),
+ m_txdownloadman{node::TxDownloadOptions{pool, opts.deterministic_rng}},
m_warnings{warnings},
diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h
index e362110212..ab6d9accbe 100644
--- a/src/node/txdownloadman.h
+++ b/src/node/txdownloadman.h
@@ -41,4 +41,2 @@ struct TxDownloadOptions {
const CTxMemPool& m_mempool;
- /** RNG provided by caller. */
- FastRandomContext& m_rng;
/** Instantiate TxRequestTracker as deterministic (used for tests). */
diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h
index c65b2beed1..f49de20960 100644
--- a/src/node/txdownloadman_impl.h
+++ b/src/node/txdownloadman_impl.h
@@ -132,7 +132,7 @@ public:
- TxDownloadManagerImpl(const TxDownloadOptions& options) :
- m_mempool{options.m_mempool},
- m_rng{options.m_deterministic_txrequest},
- m_orphanage{MakeTxOrphanage()},
- m_txrequest{options.m_deterministic_txrequest}
+ TxDownloadManagerImpl(const TxDownloadOptions& options)
+ : m_mempool{options.m_mempool},
+ m_rng{options.m_deterministic_txrequest},
+ m_orphanage{MakeTxOrphanage()},
+ m_txrequest{options.m_deterministic_txrequest}
{}
diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp
index c7a41da948..8de5664b69 100644
--- a/src/test/fuzz/txdownloadman.cpp
+++ b/src/test/fuzz/txdownloadman.cpp
@@ -176,4 +176,3 @@ FUZZ_TARGET(txdownloadman, .init = initialize)
CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};
- FastRandomContext det_rand{true};
- node::TxDownloadManager txdownloadman{node::TxDownloadOptions{pool, det_rand, true}};
+ node::TxDownloadManager txdownloadman{node::TxDownloadOptions{.m_mempool = pool, .m_deterministic_txrequest = true}};
@@ -300,4 +299,3 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize)
CTxMemPool pool{MemPoolOptionsForTest(g_setup->m_node), error};
- FastRandomContext det_rand{true};
- node::TxDownloadManagerImpl txdownload_impl{node::TxDownloadOptions{pool, det_rand, true}};
+ node::TxDownloadManagerImpl txdownload_impl{node::TxDownloadOptions{.m_mempool = pool, .m_deterministic_txrequest = true}};
diff --git a/src/test/txdownload_tests.cpp b/src/test/txdownload_tests.cpp
index 296daf5fcf..00d824f50c 100644
--- a/src/test/txdownload_tests.cpp
+++ b/src/test/txdownload_tests.cpp
@@ -116,4 +116,3 @@ BOOST_FIXTURE_TEST_CASE(tx_rejection_types, TestChain100Setup)
CTxMemPool& pool = *Assert(m_node.mempool);
- FastRandomContext det_rand{true};
- node::TxDownloadOptions DEFAULT_OPTS{pool, det_rand, true};
+ node::TxDownloadOptions DEFAULT_OPTS{.m_mempool = pool, .m_deterministic_txrequest = true};
@@ -174,4 +173,3 @@ BOOST_FIXTURE_TEST_CASE(handle_missing_inputs, TestChain100Setup)
CTxMemPool& pool = *Assert(m_node.mempool);
- FastRandomContext det_rand{true};
- node::TxDownloadOptions DEFAULT_OPTS{pool, det_rand, true};
+ node::TxDownloadOptions DEFAULT_OPTS{.m_mempool = pool, .m_deterministic_txrequest = true};
NodeId nodeid{1};
taken thanks
review ACK 80eaa6cabf28b7d68c61fc5aced189b36970f17e 🐓
<details><summary>Show signature</summary>
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 80eaa6cabf28b7d68c61fc5aced189b36970f17e 🐓
d/Ew0hJE+aIEghqIX43OwGZI1jw4abRbVGkfl5wEEnULVuGZJ6ZVNF8ddUJojKlU0cQNeaZro7XmYONIfI5/Ag==
</details>
ACK 80eaa6cabf28b7d68c61fc5aced189b36970f17e
It is unfortunate that thread safety annotations despite being applied to PeerManagerImpl::m_rng were not powerful enough to catch the case where PeerManagerImpl gave out a reference to it inside its constructor.
ACK 80eaa6cabf28b7d68c61fc5aced189b36970f17e
Post-merge ACK. FastRandomContext should be cheap to own. Giving the download manager its own RNG instance eliminates the cross-thread reference and race with the message-processing thread.
Milestone
32.0
