build: CMake security checks workarounds #31715

1. 
   theuni commented at 9:41 pm on January 22, 2025: member

   This includes 2 hackish workarounds for our security checks, which are themselves quite hackish. It’s unclear exactly what they’re supposed to be testing and why.

   Regardless, these changes are enough to fix the checks when no CXXFLAGS are passed in.

   From what I can tell, the tests are intended to be self-contained and have a default set of flags, then each tests disables a default and verifies that security is weakened as intended. I’m not sure what the utility of that is, but in that spirit I’ve fixed up the default flags/compiles to work on their own.

   The first is a real fix for the test source that gives us a better guarantee that fortification will be turned on. The second adds the flag that fortification requires rather than taking it from our build CXXFLAGS.

2. security: add a memcpy to the security check stub to force a checked call

   For some reason g++ -std=c++20 emits the call for printf, but c++17 doesn't.
   clang's behavior also differs from gcc's. We don't care about the c++ standard
   or compiler for these checks. In this case, we just want to ensure that a
   checked function is generated.

   d709f08775
3. security: "fix" Broken security checks when flags are missing

   These checks are really wonky, so this is a hackish workaround, but a
   reasonable one considering the other flags that are added.
   
   Assuming the default flags are enough to allow the individual tests to fail,
   -O2 should be added to make the fortification work.
   
   This is necessary because cmake only exposed a small subset of its CXXFLAGS
   when doing `make test-security-check`. A more correct fix would be for it to
   amalgamate the CXXFLAGS like the ones that are printed in the build summary.
   I'm leaving that as a follow-up because it's unclear what these security checks
   are even meant to be testing.

   ee208607e0
4. 
   DrahtBot commented at 9:41 pm on January 22, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31715.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #25573 ([POC] guix: produce a fully -static-pie bitcoind by fanquake)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

5. DrahtBot added the label Build system on Jan 22, 2025
6. 
   theuni commented at 9:42 pm on January 22, 2025: member

   Ping @fanquake. As discussed.

   I think the second fix is reasonable given the state of things, but if you’d rather get a better flags output from CMake instead I can do that.

7. 
   fanquake commented at 2:57 pm on January 23, 2025: member

   Given these weren’t ported completely (#31698), it seems like if were are going to touch them now, we should probably clean that up at the same time. Although I also agree that these scripts are less usful to have/maintain post-Guix, and I will look at removing them entirely.
8. fanquake added this to the milestone 29.0 on Jan 31, 2025
9. fanquake referenced this in commit 8f25a36e29 on Feb 7, 2025
10. fanquake referenced this in commit 4ec671a77f on Feb 7, 2025
11. fanquake referenced this in commit 485de09d9e on Feb 7, 2025
12. 
    fanquake commented at 3:58 pm on February 7, 2025: member

    and I will look at removing them entirely.

    Opened a removal in #31818.

13. 
    theuni commented at 5:33 pm on February 7, 2025: member

    Closing in favor of #31818.
14. theuni closed this on Feb 7, 2025

    ---

15. fanquake referenced this in commit 8d0824109c on Feb 7, 2025

Contributors
theuni DrahtBot fanquake

Labels
Build system

Milestone
29.0