build: remove ENABLE_HARDENING condition from check-security

fanquake commented at 5:13 pm on February 17, 2025: member

This check is only used in release builds, where hardening should always be enabled. I can’t think of a reason we’d want to silently skip these checks if hardening was inadvertently disabled.

build: remove ENABLE_HARDENING cond from check-security

This check is only used in release builds, where hardening should always
be enabled. I can't think of a reason we'd want to silently skip these
checks if hardening was inadvertently disabled.

113a7a363f

fanquake added this to the milestone 29.0 on Feb 17, 2025

DrahtBot commented at 5:13 pm on February 17, 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/31892.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, maflcko, hebasto

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31233 (cmake: Improve robustness and usability by hebasto)

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.

DrahtBot added the label Build system on Feb 17, 2025

TheCharlatan approved

TheCharlatan commented at 10:00 am on February 18, 2025: contributor

ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04

maflcko commented at 10:09 am on February 18, 2025: member

I can’t think of a reason we’d want to silently skip these checks if hardening was inadvertently disabled.

Does a missing target really get silently skipped? Locally I get the expected failure:

0$ cmake --build ./bld-cmake/ -j 1 --target check-security-missing ; echo $?
1gmake: *** No rule to make target 'check-security-missing'.  Stop.
22

fanquake commented at 10:10 am on February 18, 2025: member

Does a missing target really get silently skipped? Locally I get the expected failure:

Did you pass -DENABLE_HARDENING=OFF? The point here is that I can’t see a reason for this “skip if hardening is disabled condition” to exist, because this code should always be run, and these targets aren’t for use outside a release build.

fanquake commented at 10:15 am on February 18, 2025: member

Actually, I don’t understand what you’ve tested here. test-security-missing isn’t a target, so that result is expected. If you configure with -DENABLE_HARDENING=OFF, you’ll get:

0cmake --build build --target check-security        
1Built target check-security

but nothing will have actually been done, because the target is a dummy.

maflcko commented at 10:24 am on February 18, 2025: member

Sorry, I missed that a dummy target was added. Makes sense to remove the dummy target. Removing the condition seems fine as well.

lgtm ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04

hebasto commented at 11:30 am on February 18, 2025: member

For historical context, the current CMake’s implementation mirrors the Autotools’ implementation introduced in f3d3eaf78eb51238d799d8f20a585550d1567719:https://github.com/bitcoin/bitcoin/blob/32efe850438ef22e2de39e562af557872a402c31/src/Makefile.am#L1057-L1061

hebasto commented at 11:41 am on February 18, 2025: member

This check is only used in release builds, where hardening should always be enabled. I can’t think of a reason we’d want to silently skip these checks if hardening was inadvertently disabled.

In that case, removing the dummy target is the correct solution, but the if(ENABLE_HARDENING) condition should remain:

0$ cmake -B build -G "Ninja" -DENABLE_HARDENING=OFF
1$ cmake --build build -t check-security
2ninja: error: unknown target 'check-security'

or

0$ cmake -B build -G "Unix Makefiles" -DENABLE_HARDENING=OFF
1$ cmake --build build -t check-security
2gmake: *** No rule to make target 'check-security'. Stop.

fanquake commented at 11:45 am on February 18, 2025: member

but the if(ENABLE_HARDENING) condition should remain:

Why? Those aren’t release builds.

hebasto commented at 11:47 am on February 18, 2025: member

but the if(ENABLE_HARDENING) condition should remain:

Why? Those aren’t release builds.

In case when “hardening was inadvertently disabled”.

fanquake commented at 11:48 am on February 18, 2025: member

In case when “hardening was inadvertently disabled”.

I don’t know what you mean. If hardening was accidently disabled then the build failing is a good thing.

hebasto commented at 11:52 am on February 18, 2025: member

In case when “hardening was inadvertently disabled”.

I don’t know what you mean. If hardening was accidently disabled then the build failing is a good thing.

Correct. However, this PR does the opposite: the check-security custom target is available unconditionally, and building it won’t fail (at least at the build system level).

fanquake commented at 11:53 am on February 18, 2025: member

the check-security custom target is available unconditionally

This is what we want.

and building it won’t fail (at least at the build system level).

It will fail, because the checks will fail.

hebasto commented at 11:54 am on February 18, 2025: member

the check-security custom target is available unconditionally

This is what we want.

What reasons for?

and building it won’t fail (at least at the build system level).

It will fail, because the checks will fail.

It should fail earlier, at the build system level.

TheCharlatan commented at 12:05 pm on February 18, 2025: contributor

It should fail earlier, at the build system level.

I’m not sure about making it fail on the build system level (i.e. keeping as is). Isn’t that just adding brittle assumptions to the target’s configuration?

fanquake commented at 12:07 pm on February 18, 2025: member

Isn’t that just adding brittle assumptions to the target’s configuration?

Yea. I don’t see why we need to add more logic to try and preemptively catch some issue, rather than just unconditionally running the tests that soley exist to catch that issue.

hebasto approved

hebasto commented at 12:18 pm on February 18, 2025: member

ACK 113a7a363faf1ace6b08a28cf4075dbce05f8f04.

This check is only used in release builds…

Not directly related to this PR, but It seems reasonable to consider removing this condition as well:https://github.com/bitcoin/bitcoin/blob/28dec6c5f8bd35ef4e6cb8d7aa3f21b6879acf98/cmake/module/Maintenance.cmake#L27

fanquake merged this on Feb 18, 2025

fanquake closed this on Feb 18, 2025

fanquake deleted the branch on Feb 18, 2025

build: remove ENABLE_HARDENING condition from check-security #31892

Code Coverage & Benchmarks

Reviews

Conflicts