Make all tests pass UBSan without using any UBSan suppressions

practicalswift commented at 5:17 pm on October 21, 2019: contributor

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 c8c393b149f975f13e3ac7e4be17196d7c482b69 needs to be cherry-picked in from PR #17156 to get rid of the two alignment related suppressions :)

Before this PR:

 0$ ./configure --with-sanitizers=undefined
 1$ make
 2$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" src/test/test_bitcoin
 3…
 4validation.cpp:2563:164: runtime error: division by zero
 5validation.cpp:2568:138: runtime error: division by zero
 6validation.cpp:2573:161: runtime error: division by zero
 7validation.cpp:2582:164: runtime error: division by zero
 8validation.cpp:2583:144: runtime error: division by zero
 9policy/fees.cpp:347:44: runtime error: division by zero
10policy/fees.cpp:344:44: runtime error: division by zero
11wallet/wallet.cpp:2049:67: runtime error: division by zero
12wallet/wallet.cpp:3280:51: runtime error: division by zero
13wallet/wallet.cpp:3283:51: runtime error: division by zero
14…
15$ echo $?
161
17$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" test/functional/test_runner.py
18…
19AssertionError: Unexpected stderr validation.cpp:2563:164: runtime error: division by zero
20SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:2563:164 in
21validation.cpp:2568:138: runtime error: division by zero
22SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:2568:138 in
23validation.cpp:2573:161: runtime error: division by zero
24SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:2573:161 in
25validation.cpp:2582:164: runtime error: division by zero
26SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:2582:164 in
27validation.cpp:2583:144: runtime error: division by zero
28SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior validation.cpp:2583:144 in !=
29…
30$ echo $?
311

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

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

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:

https://github.com/bitcoin/bitcoin/blob/a22b62481aae95747830bd3c0db3227860b12d8e/src/compat/assumptions.h#L31-L36

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

fanquake added the label Refactoring on Oct 21, 2019

practicalswift force-pushed on Oct 21, 2019

DrahtBot commented at 10:27 pm on October 21, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17954 (wallet: Remove calls to Chain::Lock methods by ryanofsky)
#17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
#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.

MarcoFalke referenced this in commit 9dd6bbba61 on Oct 22, 2019

DrahtBot added the label Needs rebase on Oct 22, 2019

practicalswift force-pushed on Oct 22, 2019

DrahtBot removed the label Needs rebase on Oct 22, 2019

gmaxwell commented at 9:39 pm on October 29, 2019: contributor

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 commented at 11:39 pm on October 29, 2019: contributor

@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.)

0-static int64_t nBlocksTotal = 0;
1+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 :)

 0$ cling
 1[cling]$ #include <iostream>
 2[cling]$ void f(double d) {
 3[cling]$     if (d == 0.0) {
 4[cling]$     } else if (d < 0.0) {
 5[cling]$     } else if (d > 0.0) {
 6[cling]$     } else {
 7[cling]$         std::cout << "reachable.\n";
 8[cling]$     }
 9[cling]$ }
10[cling]$ f(0.0/0.0)
11reachable.
12[cling]$ .q
13$

:)

practicalswift force-pushed on Oct 29, 2019

practicalswift commented at 9:18 am on November 14, 2019: contributor

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

in src/wallet/wallet.cpp:2937 in ed3e044157 outdated

2836               nFeeRet, nBytes, nFeeNeeded, feeCalc.returnedTarget, feeCalc.desiredTarget, StringForFeeReason(feeCalc.reason), feeCalc.est.decay,
2837               feeCalc.est.pass.start, feeCalc.est.pass.end,
2838-              100 * feeCalc.est.pass.withinTarget / (feeCalc.est.pass.totalConfirmed + feeCalc.est.pass.inMempool + feeCalc.est.pass.leftMempool),
2839               feeCalc.est.pass.withinTarget, feeCalc.est.pass.totalConfirmed, feeCalc.est.pass.inMempool, feeCalc.est.pass.leftMempool,
2840               feeCalc.est.fail.start, feeCalc.est.fail.end,
2841-              100 * feeCalc.est.fail.withinTarget / (feeCalc.est.fail.totalConfirmed + feeCalc.est.fail.inMempool + feeCalc.est.fail.leftMempool),

