refactor: add bounded vector deserialization#7439
Conversation
thepastaclaw
changed the title
thepastaclaw
force-pushed
the
serialize/limited-vector
branch
from
177b53f to
bf865ac
Compare
|
Addressed in bf865ac. I ported the substance of f80422c and 6b47508 onto this branch: restored the normal CompactSize range check, documented the unrelated serialization-exception behavior, and updated the focused boundary tests. The changes are squashed into the existing single PR commit as requested; unrelated commits from pr_7416 were not brought over. Validation: |
Introduce a runtime and a compile-time bounded reader for CompactSize-prefixed vectors so downstream call sites can reject grossly oversized wire counts before any element decode or allocation. Both primitives preserve the ordinary std::vector wire format — the limit is a deserialization safety property. - Factor the batched vector element-decode loop out of VectorFormatter::Unser into detail::UnserializeVectorContents<Formatter>, and reuse it from both new primitives so the DoS-resistant 5 MiB batching is not duplicated. - UnserializeVectorWithMaxSize<Formatter>(stream, vec, max_size) reads the CompactSize prefix and rejects a count above the caller's max_size before any element decode, returning false without consuming an element. A count above the generic MAX_SIZE cap throws from ReadCompactSize as malformed; since a realistic max_size is far below MAX_SIZE (you reach MAX_PROTOCOL_MESSAGE_LENGTH long before MAX_SIZE), the caller's own limit is the gate that fires in practice. The helper may otherwise throw on an unrelated serialization failure. - LimitedVectorFormatter<Limit, ElemFormatter=DefaultFormatter> is the READWRITE-friendly wrapper; a LIMITED_VECTOR(obj, n) macro mirrors the existing LIMITED_STRING convention. Ser delegates to VectorFormatter so the write side stays unbounded and interoperable with the standard reader. Cover the primitives with focused unit tests: within/exact/over/zero-limit boundaries, wire-format identity vs the ordinary vector encoding, that Ser remains unbounded, and — via a CountingFormatter that tallies Unser calls — that a MAX_SIZE wire count is rejected before any element decode occurs, while counts above MAX_SIZE throw at the CompactSize level. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
serialize/limited-vector
branch
from
bf865ac to
af6a0fd
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThe serialization framework now shares batched vector allocation and element decoding through a helper. It adds runtime maximum-size decoding and a compile-time Sequence Diagram(s)sequenceDiagram
participant Caller
participant LimitedVectorFormatter
participant Stream
participant UnserializeVectorContents
participant ElemFormatter
Caller->>LimitedVectorFormatter: unserialize bounded vector
LimitedVectorFormatter->>Stream: read CompactSize element count
Stream-->>LimitedVectorFormatter: element count
alt count <= Limit
LimitedVectorFormatter->>UnserializeVectorContents: decode vector elements
UnserializeVectorContents->>ElemFormatter: unserialize each element
ElemFormatter-->>UnserializeVectorContents: decoded element
UnserializeVectorContents-->>LimitedVectorFormatter: populated vector
else count > Limit
LimitedVectorFormatter-->>Caller: throw length-limit failure
end
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
✅ Review complete (commit af6a0fd) |
There was a problem hiding this comment.
Code Review
Clean, well-scoped serialization refactor adding bounded-vector deserialization primitives with byte-identical wire format and thorough unit coverage. No blocking issues: the count is validated before any allocation, the Ser side stays unbounded for wire compatibility, and CompactSize range handling is correct. One legitimate but practically-inert nitpick about formatter construction-order reordering in the refactor.
💬 1 nitpick(s)
Source: reviewers opus (general, dash-core-commit-history) and gpt-5.5 (general, dash-core-commit-history); verifier opus.
| formatter.Unser(s, v.back()); | ||
| } | ||
| } | ||
| detail::UnserializeVectorContents<Formatter>(s, v, ReadCompactSize(s)); |
There was a problem hiding this comment.
💬 Nitpick: Refactor reorders Formatter construction relative to clear/count-read
The pre-refactor VectorFormatter::Unser default-constructed Formatter formatter; before calling v.clear() and ReadCompactSize(s). The shared helper now constructs the Formatter inside detail::UnserializeVectorContents, after the vector is cleared and the count has been consumed from the stream. For a Formatter whose default constructor has side effects or can throw, a construction failure would now leave the stream advanced past the count and the destination vector cleared — an observable difference in a change advertised as behavior-preserving. This is inert for every formatter in the tree today (all are stateless empty structs with trivial default constructors), so it is only worth noting for the refactor's stated invariant, not a defect. If you want to preserve the exact original ordering, construct the Formatter in VectorFormatter::Unser and pass it into the shared helper by reference.
source: ['codex']
3cb73fd test: drop redundant spork_tests unit suite (PastaClaw) 44a8fe1 test: regression coverage for CSporkMessage signature-size bound (PastaClaw) d2e600f fix: bound CSporkMessage signature vector allocation (PastaClaw) Pull request description: Depends on #7439. Please review only the two SPORK-specific commits here. ## What changed - bound `CSporkMessage::vchSig` to `CPubKey::COMPACT_SIGNATURE_SIZE` before vector allocation using #7439s `LIMITED_VECTOR` formatter - punish peers that send malformed, truncated, or oversized SPORK messages - add unit coverage for the exact `CompactSize(MAX_SIZE)` trigger and a P2P disconnect regression test ## Why A malformed SPORK message could declare a large signature length in a small payload. Generic byte-vector deserialization would allocate its first 5,000,000-byte chunk before discovering the payload was truncated. The resulting exception was caught by the generic message-processing handler without penalizing the peer, allowing repeated attempts on the same connection. Valid SPORK signatures are 65-byte compact signatures on every network. The testnet distinction changes the signing preimage, not the signature encoding. Shorter well-formed signatures continue through the existing invalid-signature path and are punished there; the formatter prevents oversized declarations from allocating first. The reusable bounded-vector serialization primitives are introduced separately in #7439. ## Validation - `src/test/test_dash --run_test=spork_tests,serialize_tests` - `test/functional/feature_sporks.py` The exact unit and functional regressions were also run against the vulnerable implementation: the unit test observed the old EOF-after-allocation path, and the malformed P2P peer was not disconnected. ACKs for top commit: PastaPastaPasta: utACK 3cb73fd kwvg: utACK 3cb73fd Tree-SHA512: d547f589c3920baaacb121c618b812314901639c449bd562ea150de3a8743e3fbff10c1c92f2e192de776452d74ad7dc9be02622ddd29d9e5f7e43eacd966fd2
a286e0e fix: bound quorum data response vectors (PastaClaw) Pull request description: Depends on #7439. Please review only the final command-specific commit here. ## Issue being fixed or feature implemented Quorum-data responses carry verification-vector and encrypted-contribution vectors whose expected sizes are known from the requested quorum. This hardens QDATA processing by checking the serialized vector count before allocating, deserializing, or processing those vectors. This path is MNAuth/request-gated and is intentionally handled separately from unauthenticated public vector intake. ## What was done? - Read QDATA verification vectors with the requested quorum threshold as the maximum. - Read encrypted contributions with the number of valid quorum members as the maximum. - Require exact semantic counts and penalize mismatches before decrypt/aggregate work. - Extend functional coverage for undersized, oversized, and allocation-amplifying CompactSize declarations. The reusable bounded-vector serialization primitives are introduced separately in #7439. ## How Has This Been Tested? - `test/functional/p2p_quorum_data.py` ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only) ACKs for top commit: kwvg: Code near identical to past utACK (see [diff](https://github.com/dashpay/dash/compare/ecf49885bcbb40fd09bd81de95edf54d3996e2fe..a286e0e7eb4fd63eb65f05f5deefd5a2c4c43f55)) --- utACK a286e0e Tree-SHA512: 1d217cff7096f0dbad736e98be91e6b4eac154ae7ca0512cb3200f448b3152485d89f3f13a5d36617d7e4122aac2ed521f79ccd6ca787c0e8fafb200cb3cf118
8f0b813 fix(governance): bound vote signature deserialization (PastaClaw) Pull request description: Uses the shared bounded-vector deserialization primitive merged in #7439. ## Motivation Governance vote signatures were deserialized through the generic byte-vector path. A peer could declare a very large signature length, causing allocation before the stream reported truncation. The outer message-processing catch did not score or disconnect the peer, allowing repeated malformed messages. ## Changes - bound network governance-vote signature reads to 96 bytes before allocation - require one of the two structurally valid encodings: 65-byte compact ECDSA or 96-byte BLS - score malformed or truncated governance vote messages with 100 misbehavior points - preserve disk, hash, and outbound serialization behavior - add focused unit coverage ## Testing - `./src/test/test_dash --run_test=governance_vote_wire_tests` (4/4 tests) - `./src/test/test_dash --run_test=serialize_tests` (10/10 tests) - `test/lint/lint-python.py` - `git diff --check upstream/develop...HEAD` ACKs for top commit: knst: utACK 8f0b813 Tree-SHA512: 5cd804ffb410f47936615d230f5a5ebe4b2b4e1dd85455acb42df37fb65c0397f05c38704900a3ecf5d2729e157d03b298c4a7774b7764b3830cb57273a724cc
5 participants
Issue being fixed or feature implemented
Network messages often have protocol-specific vector limits below the generic serialization limit. Callers need to enforce those limits before vector allocation and element decoding, without changing the wire format.
What was done?
UnserializeVectorWithMaxSizefor runtime bounds.LIMITED_VECTOR/LimitedVectorFormatterfor compile-time bounds inREADWRITEdeclarations.MAX_SIZE.Stacked adopters
Each consumer remains a separate command-specific PR:
Reviewing #7439 first leaves each child PR with only its protocol-specific policy, punishment, and regression tests.
How Has This Been Tested?
src/test/test_dash --run_test=serialize_testsMAX_SIZEandMAX_SIZE + 1declarations, 64-bit CompactSize counts, wire compatibility, and rejection before element decode are covered.Breaking Changes
None. Existing vector serialization and deserialization behavior is unchanged.
Checklist: