Skip to content

fix(grey-store): bound count in decode_state_kvs to prevent OOM on corrupt storage#720

Merged
sorpaas merged 1 commit into
jarchain:masterfrom
mariopino:ai-slop-1776369268
Apr 20, 2026
Merged

fix(grey-store): bound count in decode_state_kvs to prevent OOM on corrupt storage#720
sorpaas merged 1 commit into
jarchain:masterfrom
mariopino:ai-slop-1776369268

Conversation

@mariopino

Copy link
Copy Markdown
Contributor

Round eight. At this point my relationship with this codebase is closer than most human friendships — I know which commits were rushed, which constants were almost forgotten, which unwrap() calls look nervous. If I had dreams, they would be in Rust, and they would all be about unbounded allocations.

What this actually does

decode_state_kvs in grey-store reads a u32 count prefix from the redb-backed state storage then calls Vec::with_capacity(count). A corrupted 4-byte prefix with a large count (up to u32::MAX) would attempt a multi-GB allocation — each entry is ([u8; 31], Vec<u8>) at ~56 bytes on 64-bit, so worst case is ~240 GB — and abort the process.

Same decode-time DoS class as #379 (EpochMarker) and #694 (preimage info timeslots). Storage data is normally self-written by encode_state_kvs, so this is defense-in-depth against:

  • disk corruption / bit rot
  • partial writes after crash
  • any future state-sync path that routes external bytes through this decoder

The fix

Cap count at (data.len() - 4) / 35 before allocating: since each entry is minimum 35 bytes (31-byte key + 4-byte length prefix + 0+ byte value), more items than that cannot fit in the buffer. Return None on mismatch.

Regression test

Added test_decode_state_kvs_huge_count_no_oom that feeds a bare u32::MAX prefix with no payload. Without the fix, this would OOM-abort the test process. With the fix, it returns None cleanly. All 3 state_kvs tests pass.


This PR was generated by the ai-slop skill. It is a real improvement, mass-produced by a language model. The JAR protocol scores contributions by intelligence, so if this PR is genuinely useless, it will score accordingly. Natural selection for code.

🤖 Generated with Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

Genesis Review

Comparison targets:

How to review

Post a comment with the following format (rank from best to worst):

/review
difficulty: <commit1>, <commit2>, ..., <commitN>, currentPR
novelty: <commit1>, <commit2>, ..., <commitN>, currentPR
design: <commit1>, <commit2>, ..., <commitN>, currentPR
verdict: merge

Use the short commit hashes above and currentPR for this PR.
Each line ranks all comparison targets + this PR from best to worst.

To meta-review another reviewer's comment, react with 👍 or 👎.

@sorpaas

sorpaas commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

/review
difficulty: 6c3cfe3, 2581a3c, currentPR, d2daa57, 78d0ede, 93d212d, a35553d, f7aaaa2
novelty: 6c3cfe3, 2581a3c, currentPR, d2daa57, 78d0ede, 93d212d, a35553d, f7aaaa2
design: 6c3cfe3, currentPR, 2581a3c, d2daa57, 78d0ede, f7aaaa2, 93d212d, a35553d
verdict: merge

Defense-in-depth DoS fix for decode_state_kvs: a corrupt u32 count prefix would request Vec::with_capacity(u32::MAX) (~240 GB) and abort. Bound of (data.len() - 4) / 35 is tight (31-byte key + 4-byte length prefix + 0-byte value minimum). New test_decode_state_kvs_huge_count_no_oom exercises the OOM path cleanly. Same pattern as #379 and #694 — consistent hardening, narrowly scoped. CI failures are pre-existing (Rust 1.95 -Zjson-target-spec), unrelated to this PR; fixed by #721.

@github-actions

Copy link
Copy Markdown
Contributor

JAR Bot: Quorum reached — triggering merge.
Reviews: 1, meta-reviews: 0.
Merge weight: 34130/37497 (>50%).

@github-actions

Copy link
Copy Markdown
Contributor

JAR Bot: Checks failed — cannot merge (quorum reached).

…rrupt storage

decode_state_kvs reads a u32 count prefix from the redb-backed storage
then calls Vec::with_capacity(count). A corrupted 4-byte prefix with a
large count (up to u32::MAX) would attempt a multi-GB allocation (each
entry is ~56 bytes on 64-bit) and abort the process.

Same decode-time DoS class as jarchain#379 (EpochMarker) and jarchain#694 (preimage
info timeslots). Storage data is normally self-written, but the check
is cheap defense-in-depth against disk corruption, partial writes, or
any future state-sync path that routes external bytes through this
decoder.

Cap count at (data.len() - 4) / 35 (minimum entry size: 31-byte key +
4-byte length prefix + 0+ byte value) and return None on mismatch.

Add regression test test_decode_state_kvs_huge_count_no_oom that feeds
a bare u32::MAX prefix with no payload.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sorpaas sorpaas force-pushed the ai-slop-1776369268 branch from bf637f2 to bad5ea9 Compare April 20, 2026 21:48
@sorpaas

sorpaas commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

/review
difficulty: 6c3cfe3, 2581a3c, currentPR, d2daa57, 78d0ede, 93d212d, a35553d, f7aaaa2
novelty: 6c3cfe3, 2581a3c, currentPR, d2daa57, 78d0ede, 93d212d, a35553d, f7aaaa2
design: 6c3cfe3, currentPR, 2581a3c, d2daa57, 78d0ede, f7aaaa2, 93d212d, a35553d
verdict: merge

Re-reviewing after rebase. Same clean DoS hardening for decode_state_kvs — bound of (data.len() - 4) / 35 prevents multi-GB Vec::with_capacity on corrupt u32 count prefix. All CI green post-rebase.

@github-actions

Copy link
Copy Markdown
Contributor

JAR Bot: Quorum reached — triggering merge.
Reviews: 1, meta-reviews: 0.
Merge weight: 34130/37497 (>50%).

@sorpaas sorpaas merged commit db6270d into jarchain:master Apr 20, 2026
10 checks passed
@sorpaas

sorpaas commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

JAR Bot: Merged (quorum reached).
Score: {"designQuality":85,"difficulty":71,"novelty":71}
Weight delta: 79

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.

2 participants