Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC2b7e01f50b15: misc updates security-check and symbol-check

Summary

non backport changes:

fix identify_executable argument in security-check.py The argument name was wrong, and shown as unused by my IDE. It happenend to work by accident because in the loop where identify_executable is called there is a local variable filename which happened to be the expected thing.

test: use subprocess.run() in test-security-check.py

https://github.com/bitcoin/bitcoin/pull/18434/commits/9fe71a57a6780569e618cf9a8d4f1acf6321017f

scripts: prevent GCC optimising test symbols in test-symbol-check

I noticed in core#22381 that when the test-symbol-check target was being built with Clang and run in the CI it would fail due to using a too-new version of pow (used here). Our CIs use Focal (glibc 2.31) and the version of pow was the optimized version introduced in glibc 2.29:

Optimized generic exp, exp2, log, log2, pow, sinf, cosf, sincosf and tanf.

This made sense, except for that if it was failing when built using Clang, why hadn't it also been failing when being built with GCC?

Turns out GCC is optimizing away that call to pow at all optimization levels,

core#22645

build: Use and test PE binutils with --reloc-section

Also fix test-security-check.py to account for new PE PIE failure
indication.

https://github.com/bitcoin/bitcoin/pull/22381/commits/a8127b34bce3597b8091e14057c926197966a234

This fixes TestSecurityChecks.test_PE, I can now make it work on my machine.
contrib/devtools/test-security-check.py TestSecurityChecks.test_PE

This is a backport of core#22645, and a partial backport of core#18434 (I didn't do the CI part, as these tests are still not ready) and core#22381 (other commits are already backported)

Depends on D13966

Test Plan

gitian builds
guix build

contrib\devtools\test-security-check.py
contrib\devtools\test-symbol-check.py

test-security-check.py now works for both test_ELF and test_PE. test_macho would require running the test on a Mac.

test-symbol-check.py still only works for test_ELF. test_PE needs more backports.

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 23875
Build 47360: Build Diff
Build 47359: arc lint + arc unit

Event Timeline

PiRK created this revision.Jun 1 2023, 10:19

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 1 2023, 10:19

PiRK requested review of this revision.Jun 1 2023, 10:19

teamcity edited the summary of this revision. (Show Details)Jun 1 2023, 10:19

Harbormaster completed remote builds in B23854: Diff 40518.Jun 1 2023, 10:19

@bot gitian-osx

teamcity awarded a token.Jun 1 2023, 10:22

Build Bitcoin ABC Diffs / Diff Testing (gitian-osx) failed.

