Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Makefile.test.include
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ BITCOIN_TESTS =\
test/script_tests.cpp \
test/scriptnum_tests.cpp \
test/serfloat_tests.cpp \
test/serialize_bitset_tests.cpp \
test/serialize_tests.cpp \
test/settings_tests.cpp \
test/sighash_tests.cpp \
Expand Down
23 changes: 21 additions & 2 deletions src/serialize.h
Original file line number Diff line number Diff line change
Expand Up @@ -435,12 +435,31 @@ void WriteFixedBitSet(Stream& s, const std::vector<bool>& vec, size_t size)
s.write(AsBytes(Span{vBytes}));
}

template<typename Stream>
/** A stream that can report how many bytes are still available to read.
*
* size() must mean bytes *remaining*, not the total the stream ever held. ReadFixedBitSet
* relies on that to bound a wire-declared bit count before allocating, so a stream whose
* size() means anything else would silently weaken the bound rather than fail to compile.
*/
template<typename S>
concept SizedStream = requires(const S& s) { { s.size() } -> std::convertible_to<size_t>; };

template<SizedStream Stream>
void ReadFixedBitSet(Stream& s, std::vector<bool>& vec, size_t size)
{
const size_t nbytes = (size + 7) / 8;
// Bound the wire-declared length against the bytes actually left in the stream before
// allocating anything. Otherwise a handful of bytes declaring millions of bits forces a
// multi-megabyte resize and zero-fill that is only abandoned when the short read throws.
// A well-formed message always carries exactly the required bytes, so this rejects only
// claims that could never have been satisfied.
if (nbytes > s.size()) {
throw std::ios_base::failure("ReadFixedBitSet(): declared size exceeds remaining bytes");
}

vec.resize(size);

std::vector<uint8_t> vBytes((size + 7) / 8);
std::vector<uint8_t> vBytes(nbytes);
s.read(AsWritableBytes(Span{vBytes}));
for (size_t p = 0; p < size; p++)
vec[p] = (vBytes[p / 8] & (1 << (p % 8))) != 0;
Expand Down
103 changes: 103 additions & 0 deletions src/test/serialize_bitset_tests.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
// Copyright (c) 2026 The Dash Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <llmq/commitment.h>
#include <llmq/params.h>
#include <serialize.h>
#include <streams.h>
#include <test/util/setup_common.h>
#include <uint256.h>

#include <boost/test/unit_test.hpp>

#include <ios>
#include <string>
#include <vector>

BOOST_FIXTURE_TEST_SUITE(serialize_bitset_tests, BasicTestingSetup)

namespace {
//! The bound must reject before allocating, so a short read after the fact is not good enough.
bool RejectedBeforeAllocating(const std::string& what)
{
return what.find("exceeds remaining") != std::string::npos ||
what.find("ReadFixedBitSet") != std::string::npos;
}
Comment on lines +21 to +26

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make the regression assert the guarded failure path.

RejectedBeforeAllocating accepts any exception containing ReadFixedBitSet. A later short-read path can contain that function name and still pass. BOOST_CHECK_NE also accepts any partial resize except the exact claimed size. Match the guard-specific error and compare each destination with its size before deserialization.

Proposed test assertion fix
-    return what.find("exceeds remaining") != std::string::npos ||
-           what.find("ReadFixedBitSet") != std::string::npos;
+    return what.find("declared size exceeds remaining bytes") != std::string::npos;
...
+    const auto initial_bits_size = bits.size();
...
-    BOOST_CHECK_NE(bits.size(), kClaimedBits);
+    BOOST_CHECK_EQUAL(bits.size(), initial_bits_size);
...
+    const auto initial_signers_size = qc.signers.size();
...
-    BOOST_CHECK_NE(qc.signers.size(), 1'000'000u);
+    BOOST_CHECK_EQUAL(qc.signers.size(), initial_signers_size);

Also applies to: 44-55, 88-100

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/serialize_bitset_tests.cpp` around lines 21 - 26, Strengthen
RejectedBeforeAllocating to accept only the guard-specific “exceeds remaining”
error, not the generic ReadFixedBitSet function name. In the affected regression
assertions, compare each destination’s size exactly against its
pre-deserialization size using equality checks, rather than merely asserting it
differs from the claimed size.

Comment on lines +21 to +26

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Test assertions don't tightly prove rejection happens before mutation

RejectedBeforeAllocating() matches on either "exceeds remaining" or the bare substring "ReadFixedBitSet", the latter of which would also match any future exception message from within that function (e.g. a reworded variant of the existing "Out-of-range bits set" check, if it were ever changed to include the function name) even if the new pre-allocation guard didn't fire. Separately, BOOST_CHECK_NE(bits.size(), kClaimedBits) and BOOST_CHECK_NE(qc.signers.size(), 1'000'000u) only prove the destination wasn't resized to the exact attacker-claimed value — they don't prove it wasn't mutated to some other size, which is the actual invariant the PR is trying to assert (rejection before destination mutation/allocation). Matching only the guard-specific message text and comparing against the pre-deserialization size with BOOST_CHECK_EQUAL would make this a tight regression test for the fix rather than one that happens to pass today. This was raised by CodeRabbit on this same head and remains open.

Suggested change
//! The bound must reject before allocating, so a short read after the fact is not good enough.
bool RejectedBeforeAllocating(const std::string& what)
{
return what.find("exceeds remaining") != std::string::npos ||
what.find("ReadFixedBitSet") != std::string::npos;
}
bool RejectedBeforeAllocating(const std::string& what)
{
return what.find("declared size exceeds remaining bytes") != std::string::npos;
}

source: ['claude', 'codex']

} // namespace

/**
* DYNBITSET must not allocate from an attacker-declared CompactSize when the remaining stream
* is far too small to hold the claimed bit payload. A handful of bytes claiming ~1e6 bits is
* the amplification primitive: ReadCompactSize permits up to 33,554,432, which would resize a
* std::vector<bool> to ~4 MiB and allocate another ~4 MiB byte buffer before the short read
* throws. The claim below is deliberately modest so the pre-fix path also stays safe on CI.
*/
BOOST_AUTO_TEST_CASE(dynbitset_rejects_oversized_declared_length)
{
constexpr uint64_t kClaimedBits = 1'000'000;

CDataStream s(SER_NETWORK, PROTOCOL_VERSION);
WriteCompactSize(s, kClaimedBits);
// No bit payload follows, so the remaining size is zero.

std::vector<bool> bits;
std::string what;
bool threw = false;
try {
s >> DYNBITSET(bits);
} catch (const std::ios_base::failure& e) {
threw = true;
what = e.what();
}
BOOST_CHECK_MESSAGE(threw, "DYNBITSET must reject a declared length that exceeds remaining bytes");
BOOST_CHECK_NE(bits.size(), kClaimedBits);
BOOST_CHECK_MESSAGE(RejectedBeforeAllocating(what), "Expected a pre-allocation rejection, got: " + what);
}

/** A legitimately sized DYNBITSET (LLMQ max 400) must still round-trip unchanged. */
BOOST_AUTO_TEST_CASE(dynbitset_accepts_legitimate_llmq_size)
{
constexpr size_t kSize = Consensus::MAX_LLMQ_SIZE;
std::vector<bool> original(kSize, false);
for (size_t i = 0; i < kSize; i += 3) {
original[i] = true;
}

CDataStream s(SER_NETWORK, PROTOCOL_VERSION);
s << DYNBITSET(original);

std::vector<bool> decoded;
s >> DYNBITSET(decoded);
BOOST_CHECK(decoded == original);
}

/**
* The same primitive is reachable from an unauthenticated QFCOMMITMENT via CFinalCommitment's
* signers bitset, so cover the real message type too.
*/
BOOST_AUTO_TEST_CASE(qfinalcommitment_rejects_oversized_signers_bitset)
{
CDataStream s(SER_NETWORK, PROTOCOL_VERSION);
// nVersion (u16) | llmqType (u8) | quorumHash (32) | signers DYNBITSET | ...
s << static_cast<uint16_t>(llmq::CFinalCommitment::BASIC_BLS_NON_INDEXED_QUORUM_VERSION);
s << Consensus::LLMQType::LLMQ_400_85;
s << uint256::ONE;
WriteCompactSize(s, 1'000'000);

llmq::CFinalCommitment qc;
std::string what;
bool threw = false;
try {
s >> qc;
} catch (const std::ios_base::failure& e) {
threw = true;
what = e.what();
}
BOOST_CHECK(threw);
BOOST_CHECK_NE(qc.signers.size(), 1'000'000u);
BOOST_CHECK_MESSAGE(RejectedBeforeAllocating(what),
"Expected a pre-allocation rejection for CFinalCommitment, got: " + what);
}

BOOST_AUTO_TEST_SUITE_END()
Loading