Validate tree deserialisation bounds - #78
Merged
Merged
Conversation
This was referenced Aug 24, 2026
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-tree-bounds-checks
branch
from
August 24, 2026 14:21
27abfbc to
9988966
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-tree-bounds-checks
branch
from
August 24, 2026 14:49
f919e09 to
23ae538
Compare
Amaury Chamayou (achamayou)
marked this pull request as ready for review
August 24, 2026 16:23
Copilot started reviewing on behalf of
Amaury Chamayou (achamayou)
August 24, 2026 16:24
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens tree and path deserialization against malformed counts and truncated hash data.
Changes:
- Adds platform-safe
size_tdeserialization. - Preflights retained-leaf data and uses bounds-checked hash construction.
- Adds malformed tree serialization tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
merklecpp.h |
Strengthens deserialization bounds handling. |
test/unit_tests.cpp |
Tests excessive counts and truncated hashes. |
Suppressed comments (1)
merklecpp.h:1554
- The preflight counts only retained leaf hashes, but deserialization also consumes one extra hash for every set bit in
num_flushed. For example, one retained leaf withnum_flushed == 1and no extra hash passes this check, allocates aNode, and then throws at the extra-hash read. Because_rootis not assigned yet and the vectors contain raw pointers, that node is leaked (including when this constructor throws). Include the extra hashes in the byte preflight before any allocation.
if (
position > bytes.size() ||
num_leaf_nodes > (bytes.size() - position) / HASH_SIZE)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Validate the combined leaf count against Node::size, use width-safe full-tree sizing, and preflight hashes for flushed subtrees. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d55d93ce-932b-421d-9dd7-0d037b06a676
Copilot started reviewing on behalf of
Amaury Chamayou (achamayou)
August 24, 2026 18:50
View session
Eddy Ashton (eddyashton)
approved these changes
Aug 25, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Amaury Chamayou (achamayou)
force-pushed
the
achamayou-tree-bounds-checks
branch
from
August 25, 2026 12:51
deec215 to
05d015e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens
TreeTandPathTdeserialisation against malformed count fields, truncated headers and hash payloads, count wraparound, unrepresentable tree metadata, and undefined shifts while rebuilding large flushed prefixes.Changes
Validate serialised counts against the platform
Serialised indexes and counts are encoded as
uint64_t, while the in-memory APIs usesize_t. The newdeserialise_size_t()helper reads the wire value first and rejects it if it cannot be represented by the current platform. Anif constexprremoves the comparison entirely whensize_tis at least as wide asuint64_t.This check now covers:
This prevents silent narrowing before values are used as loop bounds, vector capacities, indexes, or arithmetic operands on platforms where
size_tis narrower thanuint64_t.Reject invalid or unrepresentable tree sizes before allocation
A serialised tree with flushed leaves but no retained leaf cannot be reconstructed and is rejected. The deserialiser also validates the combined flushed and retained leaf count before reserving or allocating nodes. A binary tree containing
Nleaves has2 * N - 1nodes, andNode::sizestores that node count in asize_t.The maximum accepted leaf count is therefore:
The subtraction-based check avoids overflowing while combining the two serialised counts. It prevents:
num_flushed + num_leaf_nodeswrapping innum_leaves()Node::sizeoverflowing during reconstructionPreflight the complete hash payload
The serialized tree contains one hash for each retained leaf plus one reconstructed left-edge hash for every set bit in
num_flushed. The deserialiser now counts both groups before allocating any nodes:The required count is compared with the remaining byte length using division, avoiding multiplication overflow. Truncated retained-leaf or flushed-subtree data is rejected before reconstruction begins, which also avoids leaking partially allocated raw nodes when a later hash read throws.
Use bounds-checked retained-leaf parsing
Retained hashes are now constructed with
Hash(bytes, position)rather than reading frombytes.data() + position. This centralises the byte-range check and advances the cursor only through the existing hash deserialisation path.A
Node::make()rvalue overload accepts the validated temporary hash directly while preservingNode's aggregate semantics.Make full-subtree sizing width-safe
Reconstruction previously used:
The literal
1is a 32-bitinton supported platforms, so a sufficiently large flushed count could shift it by 32 or more bits, causing undefined behavior.Node::full_size()now performs the calculation withsize_t{1}and handlesheight == std::numeric_limits<size_t>::digitsexplicitly by returningSIZE_MAX. The same helper is reused byNode::is_full()so both paths use identical boundary handling.Regression coverage
The added tests cover:
size_tnum_flushed == SIZE_MAX2 * N - 1node count cannot fit insize_tintshift-width boundaryTesting