build: use _FORTIFY_SOURCE=3 #27027

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:fortify_source_3 changing 1 files +2 −2
  1. fanquake commented at 3:34 pm on February 2, 2023: member

    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:

     0nm -C src/bitcoind | grep _chk
     1                 U __fprintf_chk@GLIBC_2.17
     2                 U __memcpy_chk@GLIBC_2.17
     3                 U __snprintf_chk@GLIBC_2.17
     4                 U __sprintf_chk@GLIBC_2.17
     5                 U __stack_chk_fail@GLIBC_2.17
     6                 U __stack_chk_guard@GLIBC_2.17
     7                 U __vsnprintf_chk@GLIBC_2.17
     8
     9objdump -d src/bitcoind | grep "_chk@plt" | wc -l
    1033
    

    vs this branch:

     0nm -C src/bitcoind | grep _chk
     1                 U __fprintf_chk@GLIBC_2.17
     2                 U __memcpy_chk@GLIBC_2.17
     3                 U __memset_chk@GLIBC_2.17
     4                 U __snprintf_chk@GLIBC_2.17
     5                 U __sprintf_chk@GLIBC_2.17
     6                 U __stack_chk_fail@GLIBC_2.17
     7                 U __stack_chk_guard@GLIBC_2.17
     8                 U __vsnprintf_chk@GLIBC_2.17
     9
    10objdump -d src/bitcoind | grep "_chk@plt" | wc -l
    1161
    

    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:

     0#if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0
     1# if !defined __OPTIMIZE__ || __OPTIMIZE__ <= 0
     2#  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
     3# elif !__GNUC_PREREQ (4, 1)
     4#  warning _FORTIFY_SOURCE requires GCC 4.1 or later
     5# elif _FORTIFY_SOURCE > 1
     6#  define __USE_FORTIFY_LEVEL 2
     7# else
     8#  define __USE_FORTIFY_LEVEL 1
     9# endif
    10#endif
    11#ifndef __USE_FORTIFY_LEVEL
    12# define __USE_FORTIFY_LEVEL 0
    13#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

  2. DrahtBot commented at 3:34 pm on February 2, 2023: contributor

    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.

  3. DrahtBot added the label Build system on Feb 2, 2023
  4. hebasto commented at 3:42 pm on February 2, 2023: member

    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?

  5. theuni commented at 8:33 pm on February 2, 2023: member

    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.

  6. john-moffett commented at 9:42 pm on February 2, 2023: contributor

    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.

  7. theuni commented at 9:00 pm on February 3, 2023: member

    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.

  8. fanquake commented at 3:34 pm on February 5, 2023: member

    Hahaha, I was just poking at this myself today.

    heh. I had just sent a doc change up to glibc before opening: https://github.com/bminor/glibc/commit/1423a26a488aae1c6fa7210e20c147a242f40f47.

    FYI support has been recently proposed to mingw as well:

    Looks like this has gone in https://github.com/mirror/mingw-w64/commit/e34c747fa6a3fb1def1518d1953a7ba516d24ab1.

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

  9. TheCharlatan commented at 9:49 am on February 14, 2023: contributor

    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:

    0objdump -d src/bitcoind | grep "_chk@plt" | wc -l
    1...
    236
    

    This branch:

    0objdump -d src/bitcoind | grep "_chk@plt" | wc -l
    1...
    276
    
  10. build: use _FORTIFY_SOURCE=3
    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
    4faa4e37a6
  11. fanquake force-pushed on Feb 17, 2023
  12. theuni commented at 3:18 pm on February 17, 2023: member

    ACK 4faa4e37a6511c6ada303ef7929ac99c7462f083. 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.

  13. fanquake merged this on Feb 20, 2023
  14. fanquake closed this on Feb 20, 2023

  15. fanquake deleted the branch on Feb 20, 2023
  16. sidhujag referenced this in commit 80d6385ec3 on Feb 25, 2023
  17. maflcko commented at 9:37 am on March 2, 2023: member
  18. kristapsk commented at 9:26 am on March 8, 2023: contributor

    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:

     0  CXX      bitcoind-bitcoind.o
     1In 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,
     2                 from /usr/lib/gcc/x86_64-pc-linux-gnu/11/include/g++-v11/x86_64-pc-linux-gnu/bits/c++config.h:586,
     3                 from /usr/lib/gcc/x86_64-pc-linux-gnu/11/include/g++-v11/bits/stl_algobase.h:59,
     4                 from /usr/lib/gcc/x86_64-pc-linux-gnu/11/include/g++-v11/memory:63,
     5                 from ./chainparamsbase.h:8,
     6                 from ./chainparams.h:9,
     7                 from bitcoind.cpp:10:
     8/usr/include/features.h:424:5: warning: #warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform [-Wcpp]
     9  424 | #   warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform
    10      |     ^~~~~~~
    
  19. kristapsk commented at 9:35 am on March 8, 2023: contributor
  20. fanquake commented at 11:04 am on March 8, 2023: member

    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.

  21. kristapsk commented at 11:16 am on March 8, 2023: contributor

    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?

  22. bitcoin locked this on Mar 7, 2024

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-09-29 01:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me