#35440 wallet: check descriptor cache xpub length before decoding

full analysis

https://github.com/bitcoin/bitcoin/pull/35440 · alhudz · +225/-85 in 11 files, 2 commits · labels: Wallet, CI failed

Goal

  • Prevent crashes when loading corrupted or truncated wallet descriptor cache records
  • Turn out-of-bounds memory reads into clean database corruption errors when opening a wallet

This PR checks serialized xpub lengths before decoding wallet descriptor cache records, preventing an out-of-bounds read when loading corrupted or truncated records. It also refactors CExtKey and CExtPubKey to implement standard Serialize/Unserialize stream methods, replacing custom buffer-based Encode and Decode functions across the codebase.

Problem: When loading wallet descriptor cache entries, CExtPubKey::Decode assumed a full 74-byte buffer without verifying the deserialized byte vector length, causing an out-of-bounds container read under ASan on short records. Additionally, CExtKey and CExtPubKey were unique in using raw pointer output parameters for serialization rather than standard stream abstractions.

Category: Wallet (#40 of 84)

P3 · bug fix

  • P3 because it prevents an out-of-bounds read crash when loading corrupted descriptor cache records
  • It turns an invalid memory read into a clean database error when reading corrupted wallet files
  • Impact is limited since truncated or corrupted descriptor cache records are rare in practice

Fixes a container-overflow crash when reading corrupt descriptor cache entries from the wallet database, turning an invalid memory read into a clean DBErrors::CORRUPT failure. It also eliminates raw pointer buffer writes by migrating extended keys to idiomatic stream serialization.

Membership: Directly fixes descriptor cache record deserialization in src/wallet/walletdb.cpp and adds walletload regression tests.

Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 1

Reviewability: Stale: CI failing

  • CI is failing on the latest push and needs a rerun or maintainer workflow approval

CI has failed on the latest push and needs a rerun or maintainer workflow approval.

Author status: Active, addressed all reviewer comments in the latest push.

Resolved concerns:

  • Added unit test coverage for truncated xpub cache entries in walletload_tests (maflcko, winterrdog)
  • Replaced raw buffer Encode/Decode with idiomatic Serialize/Unserialize stream interfaces (achow101)
  • Secured temporary private key deserialization buffer with secure_allocator and added stream error negative tests (molnard)
  • Addressed 32-bit compiler array-bounds diagnostics by breaking up stream chaining in unit tests (winterrdog, molnard)

Agreement: Strong

  • Strong consensus on fixing the bounds check and refactoring key serialization
  • Requested and approved converting extended keys to standard serialization streams (achow101)
  • Thoroughly tested and approved both the initial fix and the stream refactoring (molnard)
  • Validated unit test improvements and stream decoupling (winterrdog)

Strong consensus on migrating extended key serialization to streams with tests verified

Reviewers agreed that raw pointer decoding was error-prone and requested the stream refactor, which was implemented, thoroughly tested, and re-ACKed.

  • achow101 requested converting CExtKey/CExtPubKey to standard Serialize/Unserialize and ACKed de971e1
  • molnard thoroughly tested and ACKed both the initial bug fix and the latest refactored commit 3d39505
  • winterrdog validated the test improvements and stream decoupling

Review verdicts (DrahtBot): 1 (+1)

Files

165 lines under test/bench/ci.

  • src/test/bip32_tests.cpp +114/-3
  • src/wallet/test/walletload_tests.cpp +45/-0
  • src/pubkey.cpp +0/-30
  • src/wallet/walletdb.cpp +18/-12
  • src/key.h +20/-2
  • src/pubkey.h +18/-4
  • src/key.cpp +0/-19
  • src/key_io.cpp +5/-8
  • src/psbt.h +2/-4
  • src/rpc/rawtransaction.cpp +1/-2
  • src/test/fuzz/script_descriptor_cache.cpp +2/-1

Card

This PR fixes a container overflow in the wallet descriptor cache loader when encountering truncated xpub entries, ensuring invalid records fail cleanly as database corruption. At reviewer request, it refactors CExtKey and CExtPubKey serialization to use standard streams rather than raw pointer output buffers across wallet, PSBT, and key IO code paths. The bug fix addresses memory safety on corrupted databases, while the refactor eliminates an error-prone pattern across the codebase. The change has strong reviewer support and ACKs from maintainers and contributors, but CI is currently marked failing pending maintainer action.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T16:19, confidence high, input hash 9c813a872f8c5815