net: cast vector size to avoid overflow, truncation, sign change #36321

1. 
   Crypt-iQ commented at 8:32 PM on September 23, 2026: contributor

   When running with -fsanitize=integer compiled, the following can error here:

   SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /bitcoin/src/net_processing.cpp:3662:33
   SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation-or-sign-change /bitcoin/src/net_processing.cpp:3662:18
   

   When stop_index->nHeight is less than CFCHECKPT_INTERVAL, the headers vector will be empty. This will just set the loop counter to -1 and never enter the loop, so this is harmless anyways. Fix this by casting headers.size() to int.

2. DrahtBot added the label Tests on Sep 23, 2026
3. 
   DrahtBot commented at 8:32 PM on September 23, 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/36321.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

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

   Type	Reviewers
   ACK	maflcko, davidgumberg
   Stale ACK	dergoegge

   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.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

4. 
   Crypt-iQ commented at 8:33 PM on September 23, 2026: contributor

   I think this was mentioned somewhere else, but I can't find the thread anymore. Just need this so fuzzamoto can continue without yelling if enabling -fsanitize=integer.

5. 
   fanquake commented at 8:37 PM on September 23, 2026: member

   cc @dergoegge

6. dergoegge approved
7. 
   dergoegge commented at 7:28 AM on September 24, 2026: member

   utACK 05f2d59cfce15dd0ceb2b3fc04e09ae7ea22aee1

8. 
   maflcko commented at 8:52 AM on September 24, 2026: member

   I am not sure about wholesale disabling sanitizers for this function, when only a single cast is needed.

   This is not crypto code where the whole function is expected to do unsigned integer truncation or overflow.

   If the assumption is that int can hold the size, it should just be expressed so in the code:

       for (int i = int(headers.size()) - 1; i >= 0; i--) {
   

   Also, I am a bit confused why the Bitcoin Core fuzzers don't find this. According to the coverage reports, it fails in the prior line:

       3652         [ -  + ]:           8 :     if (!PrepareBlockFilterRequest(node, peer, filter_type, /*start_height=*/0, stop_hash,
   

9. 
   dergoegge commented at 9:04 AM on September 24, 2026: member

   Also, I am a bit confused why the Bitcoin Core fuzzers don't find this. According to the coverage reports, it fails in the prior line:

   Just a guess but I think the filter index might just not be enabled in any of the harnesses?

10. Crypt-iQ force-pushed on Sep 24, 2026
11. net: cast vector size to avoid overflow, truncation, sign change 308cd67019
12. Crypt-iQ force-pushed on Sep 24, 2026
13. DrahtBot added the label CI failed on Sep 24, 2026
14. 
    maflcko commented at 1:56 PM on September 24, 2026: member

    lgtm ACK 308cd670195d908fbd50caf76a8a215386121bd0

15. DrahtBot requested review from dergoegge on Sep 24, 2026
16. 
    Crypt-iQ commented at 1:57 PM on September 24, 2026: contributor

    Updated per #36321 (comment)

    I ran into this when starting to write a bip157 fuzz harness a few months ago, but never completed it and I think nothing else covers it as @dergoegge says.

17. 
    maflcko commented at 2:02 PM on September 24, 2026: member

    forgot to update title?

18. Crypt-iQ renamed this:
    test: ubsan suppression for ProcessGetCFCheckPt
    net: cast vector size to avoid overflow, truncation, sign change
    on Sep 24, 2026
19. DrahtBot removed the label CI failed on Sep 24, 2026
20. 
    davidgumberg commented at 10:09 PM on September 24, 2026: contributor

    crACK https://github.com/bitcoin/bitcoin/commit/308cd670195d908fbd50caf76a8a215386121bd0

    I don't know much about and haven't tested whether this fixes the fuzzamoto -fsanitize=integer complaints, but it makes sense to me that the sanitizer would complain and the code changes look good to me.

    Separately just adding to the pile that it would be good to have fuzz coverage for this in one of the bitcoin core harnesses.

21. 
    maflcko commented at 5:36 PM on September 25, 2026: member

    Separately just adding to the pile that it would be good to have fuzz coverage for this in one of the bitcoin core harnesses.

    Did something basic in https://github.com/bitcoin/bitcoin/pull/36337

Contributors

Crypt-iQDrahtBotfanquakedergoeggemaflckodavidgumberg

Review Requested
dergoegge