Make all tests pass UBSan without using any UBSan suppressions #17208

practicalswift · 2019-10-21T17:17:03Z

Make all tests pass UBSan (-fsanitize=undefined) without using any UBSan suppressions.

From a fuzzing perspective it would be really nice to be able to run UBSan without having to deal with suppression files :)

Note that the commit c8c393b needs to be cherry-picked in from PR #17156 to get rid of the two alignment related suppressions :)

Before this PR:

$ ./configure --with-sanitizers=undefined
$ make
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" src/test/test_bitcoin
…
validation.cpp:2563:164: runtime error: division by zero
validation.cpp:2568:138: runtime error: division by zero
validation.cpp:2573:161: runtime error: division by zero
validation.cpp:2582:164: runtime error: division by zero
validation.cpp:2583:144: runtime error: division by zero
policy/fees.cpp:347:44: runtime error: division by zero
policy/fees.cpp:344:44: runtime error: division by zero
wallet/wallet.cpp:2049:67: runtime error: division by zero
wallet/wallet.cpp:3280:51: runtime error: division by zero
wallet/wallet.cpp:3283:51: runtime error: division by zero
…
$ echo $?
1
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" test/functional/test_runner.py
…
AssertionError: Unexpected stderr validation.cpp:2563:164: runtime error: division by zero
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:2563:164 in
validation.cpp:2568:138: runtime error: division by zero
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:2568:138 in
validation.cpp:2573:161: runtime error: division by zero
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:2573:161 in
validation.cpp:2582:164: runtime error: division by zero
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:2582:164 in
validation.cpp:2583:144: runtime error: division by zero
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:2583:144 in !=
…
$ echo $?
1

After this PR (with cherry-pick as described above):

$ ./configure --with-sanitizers=undefined
$ make
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" src/test/test_bitcoin
$ echo $?
0
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" test/functional/test_runner.py
$ echo $?
0

Note: This PR is not a bug fix (there is no bug to fix!). We assume the floating-point types to fulfil the requirements of IEC 559 (IEEE 754) standard: floating-point division by zero is thus well-defined.

The IEC 559 (IEEE 754) assumption is made explicitly in assumptions.h and also checked at compile-time:

bitcoin/src/compat/assumptions.h

Lines 31 to 36 in a22b624

    
           // Assumption: We assume the floating-point types to fulfill the requirements of 
        
           //             IEC 559 (IEEE 754) standard. 
        
           // Example(s): Floating-point division by zero in ConnectBlock, CreateTransaction 
        
           //             and EstimateMedianVal. 
        
           static_assert(std::numeric_limits<float>::is_iec559, "IEEE 754 float assumed"); 
        
           static_assert(std::numeric_limits<double>::is_iec559, "IEEE 754 double assumed");

However, UBSan is not aware of that assumption we're making.

DrahtBot · 2019-10-21T22:27:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Remove calls to Chain::Lock methods #17954 (wallet: Remove calls to Chain::Lock methods by ryanofsky)
Add Single Random Draw as an additional coin selection algorithm #17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
Use effective values throughout coin selection #17331 (Use effective values throughout coin selection by achow101)

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.

…es fixed). Add documentation. 0616138 tests: Remove no longer needed UBSan suppressions (issues fixed). Add documentation. (practicalswift) Pull request description: Remove no longer needed UBSan suppressions (issues fixed). Add documentation. This PR is the CI-only subset of #17208 (which touches code). From a fuzzing perspective it would be really nice to be able to run UBSan with as few suppressions as possible :) Top commit has no ACKs. Tree-SHA512: a926ab3e80e12a805af110fbff470cdc61ef4db536919a5b8896ea8b70f761114a52d9b1c0f48b11c1d48338351bf2e003e01ce60c613612f26ba298dcc29cd9

gmaxwell · 2019-10-29T21:39:47Z

Primary platforms Bitcoin runs on (x86 / win linux) are not strictly IEEE 754 (in particular, the standard x86 FPU is non-754 due to excess precision and non-strict truncation when moving between registers and memory). It's also not too uncommon for random obscure library code to silently dork around with rounding settings even on SSE (which is much more 754). Standard compiler optimization settings also break IEEE 754 handling even more than the base platform.

