build: more robustly check for fcf-protection support #20720

fanquake · 2020-12-19T08:14:26Z

When using Clang 7, we may end up trying to use the flag when it won't
work properly, which can lead to confusing errors. i.e:

/usr/bin/ld: error: ... <corrupt x86 feature size: 0x8>

Use AX_CHECK_LINK_FLAG & --fatal-warnings to ensure we wont use the flag in this case.

We do this as even when the error is emitted, compilation succeeds, and the binaries produced will run. This means we can't just check if the compiler accepts the flag, or if compilation succeeds (without or without -Werror, and/or passing -Wl,--fatal-warnings, which may not be passed through to the linker).

This was reported by someone configuring for fuzzing, on Debian 10, where Clang 7 is the default.

See here for a minimal example of the problematic behaviour:
https://gist.github.com/fanquake/9b33555fcfebef8eb8c0795a71732bc6

hebasto · 2020-12-19T09:20:23Z

Concept ACK, the issue is known in other open-source projects: gdnsd/gdnsd@582004e

practicalswift · 2020-12-19T09:28:57Z

cr ACK 01ca4bd: patch looks correct

laanwj · 2020-12-19T10:45:36Z

This makes sense. For me it brings up the question "why aren't we doing this for all hardening flags"?

hebasto

~~ACK 01ca4bd~~ see #20720 (comment)

tested on Debian 10.7 with clang 7.0.1

Tested configuration: ./configure --with-incompatible-bdb CC=clang CXX=clang++

on master (f1dbf92) observing massive messages like these:

/usr/bin/ar: error: libbitcoin_server_a-versionbits.o: <corrupt x86 feature size: 0x8>
...
/usr/bin/ranlib: error: libtest_util.a(libtest_util_a-net.o): <corrupt x86 feature size: 0x8>
...
/usr/bin/ld: error: .libs/libbitcoinconsensus_la-arith_uint256.o: <corrupt x86 feature size: 0x8>

with this PR no errors or warnings during make

tested on Linux Mint 20 with clang 7.0.1

Tested configuration: ./configure --with-incompatible-bdb CC=clang-7 CXX=clang++-7

The same results as for Debian.

tested on Linux Mint 20 with clang 8.0.1 -- no errors even on master.

fanquake · 2020-12-20T08:12:00Z

Thanks for testing @hebasto. However the previous patch wasn't working correctly. The flag would actually NEVER be applied (even where it would work correctly), because the source being compiled wasn't valid 🤦 . I've fixed that now.

For me it brings up the question "why aren't we doing this for all hardening flags"?

I'm currently reviewing all of our flags for any other subtle breakages. It very well might make sense to do this for all flags going forward.

hebasto · 2020-12-20T09:42:49Z

... because the source being compiled wasn't valid

Indeed. I confused AC_LANG_SOURCE and AC_LANG_PROGRAM macros 🤦‍♂️ (btw, the latter seems more concise for this change).

laanwj · 2020-12-20T12:08:32Z

I'm currently reviewing all of our flags for any other subtle breakages. It very well might make sense to do this for all flags going forward.

Thinking about it, it is a bit of a compromise. We wouldn't want to increase the opportunity to silently lose hardening flags. Of course the security checks are the best place to verify this, but we don't have checks for all of the hardening mechanisms (if that is even possible).

When using Clang 7, we may end up trying to use the flag when it won't work properly, which can lead to confusing errors. i.e: ```bash /usr/bin/ld: error: ... <corrupt x86 feature size: 0x8> ``` Use CHECK_LINK_FLAG & --fatal-warnings to ensure we wont use the flag in this case.

fanquake · 2020-12-20T14:00:37Z

I've changed the approach here to check for link errors, as even when the error is emitted, compilation succeeds, and the binaries produced will run. This means we can't just check if the compiler accepts the flag, or if compilation succeeds (without or without -Werror, and/or passing -Wl,--fatal-warnings, which may not be passed through to the linker).

Instead we can do a link check, with --fatal-warnings, and not add -fcf-protection to our HARDENED_CXXFLAGS if we see any errors.

I have only seen this link issue with Clang 7 (when support for -fcf-protection was introduced). Clang 8+ look ok.

pstratem · 2020-12-20T21:26:56Z

Unfortunately still broken on Debian 10.7.

build.log

fanquake · 2020-12-20T22:53:47Z

Unfortunately still broken on Debian 10.7.

Looking at the build.log, the change here is working as expected and isn't adding -fcf-protection to the hardened flags when the linker emits an error:

checking whether the linker accepts -fcf-protection=full... no
....
configure:20769: checking whether the linker accepts -fcf-protection=full
configure:20788: clang++ -std=c++17 -o conftest -g -O2  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS  -Wl,--fatal-warnings -fcf-protection=full conftest.cpp  >&5
/usr/bin/ld: error: /tmp/conftest-afee8c.o: <corrupt x86 feature size: 0x8>
clang: error: linker command failed with exit code 1 (use -v to see invocation)
configure:20788: $? = 1
configure: failed program was:

The build error you're now seeing:

  CXXLD    test/fuzz/fuzz
/usr/bin/ld: test/fuzz/fuzz-addrdb.o: in function `ConsumeNode(FuzzedDataProvider&)':
/home/bitcoin/bitcoin/src/./test/fuzz/util.h:290: multiple definition of `ConsumeNode(FuzzedDataProvider&)'; test/fuzz/fuzz-addition_overflow.o:/home/bitcoin/bitcoin/src/./test/fuzz/util.h:290: first defined here
/usr/bin/ld: test/fuzz/fuzz-addrdb.o: in function `ConsumeSubNet(FuzzedDataProvider&)':
/home/bitcoin/bitcoin/src/./test/fuzz/util.h:275: multiple definition of `ConsumeSubNet(FuzzedDataProvider&)'; test/fuzz/fuzz-addition_overflow.o:/home/bitcoin/bitcoin/src/./test/fuzz/util.h:275: first defined here
/usr/bin/ld: test/fuzz/fuzz-addrdb.o: in function `ConsumeAddress(FuzzedDataProvider&)':
/home/bitcoin/bitcoin/src/./test/fuzz/util.h:285: multiple definition of `ConsumeAddress(FuzzedDataProvider&)'; test/fuzz/fuzz-addition_overflow.o:/home/bitcoin/bitcoin/src/./test/fuzz/util.h:285: first defined here
...

looks more like the one reported here.

pstratem · 2020-12-20T23:14:47Z

I was wrong, you're right, this is another failure described here #20560 (comment)

pstratem · 2020-12-20T23:28:50Z

tested ACK e9189a7

maflcko · 2020-12-21T07:31:41Z

not an ACK e9189a7 , I only tested configure on my system (gcc-10, clang-11):

diff --git a/tmp/a_gcc b/tmp/b_gcc
index c0ef19c9a8..d7e3ac31c2 100644
--- a/tmp/a_gcc
+++ b/tmp/b_gcc
@@ -174,7 +174,7 @@ checking whether C++ compiler accepts -fPIC... yes
 checking whether C++ compiler accepts -fstack-reuse=none... yes
 checking whether C++ compiler accepts -Wstack-protector... yes
 checking whether C++ compiler accepts -fstack-protector-all... yes
-checking whether C++ compiler accepts -fcf-protection=full... yes
+checking whether the linker accepts -fcf-protection=full... yes
 checking whether C++ compiler accepts -fstack-clash-protection... yes
 checking whether C++ preprocessor accepts -D_FORTIFY_SOURCE=2... yes
 checking whether C++ preprocessor accepts -U_FORTIFY_SOURCE... yes


diff --git a/tmp/a_clang b/tmp/b_clang
index 68c613d418..f238d83acd 100644
--- a/tmp/a_clang
+++ b/tmp/b_clang
@@ -174,7 +174,7 @@ checking whether C++ compiler accepts -fPIC... yes
 checking whether C++ compiler accepts -fstack-reuse=none... no
 checking whether C++ compiler accepts -Wstack-protector... yes
 checking whether C++ compiler accepts -fstack-protector-all... yes