Empact commented at 9:35 am on November 14, 2019:

nit: I’d rather keep the log message the same and add special handling for the divide by zero case, under the theory that avoiding an edge case is not a great reason to reduce communication

practicalswift commented at 9:46 am on November 14, 2019:

That’s actually how I did it in the original version, but I changed it after receiving this feedback from @gmaxwell:

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.

I think he makes a good point here.

Empact commented at 10:16 am on November 14, 2019:

How about this? Seems pretty clean / improves readability IMO: https://github.com/bitcoin/bitcoin/compare/master...Empact:percent-within-target

practicalswift commented at 10:27 am on November 14, 2019:

I guess it is a matter of preference, but I like the current simpler solution suggested by @gmaxwell.

I’ll let others chime in and adjust to the consensus opinion (if any) :)

Empact commented at 10:43 am on November 14, 2019:

I don’t read @gmaxwell’s comment as suggesting this approach - “don’t print if there is nothing to report” is different from removing the lines.

practicalswift commented at 10:46 am on November 14, 2019:

I meant “don’t divide numbers just give the raw numbers” which is what is done in the suggested version :)

Empact commented at 10:51 am on November 14, 2019:

Touché

in src/wallet/wallet.cpp:1661 in ed3e044157 outdated

1585@@ -1586,7 +1586,11 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1586     }
1587     double progress_current = progress_begin;
1588     while (block_height && !fAbortRescan && !chain().shutdownRequested()) {
1589-        m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin);
1590+        if (progress_end - progress_begin > 0.0) {

Empact commented at 9:38 am on November 14, 2019:

nit: progress_end > progress_begin

practicalswift commented at 9:42 am on November 14, 2019:

Thanks a lot for reviewing!

The added …

0if (progress_end - progress_begin > 0.0) {

… is meant to be “visually consistent” with the already existing L1590 …

0if (*block_height % 100 == 0 && progress_end - progress_begin > 0.0) {

Does that make sense? :)

Empact commented at 9:54 am on November 14, 2019:

Yeah, it’s a subjective point, but clearer IMO.

in src/validation.cpp:1891 in ed3e044157 outdated

1890@@ -1891,7 +1891,7 @@ static int64_t nTimeConnect = 0;
1891 static int64_t nTimeIndex = 0;
1892 static int64_t nTimeCallbacks = 0;
1893 static int64_t nTimeTotal = 0;
1894-static int64_t nBlocksTotal = 0;
1895+static int64_t nBlocksTotal = 1;

Empact commented at 9:45 am on November 14, 2019:

nit: How about instead introducing a method that returns e.g. the PerBlockTime(nTimeChainState), and handles the 0 case? More lines but the logged numbers remain accurate.

practicalswift commented at 9:50 am on November 14, 2019:

In a previous version solved this using a slightly larger change, but I received the following feedback from @gmaxwell:

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.

I think that is a good point. Since this is validation.cpp I’d like to keep the changes to the very minimum :)

Empact commented at 10:40 am on November 14, 2019:

I don’t totally buy this as removing the ubsan exception helps protect against regression / div-by-zero. Granted it’s more lines and so more review in theory, but only marginally so as you have to look over all the nBlocksTotal uses anyway to be clear about the implications, and in both cases you can see only log lines are affected, etc. But yes this is another subjective point. https://github.com/bitcoin/bitcoin/compare/master...Empact:mills-per-block

in src/wallet/wallet.cpp:1592 in ed3e044157 outdated

1585@@ -1586,7 +1586,11 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1586     }
1587     double progress_current = progress_begin;
1588     while (block_height && !fAbortRescan && !chain().shutdownRequested()) {
1589-        m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin);
1590+        if (progress_end - progress_begin > 0.0) {
1591+            m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin);
1592+        } else {
1593+            m_scanning_progress = 0.0;

MarcoFalke commented at 1:43 pm on November 14, 2019:

This is initialized to 0 already

practicalswift commented at 2:53 pm on November 14, 2019:

Good point. Fixed!

practicalswift force-pushed on Nov 14, 2019

practicalswift commented at 3:51 pm on December 29, 2019: contributor

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

practicalswift requested review from ryanofsky on Jan 14, 2020

in src/wallet/wallet.cpp:1664 in 9724a0c2ec outdated

1585@@ -1586,7 +1586,9 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1586     }
1587     double progress_current = progress_begin;
1588     while (block_height && !fAbortRescan && !chain().shutdownRequested()) {
1589-        m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin);
1590+        if (progress_end - progress_begin > 0.0) {
1591+            m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin);
1592+        }
1593         if (*block_height % 100 == 0 && progress_end - progress_begin > 0.0) {

jonatack commented at 9:35 am on February 4, 2020:

It seems to me there is no need to check twice that progress_end - progress_begin is greater than zero, even if the compiler is clever enough to optimise it. Perhaps:

0-        m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin);
1-        if (*block_height % 100 == 0 && progress_end - progress_begin > 0.0) {
2-            ShowProgress(strprintf("%s " + _("Rescanning...").translated, GetDisplayName()), std::max(1, std::min(99, (int)(m_scanning_progress * 100))));
3+        if (progress_end > progress_begin) {
4+            m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin);
5+            if (*block_height % 100 == 0) {
6+                ShowProgress(strprintf("%s " + _("Rescanning...").translated, GetDisplayName()), std::max(1, std::min(99, (int)(m_scanning_progress * 100))));
7+            }

or

0+        double blocks_to_scan = (progress_end - progress_begin);
1+        if (blocks_to_scan > 0.0) {
2+            m_scanning_progress = (progress_current - progress_begin) / blocks_to_scan;
3+            if (*block_height % 100 == 0) {
4+                ShowProgress(strprintf("%s " + _("Rescanning...").translated, GetDisplayName()), std::max(1, std::min(99, (int)(m_scanning_progress * 100))));
5+            }

practicalswift commented at 3:30 pm on February 10, 2020:

I wanted to keep the diff small by touch as few lines as possible to make review trivial :)

jonatack commented at 9:38 am on February 4, 2020: member

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

jonatack commented at 10:34 pm on February 10, 2020: member

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.

DrahtBot added the label Needs rebase on Feb 12, 2020

Make all tests pass UBSan without using any UBSan suppressions 0201cbcd86

practicalswift force-pushed on Feb 13, 2020

practicalswift commented at 11:08 am on February 13, 2020: contributor

Rebased - please re-review :)

DrahtBot removed the label Needs rebase on Feb 13, 2020

jonatack commented at 8:27 pm on February 13, 2020: member

ACK 0201cbc reviewed code, built with sanitizers=undefined and -Weverything, ran unit and functional tests and bitcoind.

 0$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" src/test/test_bitcoin
 1Running 398 test cases...
 2Using configuration: seed=298630376733616266
 3*** No errors detected
 4
 5$ echo $?
 60
 7
 8$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" test/functional/test_runner.py
 9ALL                                              | ✓ Passed  | 4229 s (accumulated) 
10Runtime: 1120 s
11
12$ echo $?
130

MarcoFalke commented at 7:27 pm on February 20, 2020: member

I’d like to see this PR and #17708 merged to be able to build this way for testing PRs locally.

It is already possible to build with the undefined sanitizer enabled. This changeset does not fix a bug and if the primary motivation for the change is to get rid of the suppressions, I’d say it is low priority refactoring that can be done when the code needs to be touched for other reasons some time later.

practicalswift closed this on Feb 20, 2020

jonatack commented at 7:50 pm on February 20, 2020: member

Upgraded my comment to “facilitate and encourage testing with UBSan by default as a general review practice.”

practicalswift commented at 7:53 pm on February 20, 2020: contributor

Totally agree with @jonatack FWIW. We need more sanitizer testing – I really cannot see any reason why developers would to not build with ASan and UBSan by default when developing.

jonatack commented at 7:54 pm on February 20, 2020: member

@practicalswift Please don’t close this. It’s worthwhile to improve our testing.

practicalswift commented at 7:56 pm on February 20, 2020: contributor

@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 commented at 8:04 pm on February 20, 2020: member

@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 reopened this on Feb 23, 2020

practicalswift commented at 10:06 am on February 23, 2020: contributor

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 commented at 4:03 pm on February 23, 2020: contributor

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

0$ make clean
1$ ./configure --with-sanitizers=undefined --with-incompatible-bdb
2$ make
3$ ./src/test/test_bitcoin
4Running 396 test cases...
5
6*** No errors detected

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

jonatack commented at 4:10 pm on February 23, 2020: member

@elichai try make distclean?

jonatack commented at 4:15 pm on February 23, 2020: member

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

elichai commented at 4:28 pm on February 23, 2020: contributor

@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)

 0Options used to compile and link:                                                                                                                                                                                                                                               
 1  with wallet   = yes                                                                                                                   
 2  with gui / qt = yes                                      
 3    with qr     = yes                                                                                                                   
 4  with zmq      = yes                                                                                                                   
 5  with test     = yes                                                                                                                   
 6    with prop   = no                                                                                                                    
 7    with fuzz   = no                                
 8  with bench    = yes                           
 9  with upnp     = yes                            
10  use asm       = yes                                          
11  sanitizers    = undefined                                                                                                             
12  debug enabled = no                                                                                                                                                                                                                                                            
13  gprof enabled = no                                                                                                                    
14  werror        = no                                                                                                                    
15                                                                                                                                        
16  target os     = linux                                    
17  build os      =                                                                                                                                                                                                                                                               
18
19  CC            = /usr/bin/ccache gcc
20  CFLAGS        = -g -O2
21  CPPFLAGS      =   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS
22  CXX           = /usr/bin/ccache g++ -std=c++11
23  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
24  LDFLAGS       = -pthread  -Wl,-z,relro -Wl,-z,now -pie  
25  ARFLAGS       = cr

practicalswift commented at 8:39 pm on February 23, 2020: contributor

@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 commented at 10:08 am on February 24, 2020: contributor

@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 commented at 1:44 pm on February 24, 2020: contributor

Finally. Before:

0./configure --with-sanitizers=undefined,float-divide-by-zero --with-incompatible-bdb
1make
2./src/test/test_bitcoin                          
3Running 396 test cases...                                                                                
4 ==                                                                                                      
5wallet/wallet.cpp:1669:67: runtime error: division by zero                                               
6wallet/wallet.cpp:2945:51: runtime error: division by zero                                                                                                                                                         
7wallet/wallet.cpp:2942:51: runtime error: division by zero                                               
8                                                                                                         
9*** No errors detected

After:

0./configure --with-sanitizers=undefined,float-divide-by-zero --with-incompatible-bdb
1make
2./src/test/test_bitcoin                          
3Running 396 test cases...
4
5*** No errors detected

practicalswift commented at 1:12 pm on March 7, 2020: contributor

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

Will close if no interest shown :)

practicalswift commented at 8:12 am on March 10, 2020: contributor

Closing due to lack of interest :)

practicalswift closed this on Mar 10, 2020

practicalswift deleted the branch on Apr 10, 2021

PastaPastaPasta referenced this in commit d045e206c0 on Sep 11, 2021

PastaPastaPasta referenced this in commit 7d94eabc20 on Sep 11, 2021

PastaPastaPasta referenced this in commit 93b356596f on Sep 12, 2021

PastaPastaPasta referenced this in commit acdebbb238 on Sep 12, 2021

PastaPastaPasta referenced this in commit 877c436049 on Sep 12, 2021

PastaPastaPasta referenced this in commit 0bfae99265 on Sep 14, 2021

PastaPastaPasta referenced this in commit ac130b6c03 on Sep 14, 2021

Munkybooty referenced this in commit 4eec04f615 on Apr 29, 2022

Munkybooty referenced this in commit cf581a76ca on May 12, 2022

Munkybooty referenced this in commit 13ea42cd67 on May 12, 2022

Munkybooty referenced this in commit abee458dee on May 17, 2022

DrahtBot locked this on Aug 18, 2022

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

Conflicts