Tail of the build log:

 * [new tag]             phabricator/diff/40375 -> phabricator/diff/40375
 * [new tag]             phabricator/diff/40379 -> phabricator/diff/40379
 * [new tag]             phabricator/diff/40380 -> phabricator/diff/40380
 * [new tag]             phabricator/diff/40381 -> phabricator/diff/40381
 * [new tag]             phabricator/diff/40400 -> phabricator/diff/40400
 * [new tag]             phabricator/diff/40401 -> phabricator/diff/40401
 * [new tag]             phabricator/diff/40402 -> phabricator/diff/40402
 * [new tag]             phabricator/diff/40404 -> phabricator/diff/40404
 * [new tag]             phabricator/diff/40405 -> phabricator/diff/40405
 * [new tag]             phabricator/diff/40406 -> phabricator/diff/40406
 * [new tag]             phabricator/diff/40408 -> phabricator/diff/40408
 * [new tag]             phabricator/diff/40420 -> phabricator/diff/40420
 * [new tag]             phabricator/diff/40429 -> phabricator/diff/40429
 * [new tag]             phabricator/diff/40439 -> phabricator/diff/40439
 * [new tag]             phabricator/diff/40441 -> phabricator/diff/40441
 * [new tag]             phabricator/diff/40461 -> phabricator/diff/40461
 * [new tag]             phabricator/diff/40463 -> phabricator/diff/40463
 * [new tag]             phabricator/diff/40469 -> phabricator/diff/40469
 * [new tag]             phabricator/diff/40476 -> phabricator/diff/40476
 * [new tag]             phabricator/diff/40477 -> phabricator/diff/40477
 * [new tag]             phabricator/diff/40478 -> phabricator/diff/40478
 * [new tag]             phabricator/diff/40479 -> phabricator/diff/40479
 * [new tag]             phabricator/diff/40480 -> phabricator/diff/40480
 * [new tag]             phabricator/diff/40487 -> phabricator/diff/40487
 * [new tag]             phabricator/diff/40489 -> phabricator/diff/40489
 * [new tag]             phabricator/diff/40490 -> phabricator/diff/40490
 * [new tag]             phabricator/diff/40495 -> phabricator/diff/40495
 * [new tag]             phabricator/diff/40507 -> phabricator/diff/40507
 * [new tag]             phabricator/diff/40508 -> phabricator/diff/40508
 * [new tag]             phabricator/diff/40509 -> phabricator/diff/40509
 * [new tag]             phabricator/diff/40510 -> phabricator/diff/40510
 * [new tag]             phabricator/diff/40511 -> phabricator/diff/40511
 * [new tag]             phabricator/diff/40512 -> phabricator/diff/40512
 * [new tag]             phabricator/diff/40513 -> phabricator/diff/40513
 * [new tag]             phabricator/diff/40518 -> phabricator/diff/40518
 * [new tag]             phabricator/diff/8992  -> phabricator/diff/8992
 * [new tag]             phabricator/diff/8993  -> phabricator/diff/8993
 * [new branch]          master                 -> master
--- Building for bullseye amd64 ---
Stopping target if it is up
Error response from daemon: No such container: gitian-target
Error: No such container: gitian-target
Making a new image copy
Starting target
Checking if target is up.
Preparing build environment
Updating apt-get repository (log in var/install.log)
Installing additional packages (log in var/install.log)
Upgrading system, may take a while (log in var/install.log)
Creating package manifest
Creating build script (var/build-script)
Running build script (log in var/build.log)
./bin/gbuild:23:in `system!': failed to run on-target setarch x86_64 bash -x < var/build-script > var/build.log 2>&1 (RuntimeError)
	from ./bin/gbuild:185:in `build_one_configuration'
	from ./bin/gbuild:339:in `block (2 levels) in <main>'
	from ./bin/gbuild:334:in `each'
	from ./bin/gbuild:334:in `block in <main>'
	from ./bin/gbuild:332:in `each'
	from ./bin/gbuild:332:in `<main>'
Build gitian-osx failed with exit code 1

PiRK planned changes to this revision.Jun 1 2023, 11:33

rebase onto D13963

Harbormaster completed remote builds in B23862: Diff 40528.Jun 1 2023, 15:19

@bot gitian-osx

teamcity updated a token.Jun 1 2023, 15:19

PiRK edited the summary of this revision. (Show Details)Jun 1 2023, 15:20

PiRK added a parent revision: D13966: refactor: use std::shared_mutex & remove Boost Thread.

Build Bitcoin ABC Diffs / Diff Testing (gitian-osx) failed.

Tail of the build log:

 * [new tag]             phabricator/diff/40419 -> phabricator/diff/40419
 * [new tag]             phabricator/diff/40422 -> phabricator/diff/40422
 * [new tag]             phabricator/diff/40423 -> phabricator/diff/40423
 * [new tag]             phabricator/diff/40428 -> phabricator/diff/40428
 * [new tag]             phabricator/diff/40433 -> phabricator/diff/40433
 * [new tag]             phabricator/diff/40447 -> phabricator/diff/40447
 * [new tag]             phabricator/diff/40450 -> phabricator/diff/40450
 * [new tag]             phabricator/diff/40451 -> phabricator/diff/40451
 * [new tag]             phabricator/diff/40461 -> phabricator/diff/40461
 * [new tag]             phabricator/diff/40463 -> phabricator/diff/40463
 * [new tag]             phabricator/diff/40467 -> phabricator/diff/40467
 * [new tag]             phabricator/diff/40469 -> phabricator/diff/40469
 * [new tag]             phabricator/diff/40474 -> phabricator/diff/40474
 * [new tag]             phabricator/diff/40476 -> phabricator/diff/40476
 * [new tag]             phabricator/diff/40477 -> phabricator/diff/40477
 * [new tag]             phabricator/diff/40478 -> phabricator/diff/40478
 * [new tag]             phabricator/diff/40479 -> phabricator/diff/40479
 * [new tag]             phabricator/diff/40480 -> phabricator/diff/40480
 * [new tag]             phabricator/diff/40482 -> phabricator/diff/40482
 * [new tag]             phabricator/diff/40486 -> phabricator/diff/40486
 * [new tag]             phabricator/diff/40489 -> phabricator/diff/40489
 * [new tag]             phabricator/diff/40491 -> phabricator/diff/40491
 * [new tag]             phabricator/diff/40495 -> phabricator/diff/40495
 * [new tag]             phabricator/diff/40507 -> phabricator/diff/40507
 * [new tag]             phabricator/diff/40508 -> phabricator/diff/40508
 * [new tag]             phabricator/diff/40509 -> phabricator/diff/40509
 * [new tag]             phabricator/diff/40510 -> phabricator/diff/40510
 * [new tag]             phabricator/diff/40511 -> phabricator/diff/40511
 * [new tag]             phabricator/diff/40512 -> phabricator/diff/40512
 * [new tag]             phabricator/diff/40513 -> phabricator/diff/40513
 * [new tag]             phabricator/diff/40522 -> phabricator/diff/40522
 * [new tag]             phabricator/diff/40523 -> phabricator/diff/40523
 * [new tag]             phabricator/diff/40524 -> phabricator/diff/40524
 * [new tag]             phabricator/diff/40525 -> phabricator/diff/40525
 * [new tag]             phabricator/diff/40526 -> phabricator/diff/40526
 * [new tag]             phabricator/diff/40527 -> phabricator/diff/40527
 * [new tag]             phabricator/diff/40528 -> phabricator/diff/40528
 * [new tag]             phabricator/diff/8992  -> phabricator/diff/8992
 * [new tag]             phabricator/diff/8993  -> phabricator/diff/8993
 * [new branch]          master                 -> master
--- Building for bullseye amd64 ---
Stopping target if it is up
Making a new image copy
Starting target
Checking if target is up.
Preparing build environment
Updating apt-get repository (log in var/install.log)
Installing additional packages (log in var/install.log)
Upgrading system, may take a while (log in var/install.log)
Creating package manifest
Creating build script (var/build-script)
Running build script (log in var/build.log)
./bin/gbuild:23:in `system!': failed to run on-target setarch x86_64 bash -x < var/build-script > var/build.log 2>&1 (RuntimeError)
	from ./bin/gbuild:185:in `build_one_configuration'
	from ./bin/gbuild:339:in `block (2 levels) in <main>'
	from ./bin/gbuild:334:in `each'
	from ./bin/gbuild:334:in `block in <main>'
	from ./bin/gbuild:332:in `each'
	from ./bin/gbuild:332:in `<main>'
Build gitian-osx failed with exit code 1

PiRK planned changes to this revision.Jun 1 2023, 15:58

Enable control-flow enforcement for hardened builds

@bot gitian-osx gitian-linux gitian-win

teamcity updated a token.Jun 2 2023, 07:24

Harbormaster completed remote builds in B23870: Diff 40542.Jun 2 2023, 07:40

PiRK added a child revision: D13970: devtools: Correctly extract symbol versions in symbol-check.Jun 2 2023, 10:42

The control flow thing is not working on MacOS: https://build.bitcoinabc.org/repository/download/BitcoinABC_BitcoinAbcStaging/568733:id/gitian-osx.tar.gz!/build.log

[1/6] Running security-check.py on bitcoin-cli...
FAILED: src/CMakeFiles/security-check-bitcoin-cli
cd /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src && /usr/bin/python3.9 /home/debian/build/bitcoin/contrib/devtools/security-check.py /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/bitcoin-cli
/home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/bitcoin-cli: failed CONTROL_FLOW
[2/6] Running security-check.py on bitcoin-tx...
FAILED: src/CMakeFiles/security-check-bitcoin-tx
cd /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src && /usr/bin/python3.9 /home/debian/build/bitcoin/contrib/devtools/security-check.py /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/bitcoin-tx
/home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/bitcoin-tx: failed CONTROL_FLOW
[3/6] Running security-check.py on bitcoin-seeder...
FAILED: src/seeder/CMakeFiles/security-check-bitcoin-seeder
cd /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/seeder && /usr/bin/python3.9 /home/debian/build/bitcoin/contrib/devtools/security-check.py /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/seeder/bitcoin-seeder
/home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/seeder/bitcoin-seeder: failed CONTROL_FLOW
[4/6] Running security-check.py on bitcoin-wallet...
FAILED: src/CMakeFiles/security-check-bitcoin-wallet
cd /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src && /usr/bin/python3.9 /home/debian/build/bitcoin/contrib/devtools/security-check.py /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/bitcoin-wallet
/home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/bitcoin-wallet: failed CONTROL_FLOW
[5/6] Running security-check.py on bitcoind...
FAILED: src/CMakeFiles/security-check-bitcoind
cd /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src && /usr/bin/python3.9 /home/debian/build/bitcoin/contrib/devtools/security-check.py /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/bitcoind
/home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/bitcoind: failed CONTROL_FLOW
[6/6] Running security-check.py on bitcoin-qt...
FAILED: src/qt/CMakeFiles/security-check-bitcoin-qt
cd /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/qt && /usr/bin/python3.9 /home/debian/build/bitcoin/contrib/devtools/security-check.py /home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/qt/BitcoinABC-Qt.app/Contents/MacOS/BitcoinABC-Qt
/home/debian/build/bitcoin/distsrc-x86_64-apple-darwin/src/qt/BitcoinABC-Qt.app/Contents/MacOS/BitcoinABC-Qt: failed CONTROL_FLOW
ninja: build stopped: subcommand failed.

Fabien requested changes to this revision.Jun 2 2023, 13:26

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/CMakeLists.txt
191 ↗	(On Diff #40542)	This is duplicating the -Wstack-protector flag. Also my understanding is that the -fcf-protection is only valid for x86_64 darwin, but this will turn it on for all the builds which would not work ?

PiRK added inline comments.Jun 2 2023, 13:44

src/CMakeLists.txt
191 ↗	(On Diff #40542)	There are other PRs later that add -fcf-protection for the other architectures. But right now I can't make it work for any of them, so 'ill drop this part of the diff so I can unblock the dependency that fixes symbol check.

drop the control flow backports, it does not work and should be done in a separate diff

Harbormaster completed remote builds in B23875: Diff 40551.Jun 2 2023, 13:53

@bot gitian-osx gitian-linux gitian-win

teamcity updated a token.Jun 2 2023, 13:58

Fabien accepted this revision.Jun 2 2023, 16:06

This revision is now accepted and ready to land.Jun 2 2023, 16:06

Closed by commit rABC2b7e01f50b15: misc updates security-check and symbol-check (authored by PiRK). · Explain WhyJun 3 2023, 08:22

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC2b7e01f50b15: misc updates security-check and symbol-check.

misc updates security-check and symbol-check
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 40551

contrib/devtools/security-check.py

contrib/devtools/test-security-check.py

contrib/devtools/test-symbol-check.py

misc updates security-check and symbol-checkClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 40551

contrib/devtools/security-check.py

contrib/devtools/test-security-check.py

contrib/devtools/test-symbol-check.py

misc updates security-check and symbol-check
ClosedPublic
Actions

Revision Contents
Changeset List