-checking whether C++ compiler accepts -fcf-protection=full... yes
+checking whether the linker accepts -fcf-protection=full... yes
 checking whether C++ compiler accepts -fstack-clash-protection... yes
 checking whether C++ preprocessor accepts -D_FORTIFY_SOURCE=2... yes
 checking whether C++ preprocessor accepts -U_FORTIFY_SOURCE... yes

hebasto

ACK e9189a7, tested with clang-7, clang-10 and gcc: the -fcf-protection=full is not applied for clang-7, but applied for others compilers.

hebasto · 2021-01-04T11:00:42Z

configure.ac

+  dnl -fcf-protection used with Clang 7 causes ld to emit warnings:
+  dnl ld: error: ... <corrupt x86 feature size: 0x8>
+  dnl Use CHECK_LINK_FLAG & --fatal-warnings to ensure we wont use the flag in this case.
+  AX_CHECK_LINK_FLAG([-fcf-protection=full],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fcf-protection=full"],, [[$LDFLAG_WERROR]])


style nit: the readability could be improved

Suggested change

AX_CHECK_LINK_FLAG([-fcf-protection=full],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fcf-protection=full"],, [[$LDFLAG_WERROR]])

AX_CHECK_LINK_FLAG([-fcf-protection=full], [HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fcf-protection=full"], [], [$LDFLAG_WERROR])

…pport e9189a7 build: more robustly check for fcf-protection support (fanquake) Pull request description: When using Clang 7, we may end up trying to use the flag when it won't work properly, which can lead to confusing errors. i.e: ```bash /usr/bin/ld: error: ... <corrupt x86 feature size: 0x8> ``` Use `AX_CHECK_LINK_FLAG` & `--fatal-warnings` to ensure we wont use the flag in this case. We do this as even when the error is emitted, compilation succeeds, and the binaries produced will run. This means we can't just check if the compiler accepts the flag, or if compilation succeeds (without or without `-Werror`, and/or passing `-Wl,--fatal-warnings`, which may not be passed through to the linker). This was reported by someone configuring for fuzzing, on Debian 10, where Clang 7 is the default. See here for a minimal example of the problematic behaviour: https://gist.github.com/fanquake/9b33555fcfebef8eb8c0795a71732bc6 ACKs for top commit: pstratem: tested ACK e9189a7 MarcoFalke: not an ACK e9189a7 , I only tested configure on my system (gcc-10, clang-11): hebasto: ACK e9189a7, tested with clang-7, clang-10 and gcc: the `-fcf-protection=full` is not applied for clang-7, but applied for others compilers. Tree-SHA512: ec24b0cc5523b90139c96cbb33bb98d1e6a24d858c466aa7dfb3c474caf8c50aca53e570fdbc0ff88378406b0ac5d687542452637b1b5fa062e829291b886fc1

…upport" a7973bf Revert "build: more robustly check for fcf-protection support" (fanquake) Pull request description: We no-longer support Clang 7 (#24164). Introduced in #20720. This reverts commit e9189a7. ACKs for top commit: hebasto: re-ACK a7973bf Tree-SHA512: 82559637f21a97434ab29f908ebda1aada08b0786cbbf0b4d11085241942314c3f04261a624c5cd2cb3c94c99046b56626830da6b9775981ab4ba10d5979f998

…ction support" a7973bf Revert "build: more robustly check for fcf-protection support" (fanquake) Pull request description: We no-longer support Clang 7 (bitcoin#24164). Introduced in bitcoin#20720. This reverts commit e9189a7. ACKs for top commit: hebasto: re-ACK a7973bf Tree-SHA512: 82559637f21a97434ab29f908ebda1aada08b0786cbbf0b4d11085241942314c3f04261a624c5cd2cb3c94c99046b56626830da6b9775981ab4ba10d5979f998

…pport e9189a7 build: more robustly check for fcf-protection support (fanquake) Pull request description: When using Clang 7, we may end up trying to use the flag when it won't work properly, which can lead to confusing errors. i.e: ```bash /usr/bin/ld: error: ... <corrupt x86 feature size: 0x8> ``` Use `AX_CHECK_LINK_FLAG` & `--fatal-warnings` to ensure we wont use the flag in this case. We do this as even when the error is emitted, compilation succeeds, and the binaries produced will run. This means we can't just check if the compiler accepts the flag, or if compilation succeeds (without or without `-Werror`, and/or passing `-Wl,--fatal-warnings`, which may not be passed through to the linker). This was reported by someone configuring for fuzzing, on Debian 10, where Clang 7 is the default. See here for a minimal example of the problematic behaviour: https://gist.github.com/fanquake/9b33555fcfebef8eb8c0795a71732bc6 ACKs for top commit: pstratem: tested ACK e9189a7 MarcoFalke: not an ACK e9189a7 , I only tested configure on my system (gcc-10, clang-11): hebasto: ACK e9189a7, tested with clang-7, clang-10 and gcc: the `-fcf-protection=full` is not applied for clang-7, but applied for others compilers. Tree-SHA512: ec24b0cc5523b90139c96cbb33bb98d1e6a24d858c466aa7dfb3c474caf8c50aca53e570fdbc0ff88378406b0ac5d687542452637b1b5fa062e829291b886fc1

fanquake added the Build system label Dec 19, 2020

hebasto approved these changes Dec 19, 2020

View reviewed changes

fanquake force-pushed the more_robust_fcf_protection branch from 01ca4bd to 1643af3 Compare December 20, 2020 08:08

fanquake marked this pull request as draft December 20, 2020 08:22

fanquake force-pushed the more_robust_fcf_protection branch from 1643af3 to e9189a7 Compare December 20, 2020 13:59

fanquake marked this pull request as ready for review December 20, 2020 14:00

fanquake requested a review from hebasto December 28, 2020 07:12

hebasto approved these changes Jan 4, 2021

View reviewed changes

fanquake merged commit 8636288 into bitcoin:master Feb 8, 2021

dcvanilla approved these changes Feb 8, 2021

View reviewed changes

fanquake deleted the more_robust_fcf_protection branch February 9, 2021 06:49

fanquake mentioned this pull request May 30, 2022

Revert "build: more robustly check for fcf-protection support" #25246

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: more robustly check for fcf-protection support #20720

build: more robustly check for fcf-protection support #20720

Uh oh!

fanquake commented Dec 19, 2020 •

edited

Loading

Uh oh!

hebasto commented Dec 19, 2020

Uh oh!

practicalswift commented Dec 19, 2020

Uh oh!

laanwj commented Dec 19, 2020

Uh oh!

hebasto left a comment •

edited

Loading

Uh oh!

fanquake commented Dec 20, 2020

Uh oh!

hebasto commented Dec 20, 2020

Uh oh!

laanwj commented Dec 20, 2020

Uh oh!

fanquake commented Dec 20, 2020

Uh oh!

pstratem commented Dec 20, 2020

Uh oh!

fanquake commented Dec 20, 2020

Uh oh!

pstratem commented Dec 20, 2020

Uh oh!

pstratem commented Dec 20, 2020

Uh oh!

maflcko commented Dec 21, 2020

Uh oh!

hebasto left a comment

Uh oh!

hebasto Jan 4, 2021

Uh oh!

Uh oh!

	AX_CHECK_LINK_FLAG([-fcf-protection=full],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fcf-protection=full"],, [[$LDFLAG_WERROR]])
	AX_CHECK_LINK_FLAG([-fcf-protection=full], [HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fcf-protection=full"], [], [$LDFLAG_WERROR])

build: more robustly check for fcf-protection support #20720

build: more robustly check for fcf-protection support #20720

Uh oh!

Conversation

fanquake commented Dec 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Dec 19, 2020

Uh oh!

practicalswift commented Dec 19, 2020

Uh oh!

laanwj commented Dec 19, 2020

Uh oh!

hebasto 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!

fanquake commented Dec 20, 2020

Uh oh!

hebasto commented Dec 20, 2020

Uh oh!

laanwj commented Dec 20, 2020

Uh oh!

fanquake commented Dec 20, 2020

Uh oh!

pstratem commented Dec 20, 2020

Uh oh!

fanquake commented Dec 20, 2020

Uh oh!

pstratem commented Dec 20, 2020

Uh oh!

pstratem commented Dec 20, 2020

Uh oh!

maflcko commented Dec 21, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Dec 19, 2020 •

edited

Loading

hebasto left a comment •

edited

Loading