So I think it's not especially wise to make a very strong assumption of IEEE 754 overall.

I don't know if any of that directly impacts this change (though 0/0 is NaN as is 1 / +/-0) Also, there are values other than ==0.0 which in division can produce under/overflow so it's not instantly obvious that the 0.0 equality tests in the code are correct. (in general, equality tests in floating point code are frequent sources of bugs).

Inserting extra floating point variables to silence a non-error error seems kinda ugly. Considering that these are just debugging messages, it might be better to just rework them (e.g. don't print if there is nothing to report, or don't divide numbers just give the raw numbers). Returning "inf" in debug lines also might needlessly mess up parsing just as much as not outputting the log in 'impossible' cases where the time elapsed is zero.

practicalswift · 2019-10-29T23:39:39Z

@gmaxwell Thanks for reviewing. Those are all good points. I agree that we should be careful with the IEEE 754 assumptions we're making.

I've now pushed an updated version of this PR -- now doing:

1.)

-static int64_t nBlocksTotal = 0;
+static int64_t nBlocksTotal = 1;

As suggested in a related PR: "This change would have been better done-- easier to review for correctness, more stable against "regression"-- by simply changing the existing initialization constant for nBlocksTotal from 0 to 1."

2.)

As suggested: now giving raw numbers instead of percentages.

3.)

Setting m_scanning_progress to 0.0 in the case of !(progress_end - progress_begin > 0.0) to avoid any potential floating-point division by zero.

Please re-review :)

Fun bonus trivia

$ cling
[cling]$ #include <iostream>
[cling]$ void f(double d) {
[cling]$     if (d == 0.0) {
[cling]$     } else if (d < 0.0) {
[cling]$     } else if (d > 0.0) {
[cling]$     } else {
[cling]$         std::cout << "reachable.\n";
[cling]$     }
[cling]$ }
[cling]$ f(0.0/0.0)
reachable.
[cling]$ .q
$

:)

practicalswift · 2019-11-14T09:18:03Z

@MarcoFalke @laanwj @Empact @promag You've all shown interest in the sanitizers before: would you mind reviewing? :)

src/wallet/wallet.cpp

src/validation.cpp

src/wallet/wallet.cpp

practicalswift · 2019-12-29T15:51:41Z

Feel free to review the updated version - would be nice to get rid of the UBSan suppressions :)

jonatack

Big concept ACK -- I was about to open a PR about this and then saw this PR.

src/wallet/wallet.cpp

jonatack

ACK 9724a0c code review, built with sanitizers=undefined and -Weverything, ran tests/bitcoind.

I'm sympathetic to @Empact's comments and suggestions above (and mine).

I'd like to see this PR and #17708 merged to facilitate and encourage testing with UBSan by default as a general review practice.

practicalswift · 2020-02-20T19:56:34Z

@jonatack I'm leaving this one as up for grabs. Feel free to cherry pick in the commits in a new PR: I think this PR has a greater probability of getting merged if you submit it TBH :)

jonatack · 2020-02-20T20:04:34Z

@practicalswift heh, to the contrary you are more effective than I at having testing improvements merged -- nevertheless, I empathise :) Let's see if #15283 makes headway.

practicalswift · 2020-02-23T10:06:56Z

I was informed that there was some interest expressed in this PR at this week's meeting. Re-opening to give it another chance :)

Pinging in @jonatack, @laanwj. @instagibbs and @elichai who I understood sounded positive at the meeting - would you mind reviewing? :)

This net change of -8 lines should hopefully be easy to review for correctness :)

elichai · 2020-02-23T16:03:05Z

I can't get the sanitizer to complain about these, tried both gcc and clang with commit ab9de43

$ make clean
$ ./configure --with-sanitizers=undefined --with-incompatible-bdb
$ make
$ ./src/test/test_bitcoin
Running 396 test cases...

*** No errors detected

tried even removing the first 6 lines in ./test/sanitizer/suppressions/ubsan
@practicalswift what am I doing wrong?

jonatack · 2020-02-23T16:10:56Z

