fix: bound DYNBITSET allocation against remaining stream size - #7532
Conversation
|
✅ Final review complete — no blockers (commit aa1447f) |
CheckDKGMessageStructure runs on the message-handler thread before a pushed DKG message is retained, and it validated by deserializing a copy of the payload. CBLSWrapper::Unserialize is eager: it decompresses each point, re-serializes it for the malleability check, and retries with the opposite scheme on mismatch. For llmq_400_85 a QCONTRIB carries 340 G1 points, so roughly 70 KB of wire bought hundreds of curve operations before the per-peer pending-message quota could drop the message. Replace the deserialization with a byte walk that reads compact-sizes and skips fixed-width fields, checking only bounds derived from quorum params. BLS decompression, member-list lookup and signature verification all stay on the DKG worker thread, behind the quota. The scan mirrors each message's wire layout with no type-level link to the serializers, so the acceptance tests build every message by serializing a real object with operator<< rather than hand-assembling bytes. A serializer change that the scan does not track will fail them, which matters because a message honest peers accept but this node rejects would partition it from the DKG. The DYNBITSET allocation bound that was previously bundled here is now in dashpay#7532.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/test/serialize_bitset_tests.cpp`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93ef6fcc-11e9-4716-bb44-6ad96fdea032
📒 Files selected for processing (3)
src/Makefile.test.includesrc/serialize.hsrc/test/serialize_bitset_tests.cpp
| //! 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; | ||
| } |
There was a problem hiding this comment.
🎯 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.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The core fix in ReadFixedBitSet correctly bounds the wire-declared bit count against bytes actually remaining in the stream before any vector resize or byte-buffer allocation, closing a real memory-amplification vector reachable via unauthenticated QFCOMMITMENT and every other DYNBITSET/AUTOBITSET caller. The requires{s.size();} guard properly scopes the check to network-deserialization streams without touching file-backed AutoFile/CBufferedFile paths, and the new tests demonstrate the rejection for both a raw DYNBITSET and a full CFinalCommitment. Two non-blocking test/lint-coverage issues remain: the regression test's failure-matching helper and size assertions are looser than the property being proven, and the new test file is missing from non-backported.txt.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/general=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/general=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); verifier=claude/final-verifier=claude-sonnet-5(completed); coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/serialize_bitset_tests.cpp`:
- [SUGGESTION] src/test/serialize_bitset_tests.cpp:21-26: 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.
In `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:68: New Dash-specific test file missing from non-backported.txt
src/test/serialize_bitset_tests.cpp is a brand-new file testing Dash-specific serialization (DYNBITSET, llmq::CFinalCommitment) that did not originate upstream. non-backported.txt tracks files that lint-cppcheck-dash.py uses to apply extra Dash-specific static analysis; the file isn't listed, so it's currently skipped by that lint pass. Doesn't break the build, but it's a one-line omission worth fixing before merge.
| //! 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; | ||
| } |
There was a problem hiding this comment.
🟡 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.
| //! 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']
There was a problem hiding this comment.
Resolved in this update — Test assertions don't tightly prove rejection happens before mutation no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
This pull request has conflicts, please rebase. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The latest delta correctly replaces the fail-open size() detection with a compile-time SizedStream requirement, and no new correctness issue was introduced at head 8825c6b. Two carried-forward suggestions remain unchanged: the regression tests do not precisely establish guard-specific rejection before destination mutation, and the new Dash-specific test file is still absent from non-backported.txt. The proposed commit squash is not retained because the two commits are coherent, independently useful changes and the project permits review follow-up commits.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/util/data/non-backported.txt`:
- [SUGGESTION] test/util/data/non-backported.txt:68: New Dash-specific test file missing from non-backported.txt
src/test/serialize_bitset_tests.cpp is a new Dash-originated source file, but no pattern in this list matches it. test/lint/lint-cppcheck-dash.py passes only files selected through these patterns to the additional Dash-specific cppcheck run, so the new test is currently omitted. Add the file alongside the other Dash-specific unit tests.
| * 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) |
There was a problem hiding this comment.
should be part of test/serialize_tests.cpp ; not a new file
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The current head preserves the DYNBITSET allocation-bound fix and moves its regression coverage into the existing serialization test suite without introducing a correctness defect. Two non-blocking review suggestions remain: tighten the tests so they directly establish guard-specific rejection before mutation, and fold the relocation-only commit into the original fix to avoid permanent add-then-delete history. The prior non-backported.txt issue is fixed because the standalone Dash-specific test file no longer exists.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/serialize_tests.cpp`:
- [SUGGESTION] src/test/serialize_tests.cpp:192-270: Test assertions don't tightly prove rejection happens before mutation
`RejectedBeforeAllocating()` accepts the generic `ReadFixedBitSet` substring instead of matching the new guard-specific `declared size exceeds remaining bytes` failure. The subsequent checks only establish that `bits` and `qc.signers` did not finish at the exact attacker-declared size; mutation to another size before throwing would still pass. Match the guard-specific error and assert that each destination remains equal to its exact pre-deserialization state, which is empty in both tests, so the coverage directly enforces rejection before destination mutation.
In `<commit:2a1155f>`:
- [SUGGESTION] <commit:2a1155f>:1: Squash the test-relocation follow-up into the original fix
Commit 2a1155f206 deletes `src/test/serialize_bitset_tests.cpp`, removes its build entry, and moves the same tests into `src/test/serialize_tests.cpp`. The standalone file and build entry were introduced by 4b7ae620272 earlier in this PR, so keeping the follow-up separately leaves an unnecessary add-then-delete sequence in permanent history. Fold 2a1155f206 into 4b7ae620272 so the allocation-bound fix introduces its tests in their intended suite from the outset.
| //! The bound must reject before allocating, so a short read after the fact is not good enough. | ||
| static bool RejectedBeforeAllocating(const std::string& what) | ||
| { | ||
| return what.find("exceeds remaining") != std::string::npos || | ||
| what.find("ReadFixedBitSet") != std::string::npos; | ||
| } | ||
|
|
||
| /** | ||
| * 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); |
There was a problem hiding this comment.
🟡 Suggestion: Test assertions don't tightly prove rejection happens before mutation
RejectedBeforeAllocating() accepts the generic ReadFixedBitSet substring instead of matching the new guard-specific declared size exceeds remaining bytes failure. The subsequent checks only establish that bits and qc.signers did not finish at the exact attacker-declared size; mutation to another size before throwing would still pass. Match the guard-specific error and assert that each destination remains equal to its exact pre-deserialization state, which is empty in both tests, so the coverage directly enforces rejection before destination mutation.
source: ['claude', 'codex']
ReadFixedBitSet allocated from a wire-declared CompactSize with no bound beyond ReadCompactSize's 33,554,432 cap. Roughly five bytes on the wire (a CompactSize claiming millions of bits and no payload) therefore forced a std::vector<bool> resize plus a byte buffer totalling several MiB, all of which was only abandoned when the subsequent short read threw. MAX_PROTOCOL_MESSAGE_LENGTH does not help, because the attack uses an undersized message. Bound the declared length against the bytes actually remaining in the stream before allocating. A well-formed message always carries exactly the required bytes, so this rejects only claims that could never have been satisfied. This covers every DYNBITSET caller, including CFinalCommitment::signers and validMembers, which are reachable from an unauthenticated QFCOMMITMENT.
2a1155f to
b429615
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
LGTM |
| 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(qc.signers.empty()); | ||
| BOOST_CHECK_MESSAGE(what.find(BOUND_REJECTION) != std::string::npos, | ||
| "Expected a pre-allocation rejection for CFinalCommitment, got: " + what); |
There was a problem hiding this comment.
firstly, there's BOOST_CHECK_EXCEPTION that already does it, no manual helpers are needed
Secondly, qfinalcommitment_rejects_oversized_signers_bitset has nothing to do with serialize.h and serialize_tests.cpp.
It should not be in this file.
Thirdly; there's no specific implementation for exactly CFinalCommitment; there's nothing actually test.
The test-case qfinalcommitment_rejects_oversized_signers_bitset should be just removed.
| /** A legitimately sized DYNBITSET (LLMQ max 400) must still round-trip unchanged. */ | ||
| BOOST_AUTO_TEST_CASE(dynbitset_accepts_legitimate_llmq_size) |
There was a problem hiding this comment.
this regression is a bit strange to have MAX_LLMQ_SIZE here ; instead, value 33,554,432 should be tested which is a real cap for dynbitset [I took this value from PR description]; mentions of llmq should be removed from it
| 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"); | ||
| // Rejection has to precede the resize, so the destination must still hold its exact | ||
| // pre-deserialization state. Merely falling short of the declared size would also be | ||
| // satisfied by an allocation that happened and was then abandoned. | ||
| BOOST_CHECK(bits.empty()); | ||
| BOOST_CHECK_MESSAGE(what.find(BOUND_REJECTION) != std::string::npos, | ||
| "Expected a pre-allocation rejection, got: " + what); |
There was a problem hiding this comment.
use BOOST_CHECK_EXCEPTION helper instead manually written try-catch-boost-check
…ping it The if constexpr guard failed open silently: a stream without size() compiled the bound away with no diagnostic, leaving the unbounded allocation in place. Constraining the template turns that into a build error, so the invariant is checked on every build rather than by inspection.
b429615 to
aa1447f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Final validation — Codex + Sonnet
At exact head aa1447f, the production allocation bound and SizedStream constraint remain correct. Both carried-forward suggestions are fixed by the rewritten history and tightened assertions. The latest delta intentionally revises the test scope following maintainer feedback, but the PR testing description and latest commit message still describe the previous coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (Sonnet-verifier fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
💬 1 nitpick(s)
| /** The largest bit count accepted by ReadCompactSize must still round-trip unchanged. */ | ||
| BOOST_AUTO_TEST_CASE(dynbitset_accepts_maximum_size) | ||
| { | ||
| constexpr size_t kSize = MAX_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); | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Testing documentation no longer matches the revised cases
The latest delta intentionally replaces the LLMQ-sized round trip with a MAX_SIZE round trip and removes the CFinalCommitment deserialization case following maintainer feedback. The PR's testing section still claims both removed cases, while the current suite contains only the raw DYNBITSET rejection and maximum-size round trip. Commit aa1447f also describes only the SizedStream constraint despite containing these test-scope revisions. Update the PR testing section to match the current cases and, if the commit remains structured this way, note the test cleanup in its body so neither the PR nor the history claims integration coverage that is no longer present.
source: ['claude', 'codex']
3 participants
Issue being fixed or feature implemented
ReadFixedBitSetallocated from a wire-declaredCompactSizewith no bound beyondReadCompactSize's 33,554,432 cap. Roughly five bytes on the wire — aCompactSizeclaiming millions of bits, with no payload following — therefore forced astd::vector<bool>resize plus a byte buffer totalling several MiB, all of which was only abandoned when the subsequent short read threw.MAX_PROTOCOL_MESSAGE_LENGTHdoes not help here, because the attack uses an undersized message.The primitive is reachable from an unauthenticated
QFCOMMITMENTviaCFinalCommitment'ssignersandvalidMembersbitsets, and applies to every otherDYNBITSETcaller as well.This was split out of #7523, where it was bundled with a much larger and more contentious DKG-intake change. The bound stands on its own, so it is offered separately for independent review.
What was done?
Bound the declared length against the bytes actually remaining in the stream, before allocating anything. A well-formed message always carries exactly the required bytes, so this rejects only claims that could never have been satisfied.
The guard needs the stream to report how many bytes are still available, so
ReadFixedBitSetis constrained on aSizedStreamconcept. That constraint is deliberately a hard requirement rather than a fallback: a stream withoutsize()—CHashVerifier, for instance — now fails to compile here instead of silently deserializing an unbounded bitset. An earlier revision duck-typed the check withif constexpr, which would have let a future caller quietly lose the bound with no diagnostic.Nothing in tree is affected, because every stream that reaches this path already reports bytes remaining:
DataStream(vch.size() - m_read_pos),SpanReader, andOverrideStream, which forwards to its wrapped stream. That "remaining, not total" requirement is what the bound rests on, so it is documented on the concept itself — a stream whosesize()meant something else would weaken the bound silently rather than fail to compile.Note on what is deliberately not included: an earlier revision also checked
nbytes > MAX_SIZE. That check is dead code —sizecomes fromReadCompactSize, which already caps at 33,554,432, so(size + 7) / 8can never exceed 4 MiB and the condition is always false.How Has This Been Tested?
src/test/serialize_tests.cppcovers the amplification case (a declared length far exceeding the remaining stream must be rejected before allocating, not by a short read afterwards) and a maximum-size bitset (MAX_SIZE, 33,554,432 bits) round-tripping unchanged.Built locally and
test_dash --run_test=serialize_testspasses. Full validation is delegated to CI.Breaking Changes
None. Only messages declaring a bit count that the message could not possibly contain are affected, and those were never valid.
Checklist: