validation: correct lifetime of precomputed tx data #35209

1. 
   darosior commented at 12:03 PM on May 5, 2026: member

   This is a cleanup that fixes the root cause of CVE-2024-52911, which was covertly fixed in #31112 (first released in 29.0).

   Security advisory for CVE-2024-52911 is available here.

2. validation: correct lifetime of precomputed tx data

   This makes sure `txsdata` always outlives the Script check queue (since local
   objects are destructed in reverse order of construction).
   
   This is the root cause for a security vulnerability reported by Cory Fields in
   2024 that could be exploited by crafting an invalid block to cause nodes to
   read freed memory. The vulnerability was covertly fixed in commit
   `492e1f09943fcb6145c21d470299305a19e17d8b`.
   
   See security advisory for CVE-2024-52911 for more details.

   1ed799fb21
3. DrahtBot added the label Validation on May 5, 2026
4. 
   DrahtBot commented at 12:03 PM on May 5, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35209.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	theuni, optout21, maflcko, achow101

   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

   Reviewers, this pull request conflicts with the following ones:

   • #32575 (consensus: Remove special treatment for single threaded script checking by fjahr)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

5. darosior referenced this in commit 9ad085bd83 on May 5, 2026
6. 
   maflcko commented at 12:33 PM on May 5, 2026: member

   concept ack. Personally I'd prefer to also use early-return on failure, because IIUC all of the validation code is structured that way and it seems odd to use a for-loop and break-pattern for this particular section. See also #31112 (review), but I guess that would be unrelated to the changes here and certainly not for backport.

7. fanquake added this to the milestone 32.0 on May 5, 2026
8. 
   darosior commented at 12:42 PM on May 5, 2026: member

   I agree. Removing the early returns was really only to covertly fix the vulnerability. I think it's fine, but now that it's public i guess this could also be cleaned up.

9. 
   theuni commented at 12:54 PM on May 5, 2026: member

   ACK 1ed799fb21db51a12cbd5579420a61b9b5b3ee7d. Thanks for the proper fix.

   +1 to reinstating the early returns too, but that can be done separately to make backporting more straightforward.

10. in src/validation.cpp:2511 in 1ed799fb21

    2507 | @@ -2508,11 +2508,10 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
    2508 |      // in multiple threads). Preallocate the vector size so a new allocation
    2509 |      // doesn't invalidate pointers into the vector, and keep txsdata in scope
    2510 |      // for as long as `control`.
    2511 | +    std::vector<PrecomputedTransactionData> txsdata(block.vtx.size());
    

    ---

    ---

    

    optout21 commented at 1:30 PM on May 5, 2026:

    Wouldn't this warrant also a comment above (at the end of the large comment block that discusses PrecomputedTransactionData lifetime), about why the order of declaration is crucial here? Soemthing like "txsdata must be declared before the guard control so that it's destructed only after the destruction of the guard."

    ---

    

    maflcko commented at 1:34 PM on May 5, 2026:

    Maybe as long as could be changed to longer than, but I think the comment is already correct, just the code is wrong.

    ---

    

    darosior commented at 2:51 PM on May 5, 2026:

    Yeah, @optout21 this is literally what the comment says already? "Precomputed transaction data pointers must not be invalidated until after control has run the script checks (potentially in multiple threads)." The issue was that the code did not do what the comment says.

    ---

    

    optout21 commented at 3:08 PM on May 5, 2026:

    Yes, it does, but it could be made more explicit regarding destruction order. It's not obvious, at least for a normie mortal like me (it may be obvious for seasoned C++ experts and LLMs...). Feel free to ignore if you consider over-explained.

    ---

    

    maflcko commented at 6:42 AM on May 6, 2026:

    Usually we have it the other way round: The code is correct and the comment is wrong. With the comment being already correct, I presume this could be a contributing factor why it hasn't been found for so long.

11. 
    optout21 commented at 1:31 PM on May 5, 2026: contributor

    ACK 1ed799fb21db51a12cbd5579420a61b9b5b3ee7d

12. 
    maflcko commented at 6:42 AM on May 6, 2026: member

    lgtm ACK 1ed799fb21db51a12cbd5579420a61b9b5b3ee7d

13. 
    achow101 commented at 7:23 AM on May 6, 2026: member

    ACK 1ed799fb21db51a12cbd5579420a61b9b5b3ee7d

14. achow101 merged this on May 6, 2026
15. achow101 closed this on May 6, 2026

    ---

16. 
    fanquake commented at 7:38 AM on May 6, 2026: member

    Backported to 31.x in #35210, 30.x in #35211, 29.x in #35212, 28.x in #35213.

17. achow101 referenced this in commit 572ef0fa43 on May 6, 2026
18. achow101 referenced this in commit 076629a3c1 on May 6, 2026
19. achow101 referenced this in commit de32850902 on May 6, 2026
20. darosior deleted the branch on May 6, 2026

Contributors

darosiorDrahtBotmaflckotheunioptout21achow101fanquake

Milestone
32.0