test: add llmq message handler fuzz target#7172
test: add llmq message handler fuzz target#7172thepastaclaw wants to merge 3 commits intodashpay:developfrom
Conversation
|
WalkthroughAdds a new fuzz test for LLMQ message handling at src/test/fuzz/llmq_messages.cpp and registers it in src/Makefile.test.include. The harness creates a TestingSetup, advances chain state, consumes fuzzed input to construct structured DKG messages, structured final commitments, or raw payloads, and dispatches them to the quorum block processor, DKG session manager, or quorum manager as appropriate. Execution tolerates ios_base::failure, syncs validation callbacks, and tears down nodes at the end. Sequence Diagram(s)sequenceDiagram
participant FuzzHarness as FuzzHarness
participant TestingSetup as TestingSetup
participant Chain as Chain
participant FuzzedDataProvider as FuzzedDataProvider
participant QuorumBlockProcessor as QuorumBlockProcessor
participant DKGSessionManager as DKGSessionManager
participant QuorumManager as QuorumManager
participant ValidationInterface as ValidationInterface
FuzzHarness->>TestingSetup: initialize environment (enable SPORK_17, pre-mine)
TestingSetup->>Chain: advance blocks to writable state
FuzzHarness->>FuzzedDataProvider: request bytes
FuzzedDataProvider->>FuzzHarness: provide message type + payload
alt QFCOMMITMENT
FuzzHarness->>QuorumBlockProcessor: process final commitment
else DKG-related (QCONTRIB/QCOMPLAINT/QJUSTIFICATION/QPCOMMITMENT/QWATCH)
FuzzHarness->>DKGSessionManager: route DKG message if session exists
else Other quorum message
FuzzHarness->>QuorumManager: process standard quorum message
end
FuzzHarness->>ValidationInterface: sync callbacks
FuzzHarness->>TestingSetup: stop nodes / teardown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/fuzz/llmq_messages.cpp (2)
175-198: Consider broadening the exception filter beyondstd::ios_base::failure.With fuzzed/malformed data,
ProcessMessagehandlers may throw other exception types (e.g.,std::out_of_range,std::runtime_error) from internal validation paths. Catching onlyios_base::failuremeans any other exception becomes a fuzzer "crash" — a false positive that masks real coverage. Consider catchingconst std::exception&to filter out all expected rejection paths, or at minimum addstd::out_of_rangeandstd::runtime_error.Proposed fix
- } catch (const std::ios_base::failure&) { + } catch (const std::ios_base::failure&) { + } catch (const std::out_of_range&) { }Or more broadly:
- } catch (const std::ios_base::failure&) { + } catch (const std::exception&) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/llmq_messages.cpp` around lines 175 - 198, The current try/catch only swallows std::ios_base::failure which lets other common exceptions from ProcessMessage bubble up and produce false-positive crashes; update the catch to handle a broader set of expected exceptions (e.g., replace or add to the handler so it catches const std::exception& or at minimum also catches std::out_of_range and std::runtime_error) around the calls to g_setup->m_node.llmq_ctx->quorum_block_processor->ProcessMessage, dkg_session_manager->ProcessMessage and g_setup->m_node.llmq_ctx->qman->ProcessMessage before calling g_setup->m_node.peerman->PostProcessMessage so malformed/fuzzed input is safely filtered.
131-133: Nit:-txreconciliationappears unrelated to LLMQ message fuzzing.This flag enables Erlay transaction reconciliation, which has no bearing on LLMQ message processing. Consider removing it to keep the setup minimal and reduce the initialization surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/llmq_messages.cpp` around lines 131 - 133, The test setup passed an unrelated "-txreconciliation" flag into MakeNoLogFileContext for TestingSetup; remove "-txreconciliation" from the extra_args list so the call becomes MakeNoLogFileContext<const TestingSetup>(CBaseChainParams::REGTEST, /*extra_args=*/{"-watchquorums=1"}) to keep initialization minimal and focused on LLMQ message fuzzing (refer to testing_setup, MakeNoLogFileContext, and TestingSetup to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/fuzz/llmq_messages.cpp`:
- Around line 175-198: The current try/catch only swallows
std::ios_base::failure which lets other common exceptions from ProcessMessage
bubble up and produce false-positive crashes; update the catch to handle a
broader set of expected exceptions (e.g., replace or add to the handler so it
catches const std::exception& or at minimum also catches std::out_of_range and
std::runtime_error) around the calls to
g_setup->m_node.llmq_ctx->quorum_block_processor->ProcessMessage,
dkg_session_manager->ProcessMessage and
g_setup->m_node.llmq_ctx->qman->ProcessMessage before calling
g_setup->m_node.peerman->PostProcessMessage so malformed/fuzzed input is safely
filtered.
- Around line 131-133: The test setup passed an unrelated "-txreconciliation"
flag into MakeNoLogFileContext for TestingSetup; remove "-txreconciliation" from
the extra_args list so the call becomes MakeNoLogFileContext<const
TestingSetup>(CBaseChainParams::REGTEST, /*extra_args=*/{"-watchquorums=1"}) to
keep initialization minimal and focused on LLMQ message fuzzing (refer to
testing_setup, MakeNoLogFileContext, and TestingSetup to locate the change).
thepastaclaw
changed the title
thepastaclaw
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/util/data/non-backported.txt`:
- Line 53: The file test/util/data/non-backported.txt contains a concatenated
entry "src/test/fuzz/llmq_messages.cppsrc/test/bls_tests.cpp" which prevents
clang-format from seeing either file; edit test/util/data/non-backported.txt to
split that single line into two separate lines containing
"src/test/fuzz/llmq_messages.cpp" and "src/test/bls_tests.cpp" (one path per
line) so both .cpp files are correctly recognized by the clang-format runner.
thepastaclaw
force-pushed
the
fuzz/tracker-108-llmq-messages
branch
from
d86de49 to
5c6764d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/fuzz/llmq_messages.cpp (2)
158-191: DKG message-type subset duplicated — consider extracting to a constant.The set
{QCONTRIB, QCOMPLAINT, QJUSTIFICATION, QPCOMMITMENT, QWATCH}is written out in full twice: once to select the structured builder (lines 162–165) and once to route to the DKG session manager (lines 180–182). Adding or removing a message type from one but not the other would silently cause the builder and the dispatcher to disagree.♻️ Proposed refactor
+const std::array<const char*, 5> DKG_SESSION_MSG_TYPES{ + NetMsgType::QCONTRIB, NetMsgType::QCOMPLAINT, NetMsgType::QJUSTIFICATION, + NetMsgType::QPCOMMITMENT, NetMsgType::QWATCH, +}; // ... inside the loop: - if ((msg_type == NetMsgType::QCONTRIB || msg_type == NetMsgType::QCOMPLAINT || - msg_type == NetMsgType::QJUSTIFICATION || msg_type == NetMsgType::QPCOMMITMENT || - msg_type == NetMsgType::QWATCH) && - fuzzed_data_provider.ConsumeBool()) { + if (std::ranges::find(DKG_SESSION_MSG_TYPES, msg_type) != DKG_SESSION_MSG_TYPES.end() && + fuzzed_data_provider.ConsumeBool()) { return BuildStructuredDKGMessage(fuzzed_data_provider, chainstate.m_chain); } // ... - if (dkg_session_manager != nullptr && - (msg_type == NetMsgType::QCONTRIB || msg_type == NetMsgType::QCOMPLAINT || - msg_type == NetMsgType::QJUSTIFICATION || msg_type == NetMsgType::QPCOMMITMENT || - msg_type == NetMsgType::QWATCH)) { + if (dkg_session_manager != nullptr && + std::ranges::find(DKG_SESSION_MSG_TYPES, msg_type) != DKG_SESSION_MSG_TYPES.end()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/llmq_messages.cpp` around lines 158 - 191, The DKG message-type set is duplicated; extract the set of NetMsgType values (QCONTRIB, QCOMPLAINT, QJUSTIFICATION, QPCOMMITMENT, QWATCH) into a single constant (e.g., a static constexpr array or std::unordered_set named kDkgMsgTypes) and replace both checks that currently list them inline with a single membership test (e.g., kDkgMsgTypes.contains(msg_type) or std::find). Update the condition that decides to call BuildStructuredDKGMessage (where msg_type is checked before calling BuildStructuredDKGMessage) and the later routing condition that checks dkg_session_manager->ProcessMessage to use this new kDkgMsgTypes constant, ensuring both builder and dispatcher stay in sync.
68-74:BuildStructuredDKGMessagenever reaches structured parsing.
vRecv << payloadserializes avector<unsigned char>with a varint length prefix. DKG parsers expect typed member fields directly after the header, not a length-prefixed blob, so deserialization will throwios_base::failurebefore any interesting logic is exercised. This path provides essentially no additional coverage over the fully-random stream fallback at line 168. Consider writing actual DKG message struct fields (e.g. aCDKGContribution,CDKGComplaint, etc.) based on the selectedmsg_typeto meaningfully reach deeper parser paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/fuzz/llmq_messages.cpp` around lines 68 - 74, The test currently writes a length-prefixed byte vector (payload) into vRecv which prevents BuildStructuredDKGMessage from exercising structured parsing; instead, detect the selected msg_type in the fuzz harness and construct and serialize the corresponding typed DKG message fields (e.g. instantiate and fill a CDKGContribution, CDKGComplaint, CDKGJustification, etc.) directly into vRecv so fields are written in the expected typed order (not as a varint-length blob), using the fuzzed_data_provider to populate their members; ensure you call the appropriate Serialize/Unserialize or stream-operator logic for those types so BuildStructuredDKGMessage can parse them (replace the vRecv << payload branch with type-specific serialization keyed by msg_type).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/fuzz/llmq_messages.cpp`:
- Around line 123-133: After creating testing_setup in initialize_llmq_messages,
add a non-null assertion for the LLMQ context to avoid null-pointer fuzzer
false-positives: explicitly assert that g_setup->m_node.llmq_ctx (or check and
fail-fast) after g_setup is assigned and before any use (the subsequent uses at
lines referencing g_setup->m_node.llmq_ctx). This mirrors the safety in
dkg_session_manager and ensures the fuzzer will not report a spurious null
dereference.
- Around line 146-147: The test leaks CNode instances because AddTestNode stores
raw pointers in connman::m_nodes and StopNodes() doesn't delete them; call
connman.ClearTestNodes() during test cleanup (after the fuzz loop or before
shutdown) to free nodes. Locate the code using CNode, AddTestNode and the
connman instance (the fuzz loop that consumes nodes) and insert a call to
connman.ClearTestNodes() so accumulated test nodes are deleted before
StopNodes()/shutdown.
---
Nitpick comments:
In `@src/test/fuzz/llmq_messages.cpp`:
- Around line 158-191: The DKG message-type set is duplicated; extract the set
of NetMsgType values (QCONTRIB, QCOMPLAINT, QJUSTIFICATION, QPCOMMITMENT,
QWATCH) into a single constant (e.g., a static constexpr array or
std::unordered_set named kDkgMsgTypes) and replace both checks that currently
list them inline with a single membership test (e.g.,
kDkgMsgTypes.contains(msg_type) or std::find). Update the condition that decides
to call BuildStructuredDKGMessage (where msg_type is checked before calling
BuildStructuredDKGMessage) and the later routing condition that checks
dkg_session_manager->ProcessMessage to use this new kDkgMsgTypes constant,
ensuring both builder and dispatcher stay in sync.
- Around line 68-74: The test currently writes a length-prefixed byte vector
(payload) into vRecv which prevents BuildStructuredDKGMessage from exercising
structured parsing; instead, detect the selected msg_type in the fuzz harness
and construct and serialize the corresponding typed DKG message fields (e.g.
instantiate and fill a CDKGContribution, CDKGComplaint, CDKGJustification, etc.)
directly into vRecv so fields are written in the expected typed order (not as a
varint-length blob), using the fuzzed_data_provider to populate their members;
ensure you call the appropriate Serialize/Unserialize or stream-operator logic
for those types so BuildStructuredDKGMessage can parse them (replace the vRecv
<< payload branch with type-specific serialization keyed by msg_type).
| void initialize_llmq_messages() | ||
| { | ||
| static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>( | ||
| /*chain_name=*/CBaseChainParams::REGTEST, | ||
| /*extra_args=*/{"-txreconciliation", "-watchquorums=1"}); | ||
| g_setup = testing_setup.get(); | ||
| for (int i = 0; i < 2 * COINBASE_MATURITY; i++) { | ||
| MineBlock(g_setup->m_node, CScript() << OP_TRUE); | ||
| } | ||
| SyncWithValidationInterfaceQueue(); | ||
| } |
There was a problem hiding this comment.
Assert llmq_ctx non-null in the initializer to prevent false-positive fuzzer crashes.
Lines 174 and 189 dereference g_setup->m_node.llmq_ctx without any null-check, but this is never asserted to be initialized after setup. The dkg_session_manager chain at lines 150–153 explicitly handles the null case, creating an inconsistency. If llmq_ctx is ever null (e.g., build-config regression), the fuzzer will report a null-pointer dereference as a crash, generating a false positive that obscures real bugs.
🛡️ Proposed fix
void initialize_llmq_messages()
{
static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(
/*chain_name=*/CBaseChainParams::REGTEST,
/*extra_args=*/{"-txreconciliation", "-watchquorums=1"});
g_setup = testing_setup.get();
+ assert(g_setup->m_node.llmq_ctx);
+ assert(g_setup->m_node.llmq_ctx->quorum_block_processor);
+ assert(g_setup->m_node.llmq_ctx->qman);
for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
MineBlock(g_setup->m_node, CScript() << OP_TRUE);
}
SyncWithValidationInterfaceQueue();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/fuzz/llmq_messages.cpp` around lines 123 - 133, After creating
testing_setup in initialize_llmq_messages, add a non-null assertion for the LLMQ
context to avoid null-pointer fuzzer false-positives: explicitly assert that
g_setup->m_node.llmq_ctx (or check and fail-fast) after g_setup is assigned and
before any use (the subsequent uses at lines referencing
g_setup->m_node.llmq_ctx). This mirrors the safety in dkg_session_manager and
ensures the fuzzer will not report a spurious null dereference.
| CNode& p2p_node = *ConsumeNodeAsUniquePtr(fuzzed_data_provider).release(); | ||
| connman.AddTestNode(p2p_node); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A 15 'AddTestNode\s*\(' --type=cppRepository: dashpay/dash
Length of output: 13621
🏁 Script executed:
rg -n -A 15 'StopNodes\s*\(' --type=cppRepository: dashpay/dash
Length of output: 2590
Missing cleanup call allows test nodes to leak memory.
The nodes added via AddTestNode() are stored as raw pointers in m_nodes, but StopNodes() does not delete them. Other test files (e.g., denialofservice_tests.cpp) explicitly call connman->ClearTestNodes() before cleanup. Add connman.ClearTestNodes() after the fuzz loop or as part of test cleanup to delete the accumulated nodes and prevent memory leaks over multiple fuzz iterations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/fuzz/llmq_messages.cpp` around lines 146 - 147, The test leaks CNode
instances because AddTestNode stores raw pointers in connman::m_nodes and
StopNodes() doesn't delete them; call connman.ClearTestNodes() during test
cleanup (after the fuzz loop or before shutdown) to free nodes. Locate the code
using CNode, AddTestNode and the connman instance (the fuzz loop that consumes
nodes) and insert a call to connman.ClearTestNodes() so accumulated test nodes
are deleted before StopNodes()/shutdown.
|
Superseded by consolidated fuzz branch in #7173 to avoid merge conflicts. |
2 participants
Summary
llmq_messagesfuzz target for Dash-specific LLMQ message handlingCQuorumBlockProcessor::ProcessMessageforQFCOMMITMENTCDKGSessionManager::ProcessMessagefor DKG paths (QCONTRIB,QCOMPLAINT,QJUSTIFICATION,QPCOMMITMENT,QWATCH)CQuorumManager::ProcessMessagefor relatedQGETDATA/QDATApathsValidation
What was tested
./configure --enable-fuzz --disable-shared --disable-bench --disable-tests --with-sanitizers=address,undefined --disable-stacktracesmake -j8PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 FUZZ=llmq_messages ./src/test/fuzz/fuzz | rg '^llmq_messages$'mkdir -p /tmp/llmq_messages_corpushead -c 2048 /dev/urandom > /tmp/llmq_messages_corpus/seedstart=$(date +%s); end=$((start+65)); runs=0; crashes=0; while [ $(date +%s) -lt $end ]; do ASAN_OPTIONS=detect_container_overflow=0 UBSAN_OPTIONS=print_stacktrace=1 FUZZ=llmq_messages ./src/test/fuzz/fuzz /tmp/llmq_messages_corpus >/tmp/llmq_messages_run.log 2>&1; rc=$?; runs=$((runs+1)); if [ $rc -ne 0 ]; then crashes=$((crashes+1)); break; fi; done; duration=$(( $(date +%s) - start )); echo "duration_seconds=$duration runs=$runs crashes=$crashes"; tail -n 5 /tmp/llmq_messages_run.logResults
llmq_messageslisted)duration_seconds=65 runs=255 crashes=0llmq_messages: succeeded against 1 files in 0s.Environment
~/Projects/dash/worktrees/tracker-108address,undefined)