–with-sanitizers=float-divide-by-zero crash with -debug=bench in Chainstate::ConnectTip #27635

1. 
   maflcko commented at 7:45 am on May 12, 2023: member

   Not sure if this UB causes issues with any compiler. clang is fine, see:

   • -fsanitize=float-divide-by-zero: Floating point division by zero. This is undefined per the C and C++ standards, but is defined by Clang (and by ISO/IEC/IEEE 60559 / IEEE 754) as producing either an infinity or NaN value, so is not included in -fsanitize=undefined.

   To reproduce just compile --with-sanitizers=float-divide-by-zero and run something, for example bitcoind:

   0UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/bitcoind -datadir=/tmp -signet -debug=bench -noprinttoconsole 
   1validation.cpp:2839:5: runtime error: division by zero
   

   float-divide-by-zero is enabled by OSS-Fuzz, see https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=58887

   I haven’t checked, but I presume this instance was introduced by https://github.com/bitcoin/bitcoin/pull/24216

2. maflcko added the label Utils/log/libs on May 12, 2023
3. maflcko added the label Brainstorming on May 12, 2023
4. 
   dergoegge commented at 9:17 am on May 16, 2023: member

   cc @Sjors @jonatack @theStack
5. 
   maflcko commented at 9:32 am on May 16, 2023: member

   I think #24216 should just be reverted. It doesn’t make sense to assume that the number of all connected blocks is equal to the number of blocks read from disk. In fact, the most common case, calling ProcessNewBlock from net processing will have the block already in memory (read from the network).

   An alternative might be to use a dedicated read counter for it.

6. 
   Sjors commented at 10:55 am on May 16, 2023: member

   The potential divide by zero was introduced here:

   https://github.com/bitcoin/bitcoin/pull/24216/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98L2607-R2612

   Let me see if I remember why…

7. 
   Sjors commented at 11:19 am on May 16, 2023: member

   Opened #27673 which drops the offending metric (and makes the sanitizer happy on Ubuntu GCC 12.2.0).

   I’d rather not revert all of #24216 since the additional logging it introduced makes it easier to spot when we get a block from disk vs when from memory. I vaguely remember this being useful to debug … something

8. fanquake closed this on May 30, 2023

   ---

9. BlackcoinDev referenced this in commit f467b28ac3 on Feb 5, 2024
10. bitcoin locked this on May 29, 2024

Contributors
maflcko dergoegge Sjors

Labels
Brainstorming Utils/log/libs