Add configure options for various -fsanitize flags #12692

pull eklitzke wants to merge 1 commits into bitcoin:master from eklitzke:sanitize changing 3 files +81 −2
  1. eklitzke commented at 3:13 am on March 15, 2018: contributor

    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.

  2. fanquake added the label Build system on Mar 15, 2018
  3. fanquake requested review from theuni on Mar 15, 2018
  4. randolf approved
  5. eklitzke force-pushed on Mar 15, 2018
  6. eklitzke force-pushed on Mar 15, 2018
  7. practicalswift commented at 10:06 am on March 15, 2018: contributor

    Concept ACK

    Very nice!

  8. eklitzke commented at 1:08 pm on March 15, 2018: contributor

    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.

  9. laanwj commented at 1:33 pm on March 15, 2018: member

    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?

  10. laanwj assigned theuni on Mar 15, 2018
  11. practicalswift commented at 2:06 pm on March 15, 2018: contributor
    @laanwj But the sanitizers are mutually exclusive, right? I.e. you cannot run ASan and TSan at the same time.
  12. luke-jr commented at 2:25 pm on March 15, 2018: member
    @practicalswift Some are. Some are not. Different compilers may have different limitations in the future as well.
  13. luke-jr commented at 2:27 pm on March 15, 2018: member
    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.)
  14. practicalswift commented at 2:31 pm on March 15, 2018: contributor

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

  15. practicalswift commented at 2:35 pm on March 15, 2018: contributor
    @eklitzke While we’re at it – what about allowing for MemorySanitizer (MSan: -fsanitize=memory) too?
  16. laanwj commented at 3:02 pm on March 15, 2018: member

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

  17. practicalswift commented at 3:05 pm on March 15, 2018: contributor
    @laanwj Yes, you’re right! I think --enable-sanitizer=a,b,c is a good idea.
  18. in configure.ac:276 in 78618b8b03 outdated 
    267@@ -247,6 +268,27 @@ if test "x$enable_debug" = xyes; then
268     fi
269 fi
270 
271+# Checking for -fsanitize flags works differently from other compiler flags.
272+# That's because with GCC there's an implicit linking dependency on libasan,
273+# libtsan, etc. when using the related -fsanitize flags. Clang works
274+# differently: it statically links these dependencies. Therefore we can't use
275+# AC_CHECK_LIB to test this, we need to compile and link.
276+if test "x$enable_asan" = xyes; then
    theuni commented at 7:40 pm on March 15, 2018:
    Does AX_CHECK_LINK_FLAG not do what we want here?
    eklitzke commented at 7:43 pm on March 16, 2018:
    That adds the flags to LDFLAGS, so no.
    eklitzke commented at 8:15 pm on March 16, 2018:
    Actually I think there’s a trick to combine these (and remove the m4 file I added).
  19. in configure.ac:283 in 78618b8b03 outdated 
    278+                                      [CXXFLAGS="$CXXFLAGS -fsanitize=address"],