@elichai try make distclean?

jonatack · 2020-02-23T16:15:43Z

@practicalswift thanks for re-opening. I prefer this PR for the reasons I mention in #15283 (comment), so my review above stands.

elichai · 2020-02-23T16:28:23Z

@elichai try make distclean?

Still nothing :/
Maybe I should re-clone? or can it be because I have a very new compiler? (GCC 9.2.1 and Clang 9.0.1)

It does seem to be part of the build: (and I added a out of bound read to one of the tests and it caught it)

Options used to compile and link:                                                                                                                                                                                                                                               
  with wallet   = yes                                                                                                                   
  with gui / qt = yes                                      
    with qr     = yes                                                                                                                   
  with zmq      = yes                                                                                                                   
  with test     = yes                                                                                                                   
    with prop   = no                                                                                                                    
    with fuzz   = no                                
  with bench    = yes                           
  with upnp     = yes                            
  use asm       = yes                                          
  sanitizers    = undefined                                                                                                             
  debug enabled = no                                                                                                                                                                                                                                                            
  gprof enabled = no                                                                                                                    
  werror        = no                                                                                                                    
                                                                                                                                        
  target os     = linux                                    
  build os      =                                                                                                                                                                                                                                                               

  CC            = /usr/bin/ccache gcc
  CFLAGS        = -g -O2
  CPPFLAGS      =   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
  CXX           = /usr/bin/ccache g++ -std=c++11
  CXXFLAGS      =   -fstack-reuse=none -Wstack-protector -fstack-protector-all  -Wall -Wextra -Wformat -Wvla -Wswitch -Wredundant-decls -Wunused-variable -Wdate-time  -Wno-unused-parameter -Wno-implicit-fallthrough   -g -O2
  LDFLAGS       = -pthread  -Wl,-z,relro -Wl,-z,now -pie  
  ARFLAGS       = cr

practicalswift · 2020-02-23T20:39:54Z

@elichai I think the reason you're not seeing this when using Clang is that you're using a more recent Clang than the one we're using in Travis: float-divide-by-zero was removed from the -fsanitize=undefined by https://reviews.llvm.org/D63793. Could that be the case?

elichai · 2020-02-24T10:08:11Z

@elichai I think the reason you're not seeing this when using Clang is that you're using a more recent Clang than the one we're using in Travis: float-divide-by-zero was removed from the -fsanitize=undefined by https://reviews.llvm.org/D63793. Could that be the case?

Makes sense. but I also can't see it with gcc not just clang
Just found this:
https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html

-fsanitize=float-divide-by-zero
Detect floating-point division by zero. Unlike other similar options, -fsanitize=float-divide-by-zero is not enabled by -fsanitize=undefined, since floating-point division by zero can be a legitimate way of obtaining infinities and NaNs.

elichai · 2020-02-24T13:44:56Z

Finally.
Before:

./configure --with-sanitizers=undefined,float-divide-by-zero --with-incompatible-bdb
make
./src/test/test_bitcoin                          
Running 396 test cases...                                                                                
 ==                                                                                                      
wallet/wallet.cpp:1669:67: runtime error: division by zero                                               
wallet/wallet.cpp:2945:51: runtime error: division by zero                                                                                                                                                         
wallet/wallet.cpp:2942:51: runtime error: division by zero                                               
                                                                                                         
*** No errors detected

After:

./configure --with-sanitizers=undefined,float-divide-by-zero --with-incompatible-bdb
make
./src/test/test_bitcoin                          
Running 396 test cases...

*** No errors detected

practicalswift · 2020-03-07T13:12:00Z

I'll leave this PR open for one week to allow for ACK:s to measure interest :)

Will close if no interest shown :)

practicalswift · 2020-03-10T08:12:43Z

Closing due to lack of interest :)

…s (issues fixed). Add documentation. 0616138 tests: Remove no longer needed UBSan suppressions (issues fixed). Add documentation. (practicalswift) Pull request description: Remove no longer needed UBSan suppressions (issues fixed). Add documentation. This PR is the CI-only subset of bitcoin#17208 (which touches code). From a fuzzing perspective it would be really nice to be able to run UBSan with as few suppressions as possible :) Top commit has no ACKs. Tree-SHA512: a926ab3e80e12a805af110fbff470cdc61ef4db536919a5b8896ea8b70f761114a52d9b1c0f48b11c1d48338351bf2e003e01ce60c613612f26ba298dcc29cd9

