fix(model): bound subtree-list allocation on untrusted block parse + fuzz#1005
Merged
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: No issues found This PR correctly addresses a security vulnerability (unbounded allocation from untrusted input) with an appropriate fix and comprehensive testing: Core Fix (model/Block.go:286) Test Coverage (model/block_fuzz_test.go)
The fix aligns with the existing maxCoinbaseBUMPSize pattern at line 329 and follows AGENTS.md security rules. |
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-06-01 15:37 UTC |
…fuzz
readBlockFromReader pre-sized the subtree slice straight from the block's
subtree-count varint:
block.Subtrees = make([]*chainhash.Hash, 0, block.subtreeLength)
A block from an untrusted peer claiming ~33M subtrees (or more) forces a
multi-hundred-MB-to-GB allocation before the first missing hash fails the
read — a cheap OOM that takes down the core sidecar via the ServiceManager
errgroup. NewBlockFromBytes' recover() does not save an OOM. The
CoinbaseBUMP read just below was already bounded (1 MB); the subtree list
was not.
Cap the speculative capacity (min(subtreeLength, 1024)); the loop already
appends, so a bogus count now errors on the first missing hash instead of
pre-allocating. Valid blocks parse unchanged.
Add fuzz coverage for the block deserializer:
- FuzzNewBlockFromBytes (public entry; guards the OOM/alloc class that
its recover() cannot catch),
- FuzzReadBlockFromReader (inner body parser, no recover wrapper, so
masked panics in varint/subtree/coinbase/bump parsing surface), and
- TestBlock_HostileSubtreeLengthIsBounded, a deterministic regression
asserting the hostile body allocates < 4 MB (was ~256 MB) before
erroring.
Both fuzz targets ran clean (~525K and ~860K execs, no crashers).
Second in the deserializer-fuzzing series after the utxopersister fix.
15e237e to
97b8eea
Compare
Collaborator
Author
|
Good catch on the implicit init — you're right that it's safe because |
|
ordishs
approved these changes
Jun 1, 2026
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
Approve. Minimal, correct fix for a real unbounded-allocation OOM vector on the untrusted block-parse path.
min(subtreeLength, 1024)cap is safe — the append loop validates each hash and errors on the first missing one; valid blocks parse identically.- Confirmed this was the only allocation sized from an untrusted count in
readBlockFromReader(CoinbaseBUMP was already bounded at 1MB). - Verified locally: regression test passes (<4MB alloc), FuzzReadBlockFromReader ran clean (~190K execs, no crashers), seed fixtures load.
Optional, non-blocking: line 307 passes the block.Subtrees slice to a %d verb (should be len(...)) — pre-existing and now effectively unreachable, but a one-line cleanup while you're here.
sgextcsi
approved these changes
Jun 2, 2026
oskarszoon
approved these changes
Jun 2, 2026
oskarszoon
left a comment
Contributor
There was a problem hiding this comment.
Reviewed. Correct, minimal fix for a real OOM vector — pre-sizing Subtrees from an untrusted varint. Capping the capacity hint is right; the append loop and post-loop length check preserve parse semantics, so valid blocks are unaffected. Fuzz + deterministic regression are well-targeted.
No blocking issues. One non-blocking nit: the allocDelta comment (block_fuzz_test.go:107) says the reading "is not racy" because it's "single-goroutine" — but TotalAlloc is process-wide. The test is fine (monotonic counter + ~512× margin), but the justification is misleading and could trip up someone who later tightens the threshold. Worth a one-line comment fix.
Optional: a deterministic regression through the public NewBlockFromBytes (not just readBlockFromReader) would close the OOM story at the API boundary — fuzz already covers it.
freemans13
added a commit
to freemans13/teranode
that referenced
this pull request
Jun 2, 2026
…tegration Merge stu/blockvalidation-inmemory-checkold (ca7ef31) — off-chain prefetch for checkOldBlockIDs plus the upstream/main it carried (bsv-blockchain#1005, bsv-blockchain#993, bsv-blockchain#1022). Conflict resolutions: - services/legacy/netsync/manager.go: kept integration's gap-watchdog self-heal (requestBlocksFromTip / maybeResyncOnMissingParent / missingParentResyncInterval); the incoming side had no content there. - stores/utxo/aerospike/batch_create.go: pass nil for the new GetBinsToStore arena param (main added *bt.Arena; integration-only BatchCreate caller predates it). nil = non-arena route, output-equivalent per create_arena_test.
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
4 participants
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.
Second in the deserializer-fuzzing series (after #1004). Targets the block parser and fixes the unbounded-allocation bug it surfaces.
Bug
readBlockFromReaderpre-sized the subtree slice directly from the block's subtree-count varint:A block from an untrusted peer claiming ~33M subtrees (or more, up to a varint's range) forces a multi-hundred-MB-to-GB allocation before the first missing hash fails the read — a cheap OOM that takes down the
coresidecar via the ServiceManager errgroup.NewBlockFromBytes'recover()converts ordinary panics to errors but does not save a genuine OOM. Notably theCoinbaseBUMPread just below was already bounded (1 MB); the subtree list was the gap.Fix
Cap the speculative capacity at
min(subtreeLength, 1024). The loop alreadyappends and validates each hash, so a bogus count now errors on the first missing hash instead of pre-allocating. Valid blocks parse unchanged.Tests
FuzzNewBlockFromBytes— public entry; guards the OOM/allocation class that itsrecover()cannot catch. Seeded frommodel/testdata/*.block.FuzzReadBlockFromReader— the inner body parser fuzzed without therecover()wrapper, so a panic in the varint / subtree-list / coinbase / bump parsing surfaces as a fuzz failure instead of being silently converted to an error in production.TestBlock_HostileSubtreeLengthIsBounded— deterministic regression: a tiny body claiming a huge subtree count allocates < 4 MB (was ~256 MB) before erroring. Fails on the old code, passes on the fix.Verification
268 MBobserved), passes after.FuzzReadBlockFromReader— ~525K execs, no crashers (no recover-masked panics surfaced; the inner reader also exercises go-bt/go-wire and stayed clean).FuzzNewBlockFromBytes— ~860K execs, no crashers.go test -race(block tests + fuzz corpus replay),go vet,gofmt,gci— clean.Last planned target (separate PR):
pkg/fileformatheader parsing.