279+                                      [AC_MSG_ERROR([-fsanitize=asan broken, check if you have libasan installed])])
280+fi
281+if test "x$enable_tsan" = xyes; then
282+  BITCOIN_CHECK_COMPILE_FLAG_AND_LINK([-fsanitize=thread],
283+                                      [CXXFLAGS="$CXXFLAGS -fsanitize=thread"],
    theuni commented at 7:42 pm on March 15, 2018:

    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 commented at 7:43 pm on March 16, 2018:
    will fix
  20. eklitzke commented at 7:44 pm on March 16, 2018: contributor
    These are good suggestions, I will redo things and put another version of this change.
  21. theuni commented at 8:31 pm on March 16, 2018: member

    On Mar 16, 2018 4:16 PM, “Evan Klitzke” notifications@github.com wrote:

    @eklitzke commented on this pull request.

    In configure.ac https://github.com/bitcoin/bitcoin/pull/12692#discussion_r175203308:

    @@ -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 https://github.com/bitcoin/bitcoin/pull/12692#discussion_r175203308, 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.

  22. eklitzke commented at 8:43 pm on March 16, 2018: contributor

    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 #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.

  23. in configure.ac:267 in e9adc6594d outdated 
    262+# for -fsanitize=address). This means that AX_CHECK_COMPILE_FLAG does not work
263+# properly with GCC, as the AX_CHECK_COMPILE_FLAG check here would pass if GCC
264+# supports the flag, but in fact linking would fail.
265+#
266+# Clang works differently: it bundles these libraries, and statically links them
267+# when using -fsanitize. For this reason, using AC_CHECK_LIB is not a good idea,
    theuni commented at 9:15 pm on March 20, 2018:

    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:

    0AX_CHECK_LINK_FLAG([[-fsanitize=undefined]],[[SANITIZER_CXXFLAGS="${SANITIZER_CXXFLAGS} -fsanitize=undefined"; SANITIZER_LDFLAGS="${SANITIZER_LDFLAGS} -fsanitize=undefined"]],
1[AC_MSG_ERROR([compiler does not support -fsanitize=undefined])], [[$CXXFLAG_WERROR]])
2
3AX_CHECK_LINK_FLAG([[-fsanitize=thread]],[[SANITIZER_CXXFLAGS="${SANITIZER_CXXFLAGS} -fsanitize=thread"; SANITIZER_LDFLAGS="${SANITIZER_LDFLAGS} -fsanitize=thread"]],
4[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.

    eklitzke commented at 1:28 am on March 21, 2018:
    I’ll try retesting this.
  24. eklitzke commented at 2:20 am on March 21, 2018: contributor

    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:

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

    Options address,thread that are mutually incompatible:

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

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

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

    And for completeness, when everything is installed and valid:

    0checking whether C++ compiler accepts -fsanitize=address,undefined... yes
1checking 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.

  25. eklitzke force-pushed on Mar 21, 2018
  26. eklitzke force-pushed on Mar 21, 2018
  27. ryanofsky commented at 1:27 pm on March 22, 2018: member

    utACK 257cb24bfdc0f5455f399816784bc5da857eca16

    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.

  28. eklitzke force-pushed on Mar 22, 2018
  29. eklitzke commented at 5:25 pm on March 22, 2018: contributor
    Docs added (including the note about --disable-asm) and re-squashed.
  30. eklitzke force-pushed on Mar 22, 2018
  31. in configure.ac:223 in e6b7d55517 outdated 
    218@@ -219,6 +219,12 @@ AC_ARG_ENABLE([debug],
219     [enable_debug=$enableval],
220     [enable_debug=no])
221 
222+# Enable different -fsanitize options
223+AC_ARG_ENABLE([sanitizer],
    theuni commented at 5:48 pm on March 22, 2018:
    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.
    eklitzke commented at 1:02 am on March 27, 2018:
    done
  32. in configure.ac:269 in e6b7d55517 outdated 
    264+  # libtsan, libubsan, etc. Make sure linking still works with these flags. This
265+  # is a separate check so we can give a better error message when the flags are
266+  # supported but libraries are missing.
267+  AX_CHECK_LINK_FLAG(
268+    [[-fsanitize=$enable_sanitizer]],
269+    [[SANITIZER_CXXFLAGS=-fsanitize=$enable_sanitizer]],
    theuni commented at 5:55 pm on March 22, 2018:
    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.
    eklitzke commented at 1:02 am on March 27, 2018:
    done
  33. in doc/developer-notes.md:233 in e6b7d55517 outdated 
    228+"san" libraries to actually compile with these flags, e.g. libasan for the
229+address sanitizer, libtsan for the thread sanitizer, and libubsan for the
230+undefined sanitizer. If you are missing required libraries, the configure script
231+will fail with a linker error when testing the sanitizer flags.
232+
233+The test suite should pass cleanly with the `thread` and `undefined` sanitizers,
    theuni commented at 6:37 pm on March 22, 2018:
    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.
    eklitzke commented at 1:02 am on March 27, 2018:
    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.
  34. eklitzke force-pushed on Mar 27, 2018
  35. eklitzke commented at 1:04 am on March 27, 2018: contributor
    Updated to use the –with form and re-squashed.
  36. eklitzke force-pushed on Mar 27, 2018
  37. eklitzke force-pushed on Mar 27, 2018
  38. eklitzke force-pushed on Mar 27, 2018
  39. eklitzke force-pushed on Mar 27, 2018
  40. theuni approved
  41. theuni commented at 5:55 pm on March 28, 2018: member

    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.

  42. eklitzke force-pushed on Mar 28, 2018
  43. Add --with-sanitizers option to configure 
    This enables the use of different compiler sanitizers, coresponding to
the -fsanitize option in GCC and Clang.
    6feb46c372
  44. eklitzke force-pushed on Mar 28, 2018
  45. laanwj commented at 8:57 pm on March 29, 2018: member
    utACK 6feb46c
  46. laanwj merged this on Mar 29, 2018
  47. laanwj closed this on Mar 29, 2018
  48. laanwj referenced this in commit de6bdfd78f on Mar 29, 2018
  49. MarcoFalke referenced this in commit 7e23972d1f on Apr 8, 2018
  50. zkbot referenced this in commit 8e8a9350c3 on Jan 30, 2020
  51. PastaPastaPasta referenced this in commit 137eb81172 on Sep 27, 2020
  52. PastaPastaPasta referenced this in commit 37b90f6e2d on Sep 27, 2020
  53. PastaPastaPasta referenced this in commit c08855de88 on Oct 14, 2020
  54. PastaPastaPasta referenced this in commit 33588fc167 on Nov 1, 2020
  55. gades referenced this in commit a97a08d095 on Jun 26, 2021
  56. gades referenced this in commit d2a552b0b5 on Jun 28, 2021
  57. MarcoFalke locked this on Sep 8, 2021


eklitzke randolf practicalswift laanwj luke-jr theuni ryanofsky

Labels
Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me