wallet: harden the read-only BDB parser against crafted files #35990

1. 
   shuv-amp commented at 9:15 PM on August 16, 2026: contributor

   The read-only BDB parser in migrate.cpp runs on any file passed to migratewallet (RPC or GUI) or bitcoin-wallet. #34959 added checks for btree levels and overflow lengths, which stop simple cycles, but a crafted file can still make the parser hang or use a lot of memory:

   • A btree page can be the child of more than one internal page. Its level still checks out, so it gets parsed again for every path that reaches it. On master an ~8 KB file built this way doesn't finish.
   • An overflow page with no data doesn't count toward the length #34959 checks, so two such pages pointing at each other loop forever. Separately, any number of records can point at the same overflow chain, and each one reads and stores its own copy.
   • All of a page's index entries can point at the same record, which then gets read and kept once per entry. A single 64 KB page can turn into ~500 MB this way. On master a 256 KB file took migratewallet from 54 MB to ~800 MB.

   The first two are fixed by tracking visited pages (the approach from #34946): btree pages in one set, and overflow pages in another that covers the whole tree. For the third, a page is rejected once its records add up to more than the page size.

   This shouldn't affect real wallets. In a valid btree every page has a single parent and every overflow page belongs to a single record. Records on a page can't overlap either, since the parser already rejects databases that allow duplicates.

   Each fix comes with a test in db_tests.cpp that builds the bad file by hand, plus a test that a page filled to the last byte still parses. wallet_migration.py passes with v28.2.

   The last commit adds two errors from #34959 to the wallet_bdb_parser fuzz target. They were missing, so the target aborted on inputs that reach them. I ran into this while feeding mutated copies of the test files through the target.

   Supersedes #35992.

2. DrahtBot added the label Wallet on Aug 16, 2026
3. 
   DrahtBot commented at 9:16 PM on August 16, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   <!--006a51241073e994b41acfe9ec718e94-->

   External sites

   • Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/35990
   • Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/35990.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/35990#author-ralph

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline and AI policy for information on the review process.

   Type	Reviewers
   Stale ACK	fametrano, vicjuma

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   No conflicts as of last run.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

   LLM Linter (✨ experimental)

   Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

   • src/wallet/test/db_tests.cpp (bdbro_btree_page_referenced_twice): builder.Index(3, 0, 30)
   • src/wallet/test/db_tests.cpp (bdbro_btree_page_referenced_twice): builder.Index(3, 1, 30)
   • src/wallet/test/db_tests.cpp (bdbro_overflow_page_referenced_twice): builder.Index(3, 0, 30)
   • src/wallet/test/db_tests.cpp (bdbro_overflow_page_referenced_twice): builder.Index(3, 1, 34)
   • src/wallet/test/db_tests.cpp (bdbro_overflow_page_data_does_not_fit): builder.Index(3, 0, 30)
   • src/wallet/test/db_tests.cpp (bdbro_overflow_page_data_does_not_fit): builder.Index(3, 1, 34)
   • src/wallet/test/db_tests.cpp (bdbro_full_overflow_pages_parse): builder.Index(3, 0, 30)
   • src/wallet/test/db_tests.cpp (bdbro_full_overflow_pages_parse): builder.Index(3, 1, 34)

   ^{2026-11-03 17:27:55}

4. shuv-amp marked this as ready for review on Aug 16, 2026
5. shuv-amp marked this as a draft on Aug 17, 2026
6. shuv-amp force-pushed on Aug 17, 2026
7. shuv-amp renamed this:
   wallet: Reject revisited pages in the read-only BDB parser
   wallet: harden the read-only BDB parser against crafted files
   on Aug 17, 2026
8. shuv-amp marked this as ready for review on Aug 17, 2026
9. shuv-amp force-pushed on Aug 17, 2026
10. sedited requested review from achow101 on Oct 2, 2026
11. shuv-amp force-pushed on Oct 6, 2026
12. 
    fametrano commented at 4:39 AM on October 15, 2026: contributor

    tACK 2dfce13f9f

    Tested on macOS arm64: the new rejection tests fail on master (the self-referencing overflow page hangs) and pass here, as do the unit tests, wallet_migration.py and migrating a v28.2 legacy wallet with internal and overflow pages. Details on request.

13. in src/wallet/migrate.cpp:445 in 2dfce13f9f outdated

     440 |              }
     441 |              default:
     442 |                  throw std::runtime_error("Unknown record type in records page");
     443 |              }
     444 |  
     445 | +            if (records_size > m_page_size) {
    

    ---

    ---

    

    vicjuma commented at 10:53 AM on November 3, 2026:

    Works as expected, and the unit test fails when commented out, meaning that, as per the test master is accepting record sizes exceeding the file size

    Before: <img width="2834" height="422" alt="Image" src="https://github.com/user-attachments/assets/f2ef6e7b-bc73-42d6-ac00-12d98e098d9c" />

    After:

    <img width="1736" height="324" alt="Image" src="https://github.com/user-attachments/assets/91863fbd-f210-4d8f-92c7-873c471b429d" />

14. in src/wallet/migrate.cpp:533 in 2dfce13f9f outdated

     528 | @@ -514,6 +529,11 @@ class InternalPage
     529 |              records.emplace_back(record);
     530 |              to_jump += InternalRecord::FIXED_SIZE + rec_hdr.len;
     531 |  
     532 | +            records_size += RecordHeader::SIZE + InternalRecord::FIXED_SIZE + rec_hdr.len;
     533 | +            if (records_size > m_page_size) {
    

    ---

    ---

    

    vicjuma commented at 10:59 AM on November 3, 2026:

    Same as above

    ---

    

    shuv-amp commented at 5:44 PM on November 3, 2026:

    Thanks, the old test was rejected by the revisited page check first. Fixed in 8b0b1f026f: the record is now marked deleted, so only this check catches it.

15. in src/wallet/migrate.cpp:721 in 2dfce13f9f outdated

     717 | @@ -691,6 +718,9 @@ void BerkeleyRODatabase::Open()
     718 |                          throw std::runtime_error("Overflow record has an impossible length");
     719 |                      }
     720 |                      while (next_page != 0) {
     721 | +                        if (!visited_overflow.insert(next_page).second) {
    

    ---

    ---

    

    vicjuma commented at 11:06 AM on November 3, 2026:

    Prevents infinite loops, which may lead to DoS and probably resource exhaustion. Running that unit test constructing the malformed file guarded by this makes the test run infinitely

16. 
    vicjuma commented at 11:16 AM on November 3, 2026: contributor

    ACK 2dfce13f9fd3623462eada54731f4ec8ef2ca438. All the tests run as expected and the code additions on /src/wallet/migrate.cpp also appear reasonable and self-contained (supporting definitions, additional function parameters, and conditional checks to strengthen validation)

17. wallet, bdbro: Reject revisited btree pages

    Validating btree page levels stops a page from being its own ancestor,
    but a page can still be reachable from more than one parent while
    keeping a consistent level. Such a page, and everything below it, is
    parsed again for every path that reaches it, so a small file can
    describe an exponential amount of work. A valid btree references each
    page once, so keep track of the pages already visited and reject any
    that is seen again.

    f6ec4ecd04
18. wallet, bdbro: Reject revisited overflow pages

    Enforcing overflow data lengths stops a chain whose pages carry data,
    but overflow pages that carry no data never advance the accumulated
    length, so a chain that cycles through empty overflow pages loops
    forever. Any number of records can also point at the same chain, which
    is then read and stored once per record.
    
    In a valid database each overflow page belongs to one record, so keep
    track of the overflow pages visited across the whole tree and reject
    one that is seen again.

    96e8842c1d
19. wallet, bdbro: Enforce that a page's records fit in the page

    A page's index entries each point at a record that is read and kept, but
    the offsets are not checked against each other. A page whose entries all
    point at the same record is therefore read into entries * len bytes of
    memory, so a single page can be read into far more than its own size. In
    a valid page the records do not overlap and cannot be larger than the
    page, so track their total size while reading and reject a page whose
    records do not fit.

    8b0b1f026f
20. fuzz: Add missing BDB overflow errors to wallet_bdb_parser

    #34959 added two errors for invalid overflow records, but did not add
    them to the errors the wallet_bdb_parser target expects, so the target
    aborts on inputs that reach them.

    897d1cdd27
21. wallet, bdbro: Reject oversized overflow page data

    An overflow page stores the length of its data in hf_offset, which is
    not checked against the page size. A 512 byte page can claim up to 65535
    bytes, read from the pages that follow it, so a file can be read into up
    to about 128 times its own size.
    
    BDB never stores more on an overflow page than fits after the page
    header, so reject a page whose data does not fit.

    e0c98a22a6
22. shuv-amp force-pushed on Nov 3, 2026
23. 
    shuv-amp commented at 5:56 PM on November 3, 2026: contributor

    Pushed a new check and fixed a test gap.

    e0c98a22a6: reject overflow pages whose data length exceeds page_size - 26, the BDB per-page maximum. 8b0b1f026f: the internal-page test record is now deleted, so only the size check can reject it. Addresses @vicjuma's comment. The rest is test formatting only.

    git range-diff 2dfce13f9f...e0c98a22a6

24. DrahtBot added the label CI failed on Nov 3, 2026
25. DrahtBot removed the label CI failed on Nov 5, 2026

Contributors

shuv-ampDrahtBotfametranovicjuma

Review Requested
achow101