Skip to content

fix(file_store): reject an entry length prefix larger than the file - #2313

Open
wangzhengzhuo05 wants to merge 1 commit into
bitcoindevkit:masterfrom
wangzhengzhuo05:fix/file-store-oversized-entry-len
Open

wangzhengzhuo05 wants to merge 1 commit into
bitcoindevkit:masterfrom
wangzhengzhuo05:fix/file-store-oversized-entry-len

Conversation

@wangzhengzhuo05

Copy link
Copy Markdown

Fixes #2306

What

EntryIter::next decoded each entry with bincode_options().deserialize_from(..) and no size
limit, so a length prefix read from the file was passed straight to the buffer allocation. A
store file whose entry declares a String length of u64::MAX panicked inside Store::load:

thread 'load_returns_error_on_oversized_length_prefix' panicked at library/alloc/src/raw_vec/mod.rs:28:5:
capacity overflow

How

Each entry is now decoded with the deserializer bounded by the bytes remaining in the file,
multiplied by nine:

  • bincode charges size_of::<u64>() (8 bytes) for every length literal it decodes, even when the
    varint encoding occupies a single byte on disk, and then charges the payload length again. A
    valid entry therefore charges at most 8 * bytes + bytes.
  • Every length literal occupies at least one byte on disk, so the total charge can never exceed
    nine times the bytes remaining — the bound cannot reject a store that fits in the file.
  • Any length prefix that cannot be satisfied now fails with bincode::ErrorKind::SizeLimit
    before an allocation is attempted, and the existing error arm maps that to
    StoreError::Bincode.
  • remaining == 0 (end of file) short-circuits to the existing clean-EOF return instead of
    handing the deserializer a zero limit.

Only crates/file_store/src/entry_iter.rs changes; lib.rs and store.rs are untouched.

Tests

Added crates/file_store/tests/test_oversized_entry_len.rs:

  • load_returns_error_on_oversized_length_prefix — the reproduction from the issue: load
    returns an error instead of panicking.
  • valid_store_still_round_trips — create/append/load two changesets, so the bound is not
    tighter than a real store needs.
cargo test -p bdk_file_store                                 # 10 + 2 + 1 passed
cargo clippy -p bdk_file_store --all-targets -- -D warnings   # clean
cargo fmt --all -- --check                                    # clean

Mutation check: with this change reverted,
load_returns_error_on_oversized_length_prefix panics with capacity overflow; with the change
applied, it passes.

Note on #2258

#2258 (replace bincode with postcard) also bounds the frame allocation and rewrites this same
file, so it supersedes this fix if it lands first. This change is scoped to the current framing
and is independently correct and testable today; I am happy to rebase or drop it in favour of
#2258 if that is the preferred path.

Implemented with AI assistance (OpenCode + muse-spark 1.3); the reproduction, the full crate
test suite, clippy, rustfmt and the mutation check above were run locally and reviewed before
submission.

EntryIter::next deserialized each entry without a size limit, so a length
prefix read from a corrupted store file was passed straight to the buffer
allocation: a `String` length of `u64::MAX` panicked with `capacity overflow`
inside `Store::load`.

Bound each entry's deserialization by the bytes remaining in the file. bincode
reports every length it reads to its `SizeLimit` and returns `ErrorKind::SizeLimit`
before allocating, so the oversized prefix now surfaces as `StoreError::Bincode`.
The bound cannot reject a well-formed store: an entry can never be larger than the
bytes left to read.
@codecov

codecov Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.85%. Comparing base (c6a6073) to head (6edf809).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2313      +/-   ##
==========================================
+ Coverage   78.84%   78.85%   +0.01%     
==========================================
  Files          31       31              
  Lines        6060     6073      +13     
  Branches      288      289       +1     
==========================================
+ Hits         4778     4789      +11     
- Misses       1203     1204       +1     
- Partials       79       80       +1     
Flag Coverage Δ
rust 78.85% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nymius

nymius commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Hi @wangzhengzhuo05, bincode must be replaced because is unmaintained. postcard is the best candidate we have so far.
As this is the priority, this PR is not going to get merged before #2258, which would make this PR obsolete.
I recommend you to close this PR and focus on review of #2258 or look for other issues to fix in BDK, if you are up to it.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

file_store: oversized entry length prefix panics Store::load instead of returning an error

2 participants