build: use _FORTIFY_SOURCE=3 #27027

fanquake · 2023-02-02T15:34:22Z

glibc 2.33 introduced a new fortification level, _FORTIFY_SOURCE=3. It improves the coverage of cases where _FORTIFY_SOURCE can use _chk functions.

For example, using GCC 13 and glibc 2.36 (Fedora Rawhide), compiling master:

nm -C src/bitcoind | grep _chk
                 U __fprintf_chk@GLIBC_2.17
                 U __memcpy_chk@GLIBC_2.17
                 U __snprintf_chk@GLIBC_2.17
                 U __sprintf_chk@GLIBC_2.17
                 U __stack_chk_fail@GLIBC_2.17
                 U __stack_chk_guard@GLIBC_2.17
                 U __vsnprintf_chk@GLIBC_2.17

objdump -d src/bitcoind | grep "_chk@plt" | wc -l
33

vs this branch:

nm -C src/bitcoind | grep _chk
                 U __fprintf_chk@GLIBC_2.17
                 U __memcpy_chk@GLIBC_2.17
                 U __memset_chk@GLIBC_2.17
                 U __snprintf_chk@GLIBC_2.17
                 U __sprintf_chk@GLIBC_2.17
                 U __stack_chk_fail@GLIBC_2.17
                 U __stack_chk_guard@GLIBC_2.17
                 U __vsnprintf_chk@GLIBC_2.17

objdump -d src/bitcoind | grep "_chk@plt" | wc -l
61

Usage of level 3 requires LLVM/Clang 9+, or GCC 12+. Older compilers/glibc will still use _FORTIFY_SOURCE=2. For example, in the glibc we currently use for Linux release builds (2.24), __USE_FORTIFY_LEVEL is determined using the following:

#if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0
# if !defined __OPTIMIZE__ || __OPTIMIZE__ <= 0
#  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
# elif !__GNUC_PREREQ (4, 1)
#  warning _FORTIFY_SOURCE requires GCC 4.1 or later
# elif _FORTIFY_SOURCE > 1
#  define __USE_FORTIFY_LEVEL 2
# else
#  define __USE_FORTIFY_LEVEL 1
# endif
#endif
#ifndef __USE_FORTIFY_LEVEL
# define __USE_FORTIFY_LEVEL 0
#endif

so any value > 1 will turn on _FORTIFY_SOURCE=2. This value detection logic has become slightly more complex in later versions of glibc.

https://sourceware.org/pipermail/libc-alpha/2021-February/122207.html
https://developers.redhat.com/blog/2021/04/16/broadening-compiler-checks-for-buffer-overflows-in-_fortify_source

DrahtBot · 2023-02-02T15:34:27Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theuni
Concept ACK	john-moffett, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

hebasto · 2023-02-02T15:42:05Z

https://sourceware.org/pipermail/libc-alpha/2021-February/122207.html:

At this level, glibc may use additional checks that may have an additional performance overhead.

Any estimations of the mentioned "additional performance overhead" for our binaries?

theuni · 2023-02-02T20:33:26Z

Hahaha, I was just poking at this myself today.

FYI support has been recently proposed to mingw as well: https://sourceforge.net/p/mingw-w64/mailman/message/37772473/

At this level, glibc may use additional checks that may have an additional performance overhead.

Any estimations of the mentioned "additional performance overhead" for our binaries?

This was my hesitation as well. It'd be nice to have a before/after for our benches at least.

Edit: Concept ACK, btw. And ACK after a quick bench shows it doesn't tank performace.

john-moffett · 2023-02-02T21:42:05Z

Concept ACK

Yeah, I ran the benchmarks before and after with both a Linux (GCC) and macOS (Clang) build and couldn’t see a significant change, not that I was expecting it on macOS at least. I may try more extensive measurements soon.

theuni · 2023-02-03T21:00:28Z

In order to test this, I decided to try out the recommended fortify metrics plugin for gcc.

