diff --git a/src/llmq/commitment.cpp b/src/llmq/commitment.cpp index b492df2e0c2d..596c9f8599c8 100644 --- a/src/llmq/commitment.cpp +++ b/src/llmq/commitment.cpp @@ -18,6 +18,7 @@ #include #include +#include #include namespace llmq @@ -234,8 +235,11 @@ bool CheckLLMQCommitment(const llmq::UtilParameters& util_params, const CTransac } if (LogAcceptDebug(BCLog::LLMQ)) { + // Clamp to validMembers.size() because the wire-format DYNBITSET may be smaller than + // llmq_params.size for malformed payloads; VerifySizes() below catches the mismatch. std::stringstream ss; - for (const auto i : util::irange(llmq_params_opt->size)) { + const auto log_size = std::min(llmq_params_opt->size, qcTx.commitment.validMembers.size()); + for (const auto i : util::irange(log_size)) { ss << "v[" << i << "]=" << qcTx.commitment.validMembers[i]; } LogPrint(BCLog::LLMQ, "CFinalCommitment -- %s llmqType[%d] validMembers[%s] signers[]\n", __func__, diff --git a/src/test/llmq_commitment_tests.cpp b/src/test/llmq_commitment_tests.cpp index 00423685851b..cb7fe1530004 100644 --- a/src/test/llmq_commitment_tests.cpp +++ b/src/test/llmq_commitment_tests.cpp @@ -5,12 +5,23 @@ #include #include +#include +#include +#include #include +#include +#include +#include +#include +#include #include +#include #include #include +#include + using namespace llmq; using namespace llmq::testutils; @@ -86,6 +97,94 @@ BOOST_AUTO_TEST_CASE(commitment_verify_sizes_test) BOOST_CHECK(!commitment.VerifySizes(TEST_PARAMS)); } +BOOST_FIXTURE_TEST_CASE(commitment_check_undersized_bitset_debug_log_test, RegTestingSetup) +{ + // Catches the OOB-read regression in CheckLLMQCommitment's debug-log loop + // by capturing log output rather than relying on undefined behaviour to + // trip a sanitizer. The wire-format validMembers DYNBITSET can deserialize + // smaller than llmq_params.size; before the clamp the loop iterated up to + // llmq_params.size and emitted v[0], v[1], ... reading past the bitset. + // With the clamp an empty bitset must produce "validMembers[]". + struct LogCaptureGuard { + const uint64_t saved_categories{LogInstance().GetCategoryMask()}; + std::list>::iterator it; + explicit LogCaptureGuard(std::vector& sink) + { + LogInstance().EnableCategory(BCLog::LLMQ); + it = LogInstance().PushBackCallback( + [&sink](const std::string& msg) { sink.push_back(msg); }); + } + ~LogCaptureGuard() + { + LogInstance().DeleteCallback(it); + if (!(saved_categories & BCLog::LLMQ)) { + LogInstance().DisableCategory(BCLog::LLMQ); + } + } + }; + + std::vector log_lines; + LogCaptureGuard guard{log_lines}; + BOOST_REQUIRE(LogAcceptDebug(BCLog::LLMQ)); + + CFinalCommitmentTxPayload payload; + payload.nVersion = CFinalCommitmentTxPayload::CURRENT_VERSION; + // base_index below sits at height 0, so CheckLLMQCommitment expects + // qcTx.nHeight == 1. Setting it to 2 forces the function to fail with + // "bad-qc-height" after the pre-validation debug-log block but before + // LookupBlockIndex / Verify / VerifySizes, which is the path the clamp + // is meant to harden. + payload.nHeight = 2; + payload.commitment.nVersion = CFinalCommitment::LEGACY_BLS_NON_INDEXED_QUORUM_VERSION; + payload.commitment.llmqType = TEST_PARAMS.type; + payload.commitment.quorumHash = GetTestQuorumHash(1); + // TEST_PARAMS.size is 3; an empty bitset models a malformed mined commitment. + payload.commitment.signers = std::vector(); + payload.commitment.validMembers = std::vector(); + + CMutableTransaction mtx; + mtx.nVersion = CTransaction::SPECIAL_VERSION; + mtx.nType = TRANSACTION_QUORUM_COMMITMENT; + SetTxPayload(mtx, payload); + const CTransaction tx{mtx}; + + // Sanity check that the undersized bitsets survive payload round-trip, + // so the call below is genuinely exercising the OOB-prone code path. + const auto opt_round_trip = GetTxPayload(tx); + BOOST_REQUIRE(opt_round_trip.has_value()); + BOOST_CHECK_LT(opt_round_trip->commitment.validMembers.size(), static_cast(TEST_PARAMS.size)); + BOOST_CHECK_LT(opt_round_trip->commitment.signers.size(), static_cast(TEST_PARAMS.size)); + + CBlockIndex base_index; + base_index.nHeight = 0; + + const llmq::UtilParameters util_params{*Assert(m_node.dmnman), + *Assert(m_node.llmq_ctx)->qsnapman, + *Assert(m_node.chainman), + &base_index}; + + TxValidationState state; + BOOST_CHECK(!llmq::CheckLLMQCommitment(util_params, tx, state)); + BOOST_CHECK(state.IsInvalid()); + BOOST_CHECK_EQUAL(state.GetRejectReason(), "bad-qc-height"); + + // Locate the validMembers debug line emitted by CheckLLMQCommitment and + // assert the loop was clamped: with an empty bitset the rendered list must + // be empty. Old code emitted "v[0]=..." here even though the bitset had no + // elements, so checking for the absence of "v[0]" pins down the regression. + const std::string marker = "CFinalCommitment -- CheckLLMQCommitment"; + const auto it = std::find_if(log_lines.begin(), log_lines.end(), + [&marker](const std::string& s) { + return s.find(marker) != std::string::npos; + }); + BOOST_REQUIRE_MESSAGE(it != log_lines.end(), + "CheckLLMQCommitment debug line not captured"); + BOOST_CHECK_MESSAGE(it->find("validMembers[]") != std::string::npos, + "expected clamped 'validMembers[]', got: " + *it); + BOOST_CHECK_MESSAGE(it->find("v[0]") == std::string::npos, + "unexpected v[0] in clamped log line: " + *it); +} + BOOST_AUTO_TEST_CASE(commitment_serialization_test) { // Test with valid commitment