configure.ac: disable FORTIFY_SOURCE for asan/msan/tsan #24763

pull Crypt-iQ wants to merge 1 commits into bitcoin:master from Crypt-iQ:disable_fortify_san changing 1 files +13 −1

Crypt-iQ commented at 4:16 PM on April 4, 2022: contributor

This can cause trouble with these specific sanitizers according to https://github.com/google/sanitizers/issues/247#issuecomment-242238980 . The MSAN CI build already disables hardening, as there can be false positives there due to lack of instrumentation on the fortified functions (e.g. __memcpy_chk), but there's no reason the other hardening checks (stack-smashing protection, and more) can't be enabled if this is the culprit. Also affects ASAN and TSAN, though I do not know how as I only encountered a problem with MSAN. This patch will disable source fortification when --with-sanitizers is used with these sanitizers, but if -fsanitize is used instead, then this patch will be bypassed.
configure.ac: disable FORTIFY_SOURCE for asan/msan/tsan 3353204848
Crypt-iQ force-pushed on Apr 4, 2022
DrahtBot added the label Build system on Apr 4, 2022
in configure.ac:948 in 3353204848
```
 944 |      AX_CHECK_PREPROC_FLAG([-D_FORTIFY_SOURCE=2],[
 945 |        AX_CHECK_PREPROC_FLAG([-U_FORTIFY_SOURCE],[
 946 |          HARDENED_CPPFLAGS="$HARDENED_CPPFLAGS -U_FORTIFY_SOURCE"
 947 |        ])
 948 | -      HARDENED_CPPFLAGS="$HARDENED_CPPFLAGS -D_FORTIFY_SOURCE=2"
 949 | +
```
laanwj commented at 11:33 AM on April 5, 2022:

I'd prefer skipping this detection when the sanitizers are in use, instead of doing this after the detection, which might have been pointless.

Crypt-iQ commented at 2:42 PM on April 5, 2022:

I chose this path since compilers may define it by default and for sanitizers it would have to be explicitly undefined (https://github.com/bitcoin/bitcoin/pull/3242/commits/9b4e03b27b04f6dc687409a13859d59bb7909d8f)

laanwj commented at 9:53 AM on April 6, 2022:

Ok, will leave further review of the build system changes to others.

fanquake commented at 11:14 AM on April 20, 2022:

I chose this path since compilers may define it by default and for sanitizers it would have to be explicitly undefined

I don't see the problem with explicitly undefining FORTIFY_SOURCE in the (limited) cases where we know it's not going to work. In general I don't like the idea of entangling logic for disabling hardening features that break external tools, inside the happy path of having hardening features used by default. It would be nicer if this existed in one of the use_sanitizers blocks.

Crypt-iQ commented at 1:17 PM on April 20, 2022:

I will look into making this change
fanquake commented at 1:08 PM on July 29, 2022: member

@Crypt-iQ are you still working on this?
Crypt-iQ commented at 2:03 PM on July 29, 2022: contributor

@Crypt-iQ are you still working on this?

I don't know the next time I'll be able to work on this - should I close it?
fanquake commented at 2:08 PM on July 29, 2022: member

I don't know the next time I'll be able to work on this - should I close it?

I think you could close it for now. If there's an actual bug you are seeing (it's not clear from the op), that this fixes, you could open an issue with details, and someone might follow up.
Crypt-iQ closed this on Jul 29, 2022
bitcoin locked this on Jul 29, 2023

Contributors

Labels