I spent all day tracking down a bug in the plugin (PR'd what I believe to be the fix here). Next week I'll use it to gather some statistics on _FORTIFY_SOURCE=2 vs =3.

It will also be usable to test how much additional fortification we'd get if we built depends that way as well.

fanquake · 2023-02-05T15:34:20Z

Hahaha, I was just poking at this myself today.

heh. I had just sent a doc change up to glibc before opening: bminor/glibc@1423a26.

FYI support has been recently proposed to mingw as well:

Looks like this has gone in mirror/mingw-w64@e34c747.

Note I've also opened #27038 to add a sanity check that foritification (like other haardneing) is being used in the release binaries.

TheCharlatan · 2023-02-14T09:49:13Z

Concept ACK

Ran this on Ubuntu 22.04 with glibc 2.35 and clang 14. Also did not observe a significant slowdown of the benchmarks.
On master:

objdump -d src/bitcoind | grep "_chk@plt" | wc -l
...
36

This branch:

objdump -d src/bitcoind | grep "_chk@plt" | wc -l
...
76

glibc 2.33 introduced a new fortification level, _FORTIFY_SOURCE=3. Which improves the coverage of cases where _FORTIFY_SOURCE can use _chk functions. For example, using GCC 13 and glibc 2.36 (Fedora Rawhide), compiling master: ```bash nm -C src/bitcoind | grep _chk U __fprintf_chk@GLIBC_2.17 U __memcpy_chk@GLIBC_2.17 U __snprintf_chk@GLIBC_2.17 U __sprintf_chk@GLIBC_2.17 U __stack_chk_fail@GLIBC_2.17 U __stack_chk_guard@GLIBC_2.17 U __vsnprintf_chk@GLIBC_2.17 objdump -d src/bitcoind | grep "_chk@plt" | wc -l 33 ``` vs this branch: ```bash nm -C src/bitcoind | grep _chk U __fprintf_chk@GLIBC_2.17 U __memcpy_chk@GLIBC_2.17 U __memset_chk@GLIBC_2.17 U __snprintf_chk@GLIBC_2.17 U __sprintf_chk@GLIBC_2.17 U __stack_chk_fail@GLIBC_2.17 U __stack_chk_guard@GLIBC_2.17 U __vsnprintf_chk@GLIBC_2.17 objdump -d src/bitcoind | grep "_chk@plt" | wc -l 61 ``` Usage of level 3 requires LLVM/Clang 9+, or GCC 12+. Older compilers/glibc will still use _FORTIFY_SOURCE=2. For example, in the glibc we currently use for Linux release builds (2.24), FORTIFY_LEVEL is determined using the following: ```c ``` so any value > 1 will turn on _FORTIFY_SOURCE=2. https://sourceware.org/pipermail/libc-alpha/2021-February/122207.html https://developers.redhat.com/blog/2021/04/16/broadening-compiler-checks-for-buffer-overflows-in-_fortify_source

theuni · 2023-02-17T15:18:20Z

ACK 4faa4e3. After playing with this quite a bit I didn't observe any noticeable pitfalls.

I'd expect this to matter more in low-level code, so I'm interested to see how much #27118 affects the numbers.

4faa4e3 build: use _FORTIFY_SOURCE=3 (fanquake) Pull request description: [glibc 2.33](https://sourceware.org/pipermail/libc-alpha/2021-February/122207.html) introduced a new fortification level, `_FORTIFY_SOURCE=3`. It improves the coverage of cases where `_FORTIFY_SOURCE` can use `_chk` functions. For example, using GCC 13 and glibc 2.36 (Fedora Rawhide), compiling master: ```bash nm -C src/bitcoind | grep _chk U __fprintf_chk@GLIBC_2.17 U __memcpy_chk@GLIBC_2.17 U __snprintf_chk@GLIBC_2.17 U __sprintf_chk@GLIBC_2.17 U __stack_chk_fail@GLIBC_2.17 U __stack_chk_guard@GLIBC_2.17 U __vsnprintf_chk@GLIBC_2.17 objdump -d src/bitcoind | grep "_chk@plt" | wc -l 33 ``` vs this branch: ```bash nm -C src/bitcoind | grep _chk U __fprintf_chk@GLIBC_2.17 U __memcpy_chk@GLIBC_2.17 U __memset_chk@GLIBC_2.17 U __snprintf_chk@GLIBC_2.17 U __sprintf_chk@GLIBC_2.17 U __stack_chk_fail@GLIBC_2.17 U __stack_chk_guard@GLIBC_2.17 U __vsnprintf_chk@GLIBC_2.17 objdump -d src/bitcoind | grep "_chk@plt" | wc -l 61 ``` Usage of level 3 requires LLVM/Clang 9+, or GCC 12+. Older compilers/glibc will still use _FORTIFY_SOURCE=2. For example, in the glibc we currently use for Linux release builds (2.24), `__USE_FORTIFY_LEVEL` is determined using the following: ```c #if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0 # if !defined __OPTIMIZE__ || __OPTIMIZE__ <= 0 # warning _FORTIFY_SOURCE requires compiling with optimization (-O) # elif !__GNUC_PREREQ (4, 1) # warning _FORTIFY_SOURCE requires GCC 4.1 or later # elif _FORTIFY_SOURCE > 1 # define __USE_FORTIFY_LEVEL 2 # else # define __USE_FORTIFY_LEVEL 1 # endif #endif #ifndef __USE_FORTIFY_LEVEL # define __USE_FORTIFY_LEVEL 0 #endif ``` so any value > 1 will turn on `_FORTIFY_SOURCE=2`. This value detection logic has become slightly more complex in later versions of glibc. https://sourceware.org/pipermail/libc-alpha/2021-February/122207.html https://developers.redhat.com/blog/2021/04/16/broadening-compiler-checks-for-buffer-overflows-in-_fortify_source ACKs for top commit: theuni: ACK 4faa4e3. After playing with this quite a bit I didn't observe any noticeable pitfalls. Tree-SHA512: e84ba49e3872c29fed1e2aea237b0d6bdff0d1274fa3297e2e08317cb62004396ee97b1cd6addb7c8b582498f3fa857a6d84c8e8f5ca97791b93985b47ff7faa

maflcko · 2023-03-02T09:37:22Z

This triggered https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56529 ?

kristapsk · 2023-03-08T09:26:33Z

On my Gentoo system with GCC 11.3.1 and glibc 2.36 this now gives me these warnings for each .cpp file when building Bitcoin Core:

  CXX      bitcoind-bitcoind.o
In file included from /usr/lib/gcc/x86_64-pc-linux-gnu/11/include/g++-v11/x86_64-pc-linux-gnu/bits/os_defines.h:39,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/11/include/g++-v11/x86_64-pc-linux-gnu/bits/c++config.h:586,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/11/include/g++-v11/bits/stl_algobase.h:59,
                 from /usr/lib/gcc/x86_64-pc-linux-gnu/11/include/g++-v11/memory:63,
                 from ./chainparamsbase.h:8,
                 from ./chainparams.h:9,
                 from bitcoind.cpp:10:
/usr/include/features.h:424:5: warning: #warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform [-Wcpp]
  424 | #   warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform
      |     ^~~~~~~

kristapsk · 2023-03-08T09:35:02Z

This triggered https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56529 ?

What's there?

fanquake · 2023-03-08T11:04:13Z

This triggered https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=56529 ?

It will have been #27118 that triggered it. I can't look at this in more detail right now, but will by next week. It's possible that this is already fixed, and there is a patch/similar we can drop into depends.

On my Gentoo system with GCC 11.3.1 and glibc 2.36 this now gives me these warnings for each .cpp file when building Bitcoin Core

Looks like that's because you're using GCC 11.x, which doesn't support >=3 (it is supported with Clang >= 9). Somewhat annoying that glibc is going to warn like that, but they are also harmless, and fortification (level 2) is still being applied.

kristapsk · 2023-03-08T11:16:37Z

Looks like that's because you're using GCC 11.x, which doesn't support >=3 (it is supported with Clang >= 9). Somewhat annoying that glibc is going to warn like that, but they are also harmless, and fortification (level 2) is still being applied.

Could we somehow detect this at configure stage and apply fortification 3 only if it is supported?

DrahtBot added the Build system label Feb 2, 2023

john-moffett mentioned this pull request Feb 3, 2023

test: format-truncation warning in dbwrapper_tests #27014

Closed

fanquake mentioned this pull request Feb 3, 2023

security-check: test for _FORTIFY_SOURCE usage in release binaries #27038

Merged

fanquake force-pushed the fortify_source_3 branch from 7c277d4 to 4faa4e3 Compare February 17, 2023 10:50

fanquake mentioned this pull request Feb 17, 2023

depends: harden libevent #27118

Merged

fanquake merged commit 0561f34 into bitcoin:master Feb 20, 2023

fanquake deleted the fortify_source_3 branch February 20, 2023 16:36

bitcoin locked and limited conversation to collaborators Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

build: use _FORTIFY_SOURCE=3 #27027

build: use _FORTIFY_SOURCE=3 #27027

Uh oh!

fanquake commented Feb 2, 2023

Uh oh!

DrahtBot commented Feb 2, 2023 •

edited

Loading

Uh oh!

hebasto commented Feb 2, 2023

Uh oh!

theuni commented Feb 2, 2023 •

edited

Loading

Uh oh!

john-moffett commented Feb 2, 2023 •

edited

Loading

Uh oh!

theuni commented Feb 3, 2023

Uh oh!

fanquake commented Feb 5, 2023

Uh oh!

TheCharlatan commented Feb 14, 2023

Uh oh!

theuni commented Feb 17, 2023

Uh oh!

maflcko commented Mar 2, 2023

Uh oh!

kristapsk commented Mar 8, 2023

Uh oh!

kristapsk commented Mar 8, 2023

Uh oh!

fanquake commented Mar 8, 2023 •

edited

Loading

Uh oh!

kristapsk commented Mar 8, 2023

Uh oh!

Uh oh!

build: use _FORTIFY_SOURCE=3 #27027

build: use _FORTIFY_SOURCE=3 #27027

Uh oh!

Conversation

fanquake commented Feb 2, 2023

Uh oh!

DrahtBot commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

hebasto commented Feb 2, 2023

Uh oh!

theuni commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

john-moffett commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Feb 3, 2023

Uh oh!

fanquake commented Feb 5, 2023

Uh oh!

TheCharlatan commented Feb 14, 2023

Uh oh!

theuni commented Feb 17, 2023

Uh oh!

maflcko commented Mar 2, 2023

Uh oh!

kristapsk commented Mar 8, 2023

Uh oh!

kristapsk commented Mar 8, 2023

Uh oh!

fanquake commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristapsk commented Mar 8, 2023

Uh oh!

Uh oh!

DrahtBot commented Feb 2, 2023 •

edited

Loading

theuni commented Feb 2, 2023 •

edited

Loading

john-moffett commented Feb 2, 2023 •

edited

Loading

fanquake commented Mar 8, 2023 •

edited

Loading