contrib: simplify test-security-check
#30423
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Mind elaborating this statement please? I mean, |
This was referenced Jul 11, 2024
Testing one thing per test. |
hebasto
reviewed
Jul 11, 2024
| (1, executable+': failed CONTROL_FLOW')) | ||
| self.assertEqual(call_security_check(cc, source, executable, ['-Wl,--nxcompat','-Wl,--enable-reloc-section','-Wl,--dynamicbase','-Wl,--high-entropy-va','-pie','-fPIE', '-fcf-protection=full','-fstack-protector-all', '-lssp']), | ||
| (0, '')) | ||
| pass_flags = ['-pie','-fPIE', '-Wl,--nxcompat','-Wl,--enable-reloc-section','-Wl,--dynamicbase','-Wl,--high-entropy-va', '-fcf-protection=full','-fstack-protector-all', '-lssp'] |
There was a problem hiding this comment.
Is the -pie flag supported here?
From the ld linker docs:
This is currently only supported on ELF platforms.
There was a problem hiding this comment.
Those look like the wrong docs:
x86_64-w64-mingw32-ld --help | grep pie
-pie, --pic-executable Create a position independent executable
-no-pie Create a position dependent executable (default)There was a problem hiding this comment.
There seems to be some inconsistency in docs. For instance, in https://manpages.debian.org/bookworm/binutils-mingw-w64-x86-64/x86_64-w64-mingw32-ld.1.en.html:
-pie
--pic-executable
Create a position independent executable. This is currently only supported on ELF platforms. Position independent executables are similar to shared libraries in that they are relocated by the dynamic linker to the virtual address the OS chooses for them (which can vary between invocations). Like normal dynamically linked executables they can be executed and symbols defined in the executable cannot be overridden by shared libraries.
-no-pie
Create a position dependent executable. This is the default.
4ec81a6 to
d1b9c2c
Compare
This was referenced Jul 12, 2024
tdb3
approved these changes
Jul 14, 2024
There was a problem hiding this comment.
Approach ACK
Nice. Increases readability.
nit:
Thought about opportunities for deduplication (e.g. creating a dictionary/list containing test cases with associated parameters/arguments, then looping through it). It's probably overkill for a file as small as test-security-check.py and wouldn't necessarily enhance readability.
d1b9c2c to
6392d8f
Compare
6392d8f to
51d8f43
Compare
If the binaries don't exist, the Guix build has failed for some other reason. There's no need to check for unknown architectures, or executable formats, as the only ones that could be built are those that we've configured toolchains for in Guix. We've also been doing this inconsistently across the two scripts.
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]
|
|
Guix Build (x86_64): 222a30ae2094e1e2536e5a1e8794d04f300fbf6ed0f67e0f6dcb4160e3d075a7 guix-build-1bc9f64bee91/output/aarch64-linux-gnu/SHA256SUMS.part
f7a7b2a6f7e85aa633d6cdb9907bf11cf05c3bdabdf32a4fefaf2bb63264583d guix-build-1bc9f64bee91/output/aarch64-linux-gnu/bitcoin-1bc9f64bee91-aarch64-linux-gnu-debug.tar.gz
8884a25e5a214b224d2c7d73e19786174ddb4eed37c2daa0dc57c23d1f49fc3c guix-build-1bc9f64bee91/output/aarch64-linux-gnu/bitcoin-1bc9f64bee91-aarch64-linux-gnu.tar.gz
36855764cdf7488af17c16524adda0b3c019d08f47e87a8aea5596aade9eba87 guix-build-1bc9f64bee91/output/arm-linux-gnueabihf/SHA256SUMS.part
8bbf0f76f2abe4ee6bdfb1eba9b67410f187afc60433e5be5f7fb11c184660a9 guix-build-1bc9f64bee91/output/arm-linux-gnueabihf/bitcoin-1bc9f64bee91-arm-linux-gnueabihf-debug.tar.gz
db5ff885a8b728415988456b3f02dd5178124a9ff4ea5f0946f3327038ea33f4 guix-build-1bc9f64bee91/output/arm-linux-gnueabihf/bitcoin-1bc9f64bee91-arm-linux-gnueabihf.tar.gz
314ce4cd1ccd1ca089c29a68d9f4d031c9dc5e0cf5f6d9b0234db1d7cdc7d641 guix-build-1bc9f64bee91/output/arm64-apple-darwin/SHA256SUMS.part
61d6eb60cdd3b37abf57b560ceb7fc617f56d8d0ab28605673faa93470e9da7a guix-build-1bc9f64bee91/output/arm64-apple-darwin/bitcoin-1bc9f64bee91-arm64-apple-darwin-unsigned.tar.gz
bbff8c672e248e2e08ceea8641ca3c58576031afaa801b920b2d6e3483684a6c guix-build-1bc9f64bee91/output/arm64-apple-darwin/bitcoin-1bc9f64bee91-arm64-apple-darwin-unsigned.zip
863ff8c24b3dffb27d548cd5e80ff9205f6b06bc21a4a8249db26acf1fb1d7e0 guix-build-1bc9f64bee91/output/arm64-apple-darwin/bitcoin-1bc9f64bee91-arm64-apple-darwin.tar.gz
c441e38346c2276f5d3e35e22b00c2bd31c8410d19b0726cce155ffe1c6649a3 guix-build-1bc9f64bee91/output/dist-archive/bitcoin-1bc9f64bee91.tar.gz
ac7784cf706551e704103e99ee1a7df79ae13870fa5e3dcd2ea8cbc74a36e775 guix-build-1bc9f64bee91/output/powerpc64-linux-gnu/SHA256SUMS.part
fb4f88f4f6e6baeb6b14e7dd107b3678bc3174192d42d9844cce7f24ea4ecc30 guix-build-1bc9f64bee91/output/powerpc64-linux-gnu/bitcoin-1bc9f64bee91-powerpc64-linux-gnu-debug.tar.gz
b86ca80ab5110228bb1c32641a75a5fbfb77454583a5cff6695f3335fc089b1e guix-build-1bc9f64bee91/output/powerpc64-linux-gnu/bitcoin-1bc9f64bee91-powerpc64-linux-gnu.tar.gz
0a345fe896f8bf91ed5092799a6512e3baa1f934b663cbe43ddf2e70333c0b76 guix-build-1bc9f64bee91/output/riscv64-linux-gnu/SHA256SUMS.part
14d06d3b49bfc8d94aa548799b7f886efba1253a5e13f3179cecfbc2a4531e0d guix-build-1bc9f64bee91/output/riscv64-linux-gnu/bitcoin-1bc9f64bee91-riscv64-linux-gnu-debug.tar.gz
bca5ca1bbc303fa078567e284e3e6983fc2e7ab9c6ca24cc200185acb608b845 guix-build-1bc9f64bee91/output/riscv64-linux-gnu/bitcoin-1bc9f64bee91-riscv64-linux-gnu.tar.gz
b7b916e97a5a9a24f83938c3dc06e8ef6541e3112f6becb372d4a0430221c4da guix-build-1bc9f64bee91/output/x86_64-apple-darwin/SHA256SUMS.part
9f1c2868a1704d60889e8675785d2a02a92b5d32d28e61cc9b9c06e12f44bd40 guix-build-1bc9f64bee91/output/x86_64-apple-darwin/bitcoin-1bc9f64bee91-x86_64-apple-darwin-unsigned.tar.gz
ca290ebae788bcad2c21882cb7962d40d41878df8ccb6e5ab9cb434aa02ab6e0 guix-build-1bc9f64bee91/output/x86_64-apple-darwin/bitcoin-1bc9f64bee91-x86_64-apple-darwin-unsigned.zip
323ea529b7646a8d0326e465d64ef36ef9b304a0f8b379d6e61f4c6aaa9479b1 guix-build-1bc9f64bee91/output/x86_64-apple-darwin/bitcoin-1bc9f64bee91-x86_64-apple-darwin.tar.gz
ab99ee9a4e76f8f81958e0ae5381d422961e1e4ca1e38e88b73a858024d4933b guix-build-1bc9f64bee91/output/x86_64-linux-gnu/SHA256SUMS.part
8e5044e712fd0d674dba95bf3b02f3c1f872c3f0933f15f6fd142aee51072d65 guix-build-1bc9f64bee91/output/x86_64-linux-gnu/bitcoin-1bc9f64bee91-x86_64-linux-gnu-debug.tar.gz
583e16f677421420cfddc4b591c7fbad7879a154572567fe8ca7d94ea16d4ab6 guix-build-1bc9f64bee91/output/x86_64-linux-gnu/bitcoin-1bc9f64bee91-x86_64-linux-gnu.tar.gz
e8cea74fcfd0202186659d76c5d3bf2d1b35560bb74d71395b1540b0223ebcf2 guix-build-1bc9f64bee91/output/x86_64-w64-mingw32/SHA256SUMS.part
f67bbf2cc9c48ae29b0543f62fec5337e711cac8b4dede393aa7d805a4d4c926 guix-build-1bc9f64bee91/output/x86_64-w64-mingw32/bitcoin-1bc9f64bee91-win64-debug.zip
8aa117609c8857365ed69a5e646aa49a1034800bfd8e40436cede8ae3a69b5cd guix-build-1bc9f64bee91/output/x86_64-w64-mingw32/bitcoin-1bc9f64bee91-win64-setup-unsigned.exe
e26f68b8949c5c5fdea3f513b88f34ddae8f2ff5624383b58ff4041924ac6d66 guix-build-1bc9f64bee91/output/x86_64-w64-mingw32/bitcoin-1bc9f64bee91-win64-unsigned.tar.gz
a4816c6b72892cfbb7518592b031df3d27937c2097a3a7928dda5872424a78ee guix-build-1bc9f64bee91/output/x86_64-w64-mingw32/bitcoin-1bc9f64bee91-win64.zip |
|
My Guix build: |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current
test-security-checkscript is hard to understand, and change (i.e https://github.com/bitcoin/bitcoin/pull/29987/files#diff-52aa0cda44721f089e53b128cb1232a876006ef257b211655456b17dfb2ec712); tests are also not done in isolation (when-possible). Fix that, and add missing checks. Simplifies future toolchain/security/hardening changes.