fanquake added the Refactoring label Oct 21, 2019

practicalswift force-pushed the ubsan-warnings branch from 84d6adc to 5b88e64 Compare October 21, 2019 18:20

practicalswift mentioned this pull request Oct 21, 2019

tests: Remove no longer needed UBSan suppressions (issues fixed). Add documentation. #17209

Merged

DrahtBot added the Needs rebase label Oct 22, 2019

practicalswift force-pushed the ubsan-warnings branch from 5b88e64 to bc993e8 Compare October 22, 2019 19:32

DrahtBot removed the Needs rebase label Oct 22, 2019

practicalswift force-pushed the ubsan-warnings branch 2 times, most recently from c4107b2 to ed3e044 Compare October 29, 2019 23:41

elichai mentioned this pull request Nov 4, 2019

RFC: Rust code integration #17090

Closed

Empact reviewed Nov 14, 2019

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

Empact reviewed Nov 14, 2019

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

Empact reviewed Nov 14, 2019

View reviewed changes

src/validation.cpp Show resolved Hide resolved

maflcko reviewed Nov 14, 2019

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the ubsan-warnings branch from ed3e044 to 9724a0c Compare November 14, 2019 14:52

practicalswift mentioned this pull request Nov 20, 2019

Use natural alignment for prevector #17530

Closed

practicalswift mentioned this pull request Dec 10, 2019

prevector: avoid misaligned member accesses #17708

Merged

practicalswift requested a review from ryanofsky January 14, 2020 22:36

jonatack reviewed Feb 4, 2020

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

practicalswift mentioned this pull request Feb 10, 2020

Get rid of VARINT default argument #18087

Merged

jonatack reviewed Feb 10, 2020

View reviewed changes

jonatack mentioned this pull request Feb 21, 2020

log: Fix UB with bench on genesis block #15283

Merged

practicalswift reopened this Feb 23, 2020

practicalswift closed this Mar 10, 2020

This was referenced Nov 8, 2020

wallet: fix scanning progress calculation for single block range #20344

Merged

wallet: fix potential division by 0 in WalletLogPrintf #20378

Merged

practicalswift deleted the ubsan-warnings branch April 10, 2021 19:39

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

	// Assumption: We assume the floating-point types to fulfill the requirements of
	// IEC 559 (IEEE 754) standard.
	// Example(s): Floating-point division by zero in ConnectBlock, CreateTransaction
	// and EstimateMedianVal.
	static_assert(std::numeric_limits<float>::is_iec559, "IEEE 754 float assumed");
	static_assert(std::numeric_limits<double>::is_iec559, "IEEE 754 double assumed");

Make all tests pass UBSan without using any UBSan suppressions #17208

Make all tests pass UBSan without using any UBSan suppressions #17208

Uh oh!

Conversation

practicalswift commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

gmaxwell commented Oct 29, 2019

Uh oh!

practicalswift commented Oct 29, 2019

Uh oh!

practicalswift commented Nov 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Dec 29, 2019

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 20, 2020

Uh oh!

jonatack commented Feb 20, 2020

Uh oh!

practicalswift commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elichai commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Feb 23, 2020

Uh oh!

jonatack commented Feb 23, 2020

Uh oh!

elichai commented Feb 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Feb 23, 2020

Uh oh!

elichai commented Feb 24, 2020

Uh oh!

elichai commented Feb 24, 2020

Uh oh!

practicalswift commented Mar 7, 2020

Uh oh!

practicalswift commented Mar 10, 2020

Uh oh!

Uh oh!

practicalswift commented Oct 21, 2019 •

edited

Loading

DrahtBot commented Oct 21, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading

practicalswift commented Feb 23, 2020 •

edited

Loading

elichai commented Feb 23, 2020 •

edited

Loading

elichai commented Feb 23, 2020 •

edited

Loading