fuzz: Restore SendMessages coverage in process_message(s) fuzz targets #34302
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34302. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
2ee061b to
fa0cb02
Compare
|
The CI fails, because I forgot to re-seat the dangling pointer. So I guess it is nice to know that the CI will catch this issue, going forward. Not sure how to re-seat the pointer. I guess I could use placement new to just overwrite the memory, but I guess this is UB without |
The addrman field is already a reference. However, some tests would benefit from the reference being re-seatable, so that they do not have to create a full Connman each time.
fa0cb02 to
fabf8d1
Compare
|
|
||
| connman.SetMsgProc(peerman.get()); | ||
| connman.SetMsgProc(node.peerman.get()); | ||
| connman.SetAddrman(*node.addrman); |
There was a problem hiding this comment.
Is it possible that one of connman's threads tries to use addrman after node.addrman.reset() but before connman.SetAddrman?
There was a problem hiding this comment.
In theory, yes. In practise, it should not, because all nodes are stopped in the end: node.connman->StopNodes();. Also, connman should not try to ask the addrman to open connections to remote nodes during any of the unit tests.
So I guess, if the fuzz test were to run into a use-after-free here, it would be something to look into and fix (likely all unit tests will be affected).
So I think it is better to leave as-is?
There was a problem hiding this comment.
Makes sense, I'm ok leaving it as-is and fixing a use-after-free if it comes up.
|
crACK fabf8d1 |
|
For reference, I used the following patch to confirm that this pull request fixes the coverage problem and that the mentioned commit introduced the coverage problem. (When the assert is hit, coverage exists. When the fuzz program runs fine, no coverage exists): a diffdiff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 1202eaf152..099ef3eb5e 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5680,2 +5680,5 @@ bool PeerManagerImpl::SetupAddressRelay(const CNode& node, Peer& peer)
+extern bool g_crash;
+bool g_crash{true};
+
bool PeerManagerImpl::SendMessages(CNode* pto)
@@ -5687,2 +5690,3 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
if (!peer) return false;
+ assert(!g_crash); // is this line covered ?
const Consensus::Params& consensusParams = m_chainparams.GetConsensus();
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp
index ef8cb686ce..dfefafd61b 100644
--- a/src/test/fuzz/process_message.cpp
+++ b/src/test/fuzz/process_message.cpp
@@ -32,2 +32,3 @@
+extern bool g_crash;
namespace {
@@ -67,2 +68,3 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
{
+ g_crash = false;
SeedRandomStateForTest(SeedRand::ZEROS);
@@ -105,2 +107,3 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
FillNode(fuzzed_data_provider, connman, p2p_node);
+ g_crash = true;
diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp
index f36f528b0e..a1c006bade 100644
--- a/src/test/fuzz/process_messages.cpp
+++ b/src/test/fuzz/process_messages.cpp
@@ -28,2 +28,3 @@
+extern bool g_crash;
namespace {
@@ -57,2 +58,3 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
{
+ g_crash = false;
SeedRandomStateForTest(SeedRand::ZEROS);
@@ -98,2 +100,3 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
+ g_crash = true;
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 30) |
|
Is it possible that these ever create a valid tx given a good dictionary? If so, I think resetting the mempool would be a good idea. I think this is probably irrelevant though. |
Found and reported by Crypt-iQ (thanks!)
Currently the process_message(s) fuzz targets do not have any meaningful
SendMessagescode coverage. This is not ideal.Fix the problem by adding back the coverage, and by hardening the code here, so that the problem hopefully does not happen again in the future.
Historic context for this regression
The regression was introduced in commit fa11eea, which built a new deterministic peerman object. However, the patch was incomplete, because it was missing one hunk to replace
g_setup->m_node.peerman->SendMessages(&p2p_node);withpeerman->SendMessages(&p2p_node);.This means the stale and empty peerman from the node context and not the freshly created and deterministic peerman was used.
A simple fix would be to just submit the missing patch hunk. However, this still leaves the risk that the issue is re-introduced at any time in the future. So instead, I think the stale and empty peerman should be de-constructed, so that any call to it will lead to a hard sanitizer error and fuzz failure.
Doing that also uncovered another issue: The connman was holding on to a reference to a stale and empty addrman.
So fix all issues by: