Add configure options for various -fsanitize flags #12692

eklitzke · 2018-03-15T03:13:34Z

This adds configure options for -fsanitize=address, -fsanitize=thread, and -fsanitize=undefined which are all disabled by default. These flags are useful for developers who wish to do additional safety checking. Note that some of these are mutually incompatible, and these may have a large performance overhead.

There's some kind of strange logic required to properly check for the availability of these flags in a way that works on both GCC and Clang, hopefully the comments make it clear what's going on.

practicalswift · 2018-03-15T10:06:02Z

Concept ACK

Very nice!

eklitzke · 2018-03-15T13:08:08Z

The code actually looks good with -fsanitize=undefined and -fsanitize=thread, the test suite passes cleanly and my bitcoind seems fine with those options.

Things are horribly broken if you try to use -fsanitize=address. If anyone wants to play with that option I recommend you use --disable-asm, otherwise you get segfaults in sha256_sse4::Transform (which I think is a bug in address sanitizer) and you can't get very far without that.

laanwj · 2018-03-15T13:33:48Z

I don't think we should add configure options for every possible sanitation flag. This might confuse people that are trying to build the software with a flurry of options. It looks like the beginning of trying to wrap all compiler options. Which is unnecessary as they can already be passed through ./configure CXXFLAGS="-fsanitize=magnetic-flux".

What about a single --enable-sanitize=a,b,c option where a b and c can be multiple such flags?

practicalswift · 2018-03-15T14:06:20Z

@laanwj But the sanitizers are mutually exclusive, right? I.e. you cannot run ASan and TSan at the same time.

luke-jr · 2018-03-15T14:25:05Z

@practicalswift Some are. Some are not. Different compilers may have different limitations in the future as well.

luke-jr · 2018-03-15T14:27:17Z

Also, it should be noted that the downside of these flags is not limited to performance overhead: they may also introduce security issues. (This is why it's discouraged to build your entire system with them enabled.)

practicalswift · 2018-03-15T14:31:23Z

@luke-jr Yes, good point!

After thinking about it a bit more and in light of @luke-jr:s comment I think @laanwj:s suggested --enable-sanitize=a,b,c (or even better: --enable-sanitizer=a,b,c is the way to go.

practicalswift · 2018-03-15T14:35:42Z

@eklitzke While we're at it – what about allowing for MemorySanitizer (MSan: -fsanitize=memory) too?

laanwj · 2018-03-15T15:02:38Z

@laanwj But the sanitizers are mutually exclusive, right? I.e. you cannot run ASan and TSan at the same time.

That's a subset right? In that case, the option would take only one argument. I was just illustrating how it generalizes to multiple. Mesa has similar option to provide a comma-separated list of drivers to build in, for example.

practicalswift · 2018-03-15T15:05:44Z

@laanwj Yes, you're right! I think --enable-sanitizer=a,b,c is a good idea.

theuni · 2018-03-15T19:40:24Z

configure.ac

+# libtsan, etc. when using the related -fsanitize flags. Clang works
+# differently: it statically links these dependencies. Therefore we can't use
+# AC_CHECK_LIB to test this, we need to compile and link.
+if test "x$enable_asan" = xyes; then


Does AX_CHECK_LINK_FLAG not do what we want here?

That adds the flags to LDFLAGS, so no.

Actually I think there's a trick to combine these (and remove the m4 file I added).

theuni · 2018-03-15T19:42:21Z

configure.ac

+fi
+if test "x$enable_tsan" = xyes; then
+  BITCOIN_CHECK_COMPILE_FLAG_AND_LINK([-fsanitize=thread],
+                                      [CXXFLAGS="$CXXFLAGS -fsanitize=thread"],


As mentioned in the debugging PR, Please add these to a SANITIZE_CXXFLAGS or DEBUG_CXXFLAGS or so, rather than CXXFLAGS directly.

Adding to CXXFLAGS means that future tests will be done with sanitizers enabled, which can cause false-negatives.

eklitzke · 2018-03-16T19:44:24Z

These are good suggestions, I will redo things and put another version of this change.

theuni · 2018-03-16T20:31:12Z

On Mar 16, 2018 4:16 PM, "Evan Klitzke" <notifications@github.com> wrote: *@eklitzke* commented on this pull request. ------------------------------ In configure.ac <#12692 (comment)>:

@@ -247,6 +268,27 @@ if test "x$enable_debug" = xyes; then

fi fi +# Checking for -fsanitize flags works differently from other compiler flags. +# That's because with GCC there's an implicit linking dependency on libasan, +# libtsan, etc. when using the related -fsanitize flags. Clang works +# differently: it statically links these dependencies. Therefore we can't use +# AC_CHECK_LIB to test this, we need to compile and link. +if test "x$enable_asan" = xyes; then Actually I think there's a trick to combine these (and remove the m4 file I added). — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#12692 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZdE82U3dp9IDhfkYnRDPPJRz6B_6hXks5tfB2FgaJpZM4Srfdg> . The variable used shouldn't matter, the contents just end up on the cmdline.

eklitzke · 2018-03-16T20:43:31Z

Updated to add a single sanitizer flag. Should be portable to all POSIX systems including traditional /bin/sh.

I tried a lot of things with the existing AX_* macros and could not get this to work right without the new m4 file I added. For instance, I tried using AX_LINK_IFELSE after AX_CHECK_COMPILE_FLAG, but that uses CXXFLAGS not the new flags I added, and it has no option to take additional compile flags.

That said, there are other options if we don't want the new m4 file:

Could do a hack where we save old CXXFLAGS, update global CXXFLAGS and use AX_LINK_IFELSE, then set SANITIZER_CXXFLAGS to CXXFLAGS and restore the old CXXFLAGS variable
In [build] Make --enable-debug pick better options #12695 I have a commit that adds better reliable detection for clang. If we want to use that we could just change the configure.ac logic to use AX_CHECK_COMPILE_FLAG and then if we are not Clang also do an AC_CHECK_LIB check.

Let's see how #12695 pans out though before continuing with this.

theuni · 2018-03-20T21:15:57Z

configure.ac

+# supports the flag, but in fact linking would fail.
+#
+# Clang works differently: it bundles these libraries, and statically links them
+# when using -fsanitize. For this reason, using AC_CHECK_LIB is not a good idea,


I'm not sure what distinction you're trying to make here. As far as I can tell, clang/gcc handle this the same way. Of course, how distros configure and distribute them is a different story.

The following works fine for me with a gcc with busted libtsan:

AX_CHECK_LINK_FLAG([[-fsanitize=undefined]],[[SANITIZER_CXXFLAGS="${SANITIZER_CXXFLAGS} -fsanitize=undefined"; SANITIZER_LDFLAGS="${SANITIZER_LDFLAGS} -fsanitize=undefined"]], [AC_MSG_ERROR([compiler does not support -fsanitize=undefined])], [[$CXXFLAG_WERROR]]) AX_CHECK_LINK_FLAG([[-fsanitize=thread]],[[SANITIZER_CXXFLAGS="${SANITIZER_CXXFLAGS} -fsanitize=thread"; SANITIZER_LDFLAGS="${SANITIZER_LDFLAGS} -fsanitize=thread"]], [AC_MSG_ERROR([compiler does not support -fsanitize=thread])], [[$CXXFLAG_WERROR]])

The second check (correctly) fails due to a linking error with libtsan:

configure:18545: checking whether the linker accepts -fsanitize=undefined
configure:18564: g++ -m64 -std=c++11 -o conftest -pipe -O2 -I/home/cory/dev/bitcoin2/depends/x86_64-pc-linux-gnu/share/../include/ -L/home/cory/dev/bitcoin2/depends/x86_64-pc-linux-gnu/share/../lib -Werror -fsanitize=undefined conftest.cpp >&5
configure:18564: $? = 0
configure:18574: result: yes
configure:18583: checking whether the linker accepts -fsanitize=thread
configure:18602: g++ -m64 -std=c++11 -o conftest -pipe -O2 -I/home/cory/dev/bitcoin2/depends/x86_64-pc-linux-gnu/share/../include/ -L/home/cory/dev/bitcoin2/depends/x86_64-pc-linux-gnu/share/../lib -Werror -fsanitize=thread conftest.cpp >&5
/tmp/ramfs/tmp/ccFEArS2.o: In function _GLOBAL__sub_I_00099_0_main': conftest.cpp:(.text.startup+0x11): undefined reference to __tsan_init'
collect2: error: ld returned 1 exit status

I don't understand why you find AX_CHECK_LINK_FLAG insufficient, as it simply does a combined compile/link through the compiler, it doesn't invoke the linker directly.

I'll try retesting this.

eklitzke · 2018-03-21T02:20:15Z

You're right, I removed the m4 file I added and made this much simpler. The error messages are also much better now.

Changes:

Since the compiler will accept sanitizer options separated by a comma, I removed the for loop and tr nonsense
First check using AX_CHECK_COMPILE_FLAGS, this ensures that the compiler accepts the flags and the combination is compatible
Next check using AX_CHECK_LINK_FLAGS, if the previous check passed but this fails then the user is missing libraries.

Here are some cases I tested. Note that this enables me to provide better error messages.

Misspelled option undfeined:

checking whether C++ compiler accepts -fsanitize=undfeined... no
configure: error: compiler did not accept requested flags

Options address,thread that are mutually incompatible:

checking whether C++ compiler accepts -fsanitize=address,thread... no
configure: error: compiler did not accept requested flags

Options address,undefined that are compatible, but when I did not have libubsan installed:

checking whether C++ compiler accepts -fsanitize=address,undefined... yes
checking whether the linker accepts -fsanitize=address,undefined... no
configure: error: linker did not accept requested flags, you are missing required libraries

And for completeness, when everything is installed and valid:

checking whether C++ compiler accepts -fsanitize=address,undefined... yes
checking whether the linker accepts -fsanitize=address,undefined... yes

I was able to run the build all the way through including linking with the existing change to src/Makefile.am that just adds the flags to AM_CXXFLAGS, no need to update AM_LDFLAGS.

ryanofsky

utACK 257cb24

It may be a good idea to say something about the new option in developer notes, for example to mention the -fsanitize=address / --disable-asm combination.

eklitzke · 2018-03-22T17:25:21Z

Docs added (including the note about --disable-asm) and re-squashed.

theuni · 2018-03-22T17:48:43Z

configure.ac

@@ -219,6 +219,12 @@ AC_ARG_ENABLE([debug],
    [enable_debug=$enableval],
    [enable_debug=no])

+# Enable different -fsanitize options
+AC_ARG_ENABLE([sanitizer],


Mind changing this to AC_ARG_WITH? In autoconf-speak, --enable-foo is intended to be binary (which is why it enables --disable-foo too), and --with-bar is intended to take options.

theuni · 2018-03-22T17:55:59Z

configure.ac

+  # supported but libraries are missing.
+  AX_CHECK_LINK_FLAG(
+    [[-fsanitize=$enable_sanitizer]],
+    [[SANITIZER_CXXFLAGS=-fsanitize=$enable_sanitizer]],


Could you add these to a SANITIZER_LDFLAGS as well, and add it to AM_LDFLAGS? I'm nervous that automake/libtool may not always add (all) CXXFLAGS to the link-line. In fact, they really shouldn't be doing that anyway.

theuni · 2018-03-22T18:37:25Z

doc/developer-notes.md

+undefined sanitizer. If you are missing required libraries, the configure script
+will fail with a linker error when testing the sanitizer flags.
+
+The test suite should pass cleanly with the `thread` and `undefined` sanitizers,


Why not notate the incompatible functions with a function attribute (no_sanitize("address")) instead? Of course we'd need to do an autoconf check for that first.

We could, but there seem to be a lot of them so I don't have a full list of what needs to be disabled yet.

eklitzke · 2018-03-27T01:04:47Z

Updated to use the --with form and re-squashed.

theuni

utACK, thanks.

Nit: --with-sanitizers (rather than --with-sanitizer) would make it more obvious that a list is accepted. Feel free to change if you agree, but it's not worth holding up merge.

I do think that we should notate known false-positives, even if there are lots of them. If nothing else, it would force us to be able to explain why the false-positives are produced in the first place, and help file good upstream bugs for the sanitizers themselves.

It makes sense to add the annotations in another PR, though.

Note that --disable-asm apparently doesn't disable leveldb's crc32 intrinsics as it arguably should. Also, the --enable-asm is passed down to libsecp256k1 which only knows about --with-asm, so assembly ends up being used there as well. I'll PR fixes for those.

So, if --disable-asm is enough to get it working, as I see it that must mean it's sha256_sse4 causing the issues.

This enables the use of different compiler sanitizers, coresponding to the -fsanitize option in GCC and Clang.

laanwj · 2018-03-29T20:57:37Z

utACK 6feb46c

6feb46c Add --with-sanitizers option to configure (Evan Klitzke) Pull request description: This adds configure options for `-fsanitize=address`, `-fsanitize=thread`, and `-fsanitize=undefined` which are all disabled by default. These flags are useful for developers who wish to do additional safety checking. Note that some of these are mutually incompatible, and these may have a large performance overhead. There's some kind of strange logic required to properly check for the availability of these flags in a way that works on both GCC and Clang, hopefully the comments make it clear what's going on. Tree-SHA512: 2d6fe402799110e59ee452dddf37f7ca2d26a7fecec50be25c8a134e4a20beb31f1e8f438dffd443641562418075896d1eeb450623425b272d80e05e3027a587

@eklitzke

01189ab build: Show enabled sanitizers in configure output (practicalswift) Pull request description: Show enabled sanitizers in `configure` output. Context: @eklitzke excellent addition of `--with-sanitizers` in #12692 Tree-SHA512: b2d52308e3476488fe47cbc059d7f3235aaeefaa2b987003923f6eaacbadf67f0cf22a32a04873d0f54c1867757841e01c8053f86a4d2f59a407b960ac15105f

./configure updates Includes code cherry-picked from the following upstream Bitcoin Core PRs: - bitcoin/bitcoin#6748 - bitcoin/bitcoin#12373 - bitcoin/bitcoin#12692 - bitcoin/bitcoin#12901 - bitcoin/bitcoin#13005 - bitcoin/bitcoin#13445 - bitcoin/bitcoin#12686 - bitcoin/bitcoin#16435 Part of #2074.

6feb46c Add --with-sanitizers option to configure (Evan Klitzke) Pull request description: This adds configure options for `-fsanitize=address`, `-fsanitize=thread`, and `-fsanitize=undefined` which are all disabled by default. These flags are useful for developers who wish to do additional safety checking. Note that some of these are mutually incompatible, and these may have a large performance overhead. There's some kind of strange logic required to properly check for the availability of these flags in a way that works on both GCC and Clang, hopefully the comments make it clear what's going on. Tree-SHA512: 2d6fe402799110e59ee452dddf37f7ca2d26a7fecec50be25c8a134e4a20beb31f1e8f438dffd443641562418075896d1eeb450623425b272d80e05e3027a587

@eklitzke

01189ab build: Show enabled sanitizers in configure output (practicalswift) Pull request description: Show enabled sanitizers in `configure` output. Context: @eklitzke excellent addition of `--with-sanitizers` in bitcoin#12692 Tree-SHA512: b2d52308e3476488fe47cbc059d7f3235aaeefaa2b987003923f6eaacbadf67f0cf22a32a04873d0f54c1867757841e01c8053f86a4d2f59a407b960ac15105f

6feb46c Add --with-sanitizers option to configure (Evan Klitzke) Pull request description: This adds configure options for `-fsanitize=address`, `-fsanitize=thread`, and `-fsanitize=undefined` which are all disabled by default. These flags are useful for developers who wish to do additional safety checking. Note that some of these are mutually incompatible, and these may have a large performance overhead. There's some kind of strange logic required to properly check for the availability of these flags in a way that works on both GCC and Clang, hopefully the comments make it clear what's going on. Tree-SHA512: 2d6fe402799110e59ee452dddf37f7ca2d26a7fecec50be25c8a134e4a20beb31f1e8f438dffd443641562418075896d1eeb450623425b272d80e05e3027a587

@eklitzke

01189ab build: Show enabled sanitizers in configure output (practicalswift) Pull request description: Show enabled sanitizers in `configure` output. Context: @eklitzke excellent addition of `--with-sanitizers` in bitcoin#12692 Tree-SHA512: b2d52308e3476488fe47cbc059d7f3235aaeefaa2b987003923f6eaacbadf67f0cf22a32a04873d0f54c1867757841e01c8053f86a4d2f59a407b960ac15105f

fanquake added the Build system label Mar 15, 2018

fanquake requested a review from theuni March 15, 2018 03:32

randolf approved these changes Mar 15, 2018

View reviewed changes

eklitzke force-pushed the sanitize branch 2 times, most recently from df37ecb to 78618b8 Compare March 15, 2018 06:23

eklitzke mentioned this pull request Mar 15, 2018

Add -ftrapv to CFLAGS and CXXFLAGS when --enable-debug is used. Enable -ftrapv in Travis. #12686

Merged

laanwj assigned theuni Mar 15, 2018

theuni reviewed Mar 15, 2018

View reviewed changes

eklitzke mentioned this pull request Mar 18, 2018

gui, wallet: random abort (segmentation fault) running master/HEAD #9683

Closed

theuni reviewed Mar 20, 2018

View reviewed changes

eklitzke force-pushed the sanitize branch 2 times, most recently from 7c9eb31 to 257cb24 Compare March 21, 2018 02:38

ryanofsky reviewed Mar 22, 2018

View reviewed changes

eklitzke force-pushed the sanitize branch from 257cb24 to 7a08131 Compare March 22, 2018 17:24

eklitzke force-pushed the sanitize branch from 7a08131 to e6b7d55 Compare March 22, 2018 17:26

eklitzke mentioned this pull request Mar 22, 2018

Enable -fsanitize flags in Travis #12691

Closed

theuni reviewed Mar 22, 2018

View reviewed changes

eklitzke force-pushed the sanitize branch from e6b7d55 to 0fa7007 Compare March 27, 2018 01:04

eklitzke force-pushed the sanitize branch 4 times, most recently from 1851744 to 21f01aa Compare March 27, 2018 03:05

theuni approved these changes Mar 28, 2018

View reviewed changes

eklitzke force-pushed the sanitize branch from 21f01aa to 9304788 Compare March 28, 2018 19:28

Add --with-sanitizers option to configure

6feb46c

This enables the use of different compiler sanitizers, coresponding to the -fsanitize option in GCC and Clang.

eklitzke force-pushed the sanitize branch from 9304788 to 6feb46c Compare March 28, 2018 19:33

laanwj merged commit 6feb46c into bitcoin:master Mar 29, 2018

practicalswift mentioned this pull request Apr 6, 2018

build: Show enabled sanitizers in configure output #12901

Merged

str4d mentioned this pull request Nov 12, 2019

./configure updates zcash/zcash#4222

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add configure options for various -fsanitize flags #12692

Add configure options for various -fsanitize flags #12692

Uh oh!

Conversation

eklitzke commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Mar 15, 2018

Uh oh!

eklitzke commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 15, 2018

Uh oh!

practicalswift commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Mar 15, 2018

Uh oh!

luke-jr commented Mar 15, 2018

Uh oh!

practicalswift commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Mar 15, 2018

Uh oh!

laanwj commented Mar 15, 2018

Uh oh!

practicalswift commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eklitzke commented Mar 16, 2018

Uh oh!

theuni commented Mar 16, 2018 via email

Uh oh!

eklitzke commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eklitzke commented Mar 21, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

eklitzke commented Mar 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eklitzke commented Mar 27, 2018

Uh oh!

theuni left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 29, 2018

Uh oh!

Uh oh!

eklitzke commented Mar 15, 2018 •

edited

Loading

eklitzke commented Mar 15, 2018 •

edited

Loading

practicalswift commented Mar 15, 2018 •

edited

Loading

practicalswift commented Mar 15, 2018 •

edited

Loading