Skip to content

Validate tree deserialisation bounds - #78

Merged
Amaury Chamayou (achamayou) merged 4 commits into
mainfrom
achamayou-tree-bounds-checks
Aug 25, 2026
Merged

Validate tree deserialisation bounds#78
Amaury Chamayou (achamayou) merged 4 commits into
mainfrom
achamayou-tree-bounds-checks

Conversation

@achamayou

@achamayou Amaury Chamayou (achamayou) commented Aug 24, 2026

Copy link
Copy Markdown
Member

Summary

Hardens TreeT and PathT deserialisation 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 use size_t. The new deserialise_size_t() helper reads the wire value first and rejects it if it cannot be represented by the current platform. An if constexpr removes the comparison entirely when size_t is at least as wide as uint64_t.

This check now covers:

  • path leaf indexes
  • path maximum indexes
  • path element counts
  • retained tree leaf counts
  • flushed tree leaf counts

This prevents silent narrowing before values are used as loop bounds, vector capacities, indexes, or arithmetic operands on platforms where size_t is narrower than uint64_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 N leaves has 2 * N - 1 nodes, and Node::size stores that node count in a size_t.

The maximum accepted leaf count is therefore:

std::numeric_limits<size_t>::max() / 2 + 1

The subtraction-based check avoids overflowing while combining the two serialised counts. It prevents:

  • num_flushed + num_leaf_nodes wrapping in num_leaves()
  • Node::size overflowing during reconstruction
  • malformed input producing a tree height unsupported by the in-memory representation

Preflight 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:

required hashes = retained leaves + popcount(num_flushed)

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 from bytes.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 preserving Node's aggregate semantics.

Make full-subtree sizing width-safe

Reconstruction previously used:

(1 << height) - 1

The literal 1 is a 32-bit int on 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 with size_t{1} and handles height == std::numeric_limits<size_t>::digits explicitly by returning SIZE_MAX. The same helper is reused by Node::is_full() so both paths use identical boundary handling.

Regression coverage

The added tests cover:

  • every truncation point in the two-field serialized header
  • a serialised count that exceeds the platform's size_t
  • fewer retained hashes than the declared leaf count
  • flushed leaves without any retained leaf
  • a retained leaf combined with num_flushed == SIZE_MAX
  • a combined leaf count whose 2 * N - 1 node count cannot fit in size_t
  • a missing flushed-subtree hash
  • successful reconstruction above the signed-int shift-width boundary

Testing

  • full Debug build with repository clang-tidy checks enabled
  • all 19 unit and serialization test executables
  • full pull-request CI matrix and CodeQL

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@achamayou
Amaury Chamayou (achamayou) marked this pull request as ready for review August 24, 2026 16:23
@achamayou
Amaury Chamayou (achamayou) requested a review from a team as a code owner August 24, 2026 16:23
Copilot AI balanced review requested due to automatic review settings August 24, 2026 16:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Hardens tree and path deserialization against malformed counts and truncated hash data.

Changes:

  • Adds platform-safe size_t deserialization.
  • 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 with num_flushed == 1 and no extra hash passes this check, allocates a Node, and then throws at the extra-hash read. Because _root is 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.

Comment thread merklecpp.h Outdated
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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comment thread merklecpp.h
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@achamayou
Amaury Chamayou (achamayou) merged commit 6258e3c into main Aug 25, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants