guix: Test security-check sanity before performing them

dongcarl commented at 7:07 pm on January 21, 2021: member

These changes allow us to make use of the test-security-check target to check the sanity of our security/symbol checking suite before running them.

dongcarl added the label Build system on Jan 21, 2021

dongcarl added the label Needs Guix build on Jan 21, 2021

in contrib/devtools/security-check.py:20 in 460b697708 outdated

17 import pixie
18 
19-OBJDUMP_CMD = os.getenv('OBJDUMP', '/usr/bin/objdump')
20-OTOOL_CMD = os.getenv('OTOOL', '/usr/bin/otool')
21+OBJDUMP_CMD = os.getenv('OBJDUMP', shutil.which('objdump'))
22+OTOOL_CMD = os.getenv('OTOOL', shutil.which('otool'))

fanquake commented at 4:26 am on January 22, 2021:

If you are going to change these, you’ll have to fixup the mypy issues (causing the lint job to fail):

0contrib/devtools/symbol-check.py:198: error: List item 0 has incompatible type "Optional[str]"; expected "Union[bytes, str, _PathLike[Any]]"
1contrib/devtools/symbol-check.py:219: error: List item 0 has incompatible type "Optional[str]"; expected "Union[bytes, str, _PathLike[Any]]"
2Found 2 errors in 1 file (checked 189 source files)
3^---- failure generated from test/lint/lint-python.sh

It’s unhappy because we’ve got an Optional[str] (from shutil.which) being added into the first argument of subprocess.Popen.

dongcarl commented at 8:15 pm on January 22, 2021:

Not sure how best to fix…

Naive fix:

 0diff --git a/contrib/devtools/symbol-check.py b/contrib/devtools/symbol-check.py
 1index 52f04e8cdf..e85f5b5fd4 100755
 2--- a/contrib/devtools/symbol-check.py
 3+++ b/contrib/devtools/symbol-check.py
 4@@ -52,9 +52,9 @@ IGNORE_EXPORTS = {
 5 '_edata', '_end', '__end__', '_init', '__bss_start', '__bss_start__', '_bss_end__', '__bss_end__', '_fini', '_IO_stdin_used', 'stdin', 'stdout', 'stderr',
 6 'environ', '_environ', '__environ',
 7 }
 8-CPPFILT_CMD = os.getenv('CPPFILT', shutil.which('c++filt'))
 9-OBJDUMP_CMD = os.getenv('OBJDUMP', shutil.which('objdump'))
10-OTOOL_CMD = os.getenv('OTOOL', shutil.which('otool'))
11+CPPFILT_CMD = os.getenv('CPPFILT', shutil.which('c++filt')) # type: ignore[list-item]
12+OBJDUMP_CMD = os.getenv('OBJDUMP', shutil.which('objdump')) # type: ignore[list-item]
13+OTOOL_CMD = os.getenv('OTOOL', shutil.which('otool')) # type: ignore[list-item]
14
15 # Allowed NEEDED libraries
16 ELF_ALLOWED_LIBRARIES = {

in Makefile.am:373 in 460b697708 outdated

374+	$(AM_V_at) OBJDUMP=$(OBJDUMP) $(PYTHON) $(top_srcdir)/contrib/devtools/test-symbol-check.py TestSymbolChecks.test_PE
375 endif
376 if TARGET_LINUX
377-	$(AM_V_at) $(PYTHON) $(top_srcdir)/contrib/devtools/test-security-check.py TestSecurityChecks.test_ELF
378-	$(AM_V_at) $(PYTHON) $(top_srcdir)/contrib/devtools/test-symbol-check.py TestSymbolChecks.test_ELF
379+	$(AM_V_at) OBJDUMP=$(OBJDUMP) OTOOL=$(OTOOL) $(PYTHON) $(top_srcdir)/contrib/devtools/test-security-check.py TestSecurityChecks.test_ELF

fanquake commented at 4:28 am on January 22, 2021:

The Linux security checks don’t need either objdump, or otool (we are using pixie), so why set them here? Same for macOS not needing objdump and PE not needing otool.

dongcarl commented at 8:15 pm on January 22, 2021:

Fixed!

in contrib/guix/libexec/build.sh:211 in 460b697708 outdated

207@@ -208,6 +208,8 @@ mkdir -p "$DISTSRC"
208     # Build Bitcoin Core
209     make --jobs="$MAX_JOBS" ${V:+V=1}
210 
211+    # Check that ELF security checks tools are sane

fanquake commented at 4:37 am on January 22, 2021:

nit: could drop ELF from here and below. As the security (and symbol) checks are across all platforms.

dongcarl commented at 8:14 pm on January 22, 2021:

Fixed!

fanquake commented at 4:38 am on January 22, 2021: member

Concept ACK on running the test-security-check target.

Could link to #18629 in 02706aa470466526dfdd041bc281b9b586743ebd for some additional context.

Can you also rebase on master now that macOS support is merged.

dongcarl force-pushed on Jan 22, 2021

dongcarl added this to the "Next (Not based on any other PRs)" column in a project

practicalswift commented at 11:11 am on January 26, 2021: contributor

Concept ACK on sanity checking test before testing

DrahtBot removed the label Needs Guix build on Jan 30, 2021

MarcoFalke commented at 7:22 am on January 30, 2021: member

 0make[1]: Leaving directory '/distsrc-base/distsrc-65f9b3f774df-x86_64-apple-darwin18'
 1+ make test-security-check V=1
 2OTOOL=/bitcoin/depends/x86_64-apple-darwin18/native/bin/x86_64-apple-darwin18-otool /gnu/store/skvjjmxwgy7yjn1jyc5w6z6lmjs6rsjb-profile/bin/python3.7 ./contrib/devtools/test-security-check.py TestSecurityChecks.test_MACHO
 3ld: unrecognized -a option `llow_stack_execute'
 4clang-8: error: linker command failed with exit code 1 (use -v to see invocation)
 5E
 6======================================================================
 7ERROR: test_MACHO (__main__.TestSecurityChecks)
 8----------------------------------------------------------------------
 9Traceback (most recent call last):
10  File "./contrib/devtools/test-security-check.py", line 70, in test_MACHO
11    self.assertEqual(call_security_check(cc, source, executable, ['-Wl,-no_pie','-Wl,-flat_namespace','-Wl,-allow_stack_execute','-fno-stack-protector']),
12  File "./contrib/devtools/test-security-check.py", line 23, in call_security_check
13    subprocess.run([cc,source,'-o',executable] + options, check=True)
14  File "/gnu/store/hhi58l8s977qv3rvsvs7s9njzy2vpjaa-python-3.7.4/lib/python3.7/subprocess.py", line 487, in run
15    output=stdout, stderr=stderr)
16subprocess.CalledProcessError: Command '['clang', 'test1.c', '-o', 'test1', '-Wl,-no_pie', '-Wl,-flat_namespace', '-Wl,-allow_stack_execute', '-fno-stack-protector']' returned non-zero exit status 1.
17
18----------------------------------------------------------------------
19Ran 1 test in 0.380s
20
21FAILED (errors=1)
22make: *** [Makefile:1429: test-security-check] Error 1

dongcarl force-pushed on Feb 2, 2021

dongcarl force-pushed on Feb 5, 2021

MarcoFalke referenced this in commit ca85449f22 on Feb 8, 2021

sidhujag referenced this in commit f2c9a6f37e on Feb 8, 2021

dongcarl commented at 6:58 pm on February 9, 2021: member

Python nerds: Anyone know why the security-check.py scripts can import pixie but test-security-check.py cannot import a function from my utils.py file?

MarcoFalke commented at 7:03 pm on February 9, 2021: member

Does this help?

 0diff --git a/Makefile.am b/Makefile.am
 1index f6b824faaa..aed44113b8 100644
 2--- a/Makefile.am
 3+++ b/Makefile.am
 4@@ -58,6 +58,7 @@ DIST_SHARE = \
 5 
 6 BIN_CHECKS=$(top_srcdir)/contrib/devtools/symbol-check.py \
 7            $(top_srcdir)/contrib/devtools/security-check.py \
 8+           $(top_srcdir)/contrib/devtools/utils.py \
 9            $(top_srcdir)/contrib/devtools/pixie.py
10 
11 WINDOWS_PACKAGING = $(top_srcdir)/share/pixmaps/bitcoin.ico \

dongcarl force-pushed on Feb 19, 2021

dongcarl force-pushed on Feb 22, 2021

dongcarl force-pushed on Feb 23, 2021

dongcarl commented at 3:09 am on February 23, 2021: member

Pushed 809e14a62e8ed173255d0e56b89c657a2a0c022e → cae518c9a9d1a7faae730246dfc543b77e0e2baa

Rebased on top of master

dongcarl commented at 5:09 pm on February 23, 2021: member

I tried running a build, but it seems to break due to the introduction of: #21255

Logs:

 0CC='x86_64-linux-gnu-gcc' CPPFILT=/gnu/store/3rjpkl6g8iwjis5rrpmgrblk21vz7pgx-profile/bin/x86_64-linux-gnu-c++filt /gnu/store/3rjpkl6g8iwjis5rrpmgrblk21vz7pgx-profile/bin/python3.8 ./contrib/devtools/test-symbol-check.py TestSymbolChecks.test_ELF
 1x86_64-linux-gnu-ld: /tmp/cczw0TSm.o: in function `main':
 2test1.c:(.text+0x1f): undefined reference to `renameat2'
 3collect2: error: ld returned 1 exit status
 4E
 5======================================================================
 6ERROR: test_ELF (__main__.TestSymbolChecks)
 7----------------------------------------------------------------------
 8Traceback (most recent call last):
 9  File "./contrib/devtools/test-symbol-check.py", line 47, in test_ELF
10    self.assertEqual(call_symbol_check(cc, source, executable, []),
11  File "./contrib/devtools/test-symbol-check.py", line 15, in call_symbol_check
12    subprocess.run([*cc,source,'-o',executable] + options, check=True)
13  File "/gnu/store/jki2m0s42hzjfppdqdc7j3y4qlzawcl0-python-3.8.2/lib/python3.8/subprocess.py", line 512, in run
14    raise CalledProcessError(retcode, process.args,
15subprocess.CalledProcessError: Command '['x86_64-linux-gnu-gcc', 'test1.c', '-o', 'test1']' returned non-zero exit status 1.
16
17----------------------------------------------------------------------
18Ran 1 test in 0.091s
19
20FAILED (errors=1)
21make: *** [Makefile:1439: test-security-check] Error 1

I think I know why: I constructed all of the guix cross-compilation toolchain to be glibc 2.27 based, which means that instead of the symbol check failing, it’ll straight up not compile… Not sure what to do here, thoughts? @fanquake

fanquake commented at 11:51 pm on February 23, 2021: member

Spoke with Carl and the solution here is to move the Guix cross-compilation toolchain to be glibc 2.31 based.

dongcarl moved this from the "Next (Not based on any other PRs)" to the "PRs" column in a project

fanquake referenced this in commit a28c053c88 on Mar 2, 2021

dongcarl force-pushed on Mar 2, 2021

dongcarl commented at 10:49 pm on March 2, 2021: member

Pushed cae518c9a9d1a7faae730246dfc543b77e0e2baa -> db6e91a5cdbd681f51d1eae4e266164cc602d841

Rebased over master
Use binutils disable flag patch from debian upstream for binutils 2.34
Add commit to use/test --reloc-section

 0b2dc314c882ba3dd119c44ed2673b1efe759f94ffd09a1f303a3bedc111cd39c  output/bitcoin-db6e91a5cdbd-aarch64-linux-gnu-debug.tar.gz
 1a0ae4738bd48c9cad43e4d45cfd3247462a96f1a2558bd27f81a7c44a8213883  output/bitcoin-db6e91a5cdbd-aarch64-linux-gnu.tar.gz
 27b189a772cf0eb0911f137780b16c6e3bf12cd7663f7c03be03b4450797210dd  output/bitcoin-db6e91a5cdbd-arm-linux-gnueabihf-debug.tar.gz
 340d3a6255484761e899a9ce75c35b0bb03d0612a6b80b27ac3910e28e5ca48dd  output/bitcoin-db6e91a5cdbd-arm-linux-gnueabihf.tar.gz
 4e70e92ce37132641b66a99a53716b3b66e61e0b096ecfee6d321a56a64a850d3  output/bitcoin-db6e91a5cdbd-osx-unsigned.dmg
 5d5ea424fd1083878e95cb6b7c09a6b0b3e716b8a0a6e37c403864fe99ec9477a  output/bitcoin-db6e91a5cdbd-osx-unsigned.tar.gz
 68254778671c315aec66dbcfc020ff19bbf6070d61a1bb5d5880ac3e3c3ef8681  output/bitcoin-db6e91a5cdbd-osx64.tar.gz
 7a4dafcf884c89fca24109946e66f5c411d1f81154279ac1013a0a69d41b2650f  output/bitcoin-db6e91a5cdbd-powerpc64-linux-gnu-debug.tar.gz
 802934a669612312b461e3d66623bfeeeb17088a173650db15ff8fc52eec529c4  output/bitcoin-db6e91a5cdbd-powerpc64-linux-gnu.tar.gz
 9f7de27d951003d632dd19447c13b96e575759380d1d15fb0c1d7272cc963b074  output/bitcoin-db6e91a5cdbd-powerpc64le-linux-gnu-debug.tar.gz
1016a6cc048e04ea59e58855cdfb9fa653eab941e47e1edf4c4abc12edadd25b7d  output/bitcoin-db6e91a5cdbd-powerpc64le-linux-gnu.tar.gz
114a72908757e2ea4e9d5c9051b92e7cd10ab7193cae902d1631c364e78b03810b  output/bitcoin-db6e91a5cdbd-riscv64-linux-gnu-debug.tar.gz
123a26eecf0da5ed66c8fad9d13ffd342f2d7492878cbf3699056d1f3ebae3ee43  output/bitcoin-db6e91a5cdbd-riscv64-linux-gnu.tar.gz
1399da16a244e8711c8b3a340c71b40f4f41248410629060d2fa59b2366cf7a41b  output/bitcoin-db6e91a5cdbd-win-unsigned.tar.gz
14214c02c42f0932c988c0112762e9ee55e66b697a68fd22109aec89478b88a8ab  output/bitcoin-db6e91a5cdbd-win64-debug.zip
159b9810ed2fe6cf74d134618fcb6661184d8025902d82f4f8c10bd920a0c32e26  output/bitcoin-db6e91a5cdbd-win64-setup-unsigned.exe
16059722aa12c33aec749c64a2a6ae395ebc4f62fe6e76d5c36c82437b17d1b627  output/bitcoin-db6e91a5cdbd-win64.zip
17658256d2594448715944463d345cb3b3db3f55e1d5152dbfcaa278b247cd0fcc  output/bitcoin-db6e91a5cdbd-x86_64-linux-gnu-debug.tar.gz
18bf8c5725bd2b475172dcda7d30d8e283612a776115101429753ade9ae5085576  output/bitcoin-db6e91a5cdbd-x86_64-linux-gnu.tar.gz
195020065aef12af03f056cb2810a74a646618a3d0b98a49e0a48d98808d8616e1  output/src/bitcoin-db6e91a5cdbd.tar.gz

MarcoFalke added the label Needs Guix build on Mar 3, 2021

DrahtBot commented at 10:06 am on March 3, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21515 by naumenkogs

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.

dongcarl moved this from the "PRs" to the "Next (Not based on any other PRs)" column in a project

in configure.ac:908 in db6e91a5cd outdated

883@@ -884,6 +884,7 @@ if test x$use_hardening != xno; then
884     ])
885   fi
886 
887+  AX_CHECK_LINK_FLAG([[-Wl,--enable-reloc-section]], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,--enable-reloc-section"],, [[$LDFLAG_WERROR]])

fanquake commented at 1:50 am on March 4, 2021:

In d3e6ee6439f98c3676ce1909a5218549cbfd84d8. I think testing for this, and adding to our hardened ldflags when available is fine. It’s enabled by default, however we like to be explicit. It’s also available with the binutils (2.34) we are using for gitian builds.

Note that some of these flags also imply each other: --high-entropy-va implies --dynamic-base & --enable-reloc-section --dynamic-base implies --enable-reloc-section ``

in contrib/guix/patches/binutils-mingw-w64-disable-flags.patch:1 in db6e91a5cd outdated

0@@ -0,0 +1,171 @@
1+Description: Add disable opposites to the security-related flags

fanquake commented at 2:00 am on March 4, 2021:

Checked that this matches https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/blob/master/debian/patches/disable-flags.patch bar whitespace changes.

in contrib/devtools/test-security-check.py:62 in db6e91a5cd outdated

49@@ -47,24 +50,26 @@ def test_ELF(self):
50     def test_PE(self):
51         source = 'test1.c'
52         executable = 'test1.exe'
53-        cc = 'x86_64-w64-mingw32-gcc'
54+        cc = determine_wellknown_cmd('CC', 'x86_64-w64-mingw32-gcc')
55         write_testcode(source)
56 
57-        self.assertEqual(call_security_check(cc, source, executable, ['-Wl,--no-nxcompat','-Wl,--no-dynamicbase','-Wl,--no-high-entropy-va','-no-pie','-fno-PIE']),
58+        self.assertEqual(call_security_check(cc, source, executable, ['-Wl,--no-nxcompat','-Wl,--disable-reloc-section','-Wl,--no-dynamicbase','-Wl,--no-high-entropy-va','-no-pie','-fno-PIE']),

fanquake commented at 2:04 am on March 4, 2021:

At this stage we have already given in to not being able to run the test security check target for windows in gitian due to lack of --no options in ld, so adding --disable here to test --enable-reloc-section isn’t making anything worse. If anything this speaks to the usefulness of Guix, given how easy it is to patch these --no/--disable flags back into our toolchain. It would be much more difficult trying to achieve the same using gitian.

fanquake commented at 2:13 am on March 4, 2021: member

I think this looks pretty good now. Going to run some builds.

Can you exclude contrib/guix/patches/ from codespell so we don’t have to deal with this:

0contrib/guix/patches/binutils-mingw-w64-disable-flags.patch:61: SEH ==> SHE
1contrib/guix/patches/binutils-mingw-w64-disable-flags.patch:145: SEH ==> SHE
2^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt

 0diff --git a/test/lint/lint-spelling.sh b/test/lint/lint-spelling.sh
 1index fbdf3c59c..238fa63c4 100755
 2--- a/test/lint/lint-spelling.sh
 3+++ b/test/lint/lint-spelling.sh
 4@@ -15,6 +15,6 @@ if ! command -v codespell > /dev/null; then
 5 fi
 6 
 7 IGNORE_WORDS_FILE=test/lint/lint-spelling.ignore-words.txt
 8-if ! codespell --check-filenames --disable-colors --quiet-level=7 --ignore-words=${IGNORE_WORDS_FILE} $(git ls-files -- ":(exclude)build-aux/m4/" ":(exclude)contrib/seeds/*.txt" ":(exclude)depends/" ":(exclude)doc/release-notes/" ":(exclude)src/leveldb/" ":(exclude)src/crc32c/" ":(exclude)src/qt/locale/" ":(exclude)src/qt/*.qrc" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/" ":(exclude)contrib/gitian-keys/keys.txt"); then
 9+if ! codespell --check-filenames --disable-colors --quiet-level=7 --ignore-words=${IGNORE_WORDS_FILE} $(git ls-files -- ":(exclude)build-aux/m4/" ":(exclude)contrib/seeds/*.txt" ":(exclude)depends/" ":(exclude)doc/release-notes/" ":(exclude)src/leveldb/" ":(exclude)src/crc32c/" ":(exclude)src/qt/locale/" ":(exclude)src/qt/*.qrc" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/" ":(exclude)contrib/gitian-keys/keys.txt" ":(exclude)contrib/guix/patches"); then
10     echo "^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in ${IGNORE_WORDS_FILE}"

fanquake commented at 3:56 am on March 4, 2021: member

One transient failure while building:

 0substitution of /gnu/store/04qddg51ih327yc8p7q2vn00slg4v1n9-gcc-cross-x86_64-w64-mingw32-9.3.0-lib complete
 1 binutils-cross-x86_64-w64-mingw32-2.34                                            18.4MiB/s 00:01 | 27.1MiB transferred
 2
 3downloading from https://guix.carldong.io/nar/gzip/r7kbdcmb1w4is2bwjxx8jqy9fpb9pa9b-ld-wrapper-x86_64-w64-mingw32-0 ...
 4 ld-wrapper-x86_64-w64-mingw32-0                                                      6.1MiB/s 00:00 | 19KiB transferred
 5
 6Backtrace:
 7In guix/ui.scm:
 8  2164:12 19 (run-guix-command _ . _)
 9In guix/scripts/substitute.scm:
10    931:2 18 (guix-substitute . _)
11In unknown file:
12          17 (with-continuation-barrier #<procedure thunk ()>)
13In ice-9/boot-9.scm:
14  1736:10 16 (with-exception-handler _ _ #:unwind? _ # _)
15In unknown file:
16          15 (apply-smob/0 #<thunk 7f1a2032fdc0>)
17In ice-9/boot-9.scm:
18  1736:10 14 (with-exception-handler _ _ #:unwind? _ # _)
19  1736:10 13 (with-exception-handler _ _ #:unwind? _ # _)
20  1731:15 12 (with-exception-handler #<procedure 7f1a1dd610f0 at ic?> ?)
21In guix/scripts/substitute.scm:
22   980:17 11 (_)
23    689:7 10 (process-substitution _ "/gnu/store/grb2m42291nkny2vid?" ?)
24In ice-9/boot-9.scm:
25  1736:10  9 (with-exception-handler _ _ #:unwind? _ # _)
26In guix/scripts/substitute.scm:
27    698:9  8 (_)
28In ice-9/boot-9.scm:
29  1731:15  7 (with-exception-handler #<procedure 7f1a1ecd18a0 at ic?> ?)
30  1669:16  6 (raise-exception _ #:continuable? _)
31  1667:16  5 (raise-exception _ #:continuable? _)
32  1669:16  4 (raise-exception _ #:continuable? _)
33  1764:13  3 (_ #<&compound-exception components: (#<&error> #<&irri?>)
34  1669:16  2 (raise-exception _ #:continuable? _)
35  1667:16  1 (raise-exception _ #:continuable? _)
36  1669:16  0 (raise-exception _ #:continuable? _)
37
38ice-9/boot-9.scm:1669:16: In procedure raise-exception:
39Bad http-version header component: K?%s-??
40
41Backtrace:
42           1 (primitive-load "/gnu/store/lvp5s8l0zwkrn2a0mmh6wf6z9ja?")
43In guix/ui.scm:
44  2164:12  0 (run-guix-command _ . _)
45
46guix/ui.scm:2164:12: In procedure run-guix-command:
47Bad http-version header component: K?%s-??
48
49substitution of /gnu/store/grb2m42291nkny2vid35w7xrgirkxnrk-gcc-cross-x86_64-w64-mingw32-9.3.0 failed
50guix environment: error: some substitutes for the outputs of derivation `/gnu/store/71f0wbcm7v2kbs3jfxjp44a7gx2iz66q-gcc-cross-x86_64-w64-mingw32-9.3.0.drv' failed (usually happens due to networking issues); try `--fallback' to build derivation from source

but it looks like I’ve got matches except for output/bitcoin-db6e91a5cdbd-osx-unsigned.tar.gz :

 0find output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
 1b2dc314c882ba3dd119c44ed2673b1efe759f94ffd09a1f303a3bedc111cd39c  output/bitcoin-db6e91a5cdbd-aarch64-linux-gnu-debug.tar.gz
 2a0ae4738bd48c9cad43e4d45cfd3247462a96f1a2558bd27f81a7c44a8213883  output/bitcoin-db6e91a5cdbd-aarch64-linux-gnu.tar.gz
 37b189a772cf0eb0911f137780b16c6e3bf12cd7663f7c03be03b4450797210dd  output/bitcoin-db6e91a5cdbd-arm-linux-gnueabihf-debug.tar.gz
 440d3a6255484761e899a9ce75c35b0bb03d0612a6b80b27ac3910e28e5ca48dd  output/bitcoin-db6e91a5cdbd-arm-linux-gnueabihf.tar.gz
 5e70e92ce37132641b66a99a53716b3b66e61e0b096ecfee6d321a56a64a850d3  output/bitcoin-db6e91a5cdbd-osx-unsigned.dmg
 618b8f49e36a35f7caeb7e2c34410884bd9e20e3dd4c875afe7202610918c1084  output/bitcoin-db6e91a5cdbd-osx-unsigned.tar.gz
 78254778671c315aec66dbcfc020ff19bbf6070d61a1bb5d5880ac3e3c3ef8681  output/bitcoin-db6e91a5cdbd-osx64.tar.gz
 8a4dafcf884c89fca24109946e66f5c411d1f81154279ac1013a0a69d41b2650f  output/bitcoin-db6e91a5cdbd-powerpc64-linux-gnu-debug.tar.gz
 902934a669612312b461e3d66623bfeeeb17088a173650db15ff8fc52eec529c4  output/bitcoin-db6e91a5cdbd-powerpc64-linux-gnu.tar.gz
10f7de27d951003d632dd19447c13b96e575759380d1d15fb0c1d7272cc963b074  output/bitcoin-db6e91a5cdbd-powerpc64le-linux-gnu-debug.tar.gz
1116a6cc048e04ea59e58855cdfb9fa653eab941e47e1edf4c4abc12edadd25b7d  output/bitcoin-db6e91a5cdbd-powerpc64le-linux-gnu.tar.gz
124a72908757e2ea4e9d5c9051b92e7cd10ab7193cae902d1631c364e78b03810b  output/bitcoin-db6e91a5cdbd-riscv64-linux-gnu-debug.tar.gz
133a26eecf0da5ed66c8fad9d13ffd342f2d7492878cbf3699056d1f3ebae3ee43  output/bitcoin-db6e91a5cdbd-riscv64-linux-gnu.tar.gz
1499da16a244e8711c8b3a340c71b40f4f41248410629060d2fa59b2366cf7a41b  output/bitcoin-db6e91a5cdbd-win-unsigned.tar.gz
15214c02c42f0932c988c0112762e9ee55e66b697a68fd22109aec89478b88a8ab  output/bitcoin-db6e91a5cdbd-win64-debug.zip
169b9810ed2fe6cf74d134618fcb6661184d8025902d82f4f8c10bd920a0c32e26  output/bitcoin-db6e91a5cdbd-win64-setup-unsigned.exe
17059722aa12c33aec749c64a2a6ae395ebc4f62fe6e76d5c36c82437b17d1b627  output/bitcoin-db6e91a5cdbd-win64.zip
18658256d2594448715944463d345cb3b3db3f55e1d5152dbfcaa278b247cd0fcc  output/bitcoin-db6e91a5cdbd-x86_64-linux-gnu-debug.tar.gz
19bf8c5725bd2b475172dcda7d30d8e283612a776115101429753ade9ae5085576  output/bitcoin-db6e91a5cdbd-x86_64-linux-gnu.tar.gz
205020065aef12af03f056cb2810a74a646618a3d0b98a49e0a48d98808d8616e1  output/src/bitcoin-db6e91a5cdbd.tar.gz

MarcoFalke deleted a comment on Mar 8, 2021

DrahtBot commented at 10:30 pm on March 8, 2021: member

Guix builds

File	commit 48725e64fbfb85200dd2226386fbf1cfc8fe6c1f(master)	commit 7a5313af9816cfe0cd45e2cb6cb7df953a70da5a(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz	`c0874d1b330fbefe...`	`632ddba8c043bed5...`
*-aarch64-linux-gnu.tar.gz	`aa5812a46cdb9b17...`	`0ce0e390642d9aa4...`
*-arm-linux-gnueabihf-debug.tar.gz	`829d64e019d44cdb...`	`3067b0eed09d5b80...`
*-arm-linux-gnueabihf.tar.gz	`bfe6daf32d21c1ba...`	`b41cb6a932bb7f39...`
*-osx-unsigned.dmg	`994a38a15423c943...`	`bd49332c0c868f7e...`
*-osx-unsigned.tar.gz	`b8ba9767e95854c3...`	`1d3e4edb725ce985...`
*-osx64.tar.gz	`e40a6c3d367faac0...`	`6dea5e06af4861b5...`
*-powerpc64-linux-gnu-debug.tar.gz	`e578666d86258fba...`	`f187d84f384ea751...`
*-powerpc64-linux-gnu.tar.gz	`25cf822c602f666c...`	`a5d57685089eb48e...`
*-powerpc64le-linux-gnu-debug.tar.gz	`ecc42f86e8f789f1...`	`ced1c78834766fc7...`
*-powerpc64le-linux-gnu.tar.gz	`54550694128472dc...`	`5003fa756f6e3dfc...`
*-riscv64-linux-gnu-debug.tar.gz	`a47b303965bb52bf...`	`bd7d78daa8df5d5e...`
*-riscv64-linux-gnu.tar.gz	`fa622fe7877da0bf...`	`a24bd3522aabe6b2...`
*-win-unsigned.tar.gz	`2c86f79a791858e3...`	`d6e6bb03aadb240c...`
*-win64-debug.zip	`49435c78f11fb3c8...`	`277f5fad30fa9060...`
*-win64-setup-unsigned.exe	`0e76b2d89273a43e...`	`d73d1d19db13ffd2...`
*-win64.zip	`d5c10f3570fa1003...`	`e41265750c2c0e04...`
*-x86_64-linux-gnu-debug.tar.gz	`fc1699b9ca5f0d2c...`	`42c7df68f9cc6099...`
*-x86_64-linux-gnu.tar.gz	`92f5688dc334ea8e...`	`39bb985d62a46edd...`
*.tar.gz	`ff440483a5453fd3...`	`d43d9dce7eceabb7...`
guix_build.log	`2070abe8ac15129b...`	`ff55fb944916f5e2...`
guix_build.log.diff		`bd57bbce4d347f59...`

DrahtBot removed the label Needs Guix build on Mar 8, 2021

dongcarl force-pushed on Mar 8, 2021

dongcarl commented at 11:47 pm on March 8, 2021: member

Pushed db6e91a5cdbd681f51d1eae4e266164cc602d841 -> 1cd35ec2c3ed51c5e044d7bf5c932d7f97b5df91

Addressed #20980#pullrequestreview-603630859

dongcarl force-pushed on Mar 8, 2021

dongcarl commented at 11:50 pm on March 8, 2021: member

Pushed 1cd35ec2c3ed51c5e044d7bf5c932d7f97b5df91 -> 05f870018c618167c4740e7d1381e98bd30ae5f9

Rebased over master

One transient failure while building: …

Thanks for your diligent testing ☺️. Apparently we should provide the --fallback flag so that builds don’t stop when a substitution fails. I will add this in a future PR.

hebasto commented at 3:17 pm on March 13, 2021: member

Concept ACK.

hebasto commented at 8:11 pm on March 13, 2021: member

Guix builds:

 0$ find output -type f -name *$(git rev-parse --short HEAD)*.* -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
 1d975454fdfc02b97ed1acb0168e5cacf736f22fc8eacfc9425b7da0ac31bde4d  output/bitcoin-05f870018c61-aarch64-linux-gnu-debug.tar.gz
 2c3542af7161f6daf1aafa9ef7ee8c6fc648d6d22716f9e4d85b7663e38fbdc8a  output/bitcoin-05f870018c61-aarch64-linux-gnu.tar.gz
 3bddeb0cb201f82d0bc41adabf2c4649b7ee185a3edbf7710e99e03adbd9356be  output/bitcoin-05f870018c61-arm-linux-gnueabihf-debug.tar.gz
 4a10e9ba3a1552a290dce18b826a29b009ec607ccc89ad8d100b32e653ce151b1  output/bitcoin-05f870018c61-arm-linux-gnueabihf.tar.gz
 5bc6a51747e154d7382f38e30490bb03cbe909e787fd6ebe1f5d4e78edf402c5c  output/bitcoin-05f870018c61-osx-unsigned.dmg
 605ce2ed4178b1a92bf5ea0854ab5638faf154a79c26a8ef2b93f23fd5aec3729  output/bitcoin-05f870018c61-osx-unsigned.tar.gz
 7a18ae160f80003616a20f24c397070bc77c64ae5252bb243feabc76b1feb8127  output/bitcoin-05f870018c61-osx64.tar.gz
 8ba1fa80c725c7d3fb58a39b056459f65c90e275eaef4ec871f1aefcbca2629bd  output/bitcoin-05f870018c61-powerpc64-linux-gnu-debug.tar.gz
 977d90228ce1d7e5e38bcc52c284724ca3cce7648d057125d745be9d6fe147cba  output/bitcoin-05f870018c61-powerpc64-linux-gnu.tar.gz
102c05d07b554dde94459b8eba2c8b2b50b71f49e3490edd67be4ece0fb7cb0086  output/bitcoin-05f870018c61-powerpc64le-linux-gnu-debug.tar.gz
1133f6f6f7018b451db9caf113b3df8182f8dd172cbfaa2f695013f77344270f19  output/bitcoin-05f870018c61-powerpc64le-linux-gnu.tar.gz
12a5666c3714caae2fe9ed06557a3a9c0b5a3e1ee9747c8fafa5fcb90949020faf  output/bitcoin-05f870018c61-riscv64-linux-gnu-debug.tar.gz
13f3106f91a3402377ce361fa548656accb73f74c44f86405e611157bb72fd3653  output/bitcoin-05f870018c61-riscv64-linux-gnu.tar.gz
149d688ad34555f0bdc66e41850dacc932907be5c44157ebf0bea1330870696901  output/bitcoin-05f870018c61-win-unsigned.tar.gz
1546d1c5c8c2737ee3deb83d7c4ce3c3b0c2d4606e9d156bb534f6ab9c1b5da439  output/bitcoin-05f870018c61-win64-debug.zip
16317804c393af6cf9ad0c62161f747ee508fc06c2232c72a92c6a0b7ad22b1506  output/bitcoin-05f870018c61-win64-setup-unsigned.exe
1759240e72d6bfee0984a6d77a9429b62ff1056860b4827f457649deab77c5b93b  output/bitcoin-05f870018c61-win64.zip
18be010f6c5e35e46b8605b7d3e5d4c37f64ba8f5f6bcd75dbccbe0a2d151c2e1a  output/bitcoin-05f870018c61-x86_64-linux-gnu-debug.tar.gz
198ac323bd51ae9a2aa856d39f7f571829b7e280b147bf223063dd2b7e249a030b  output/bitcoin-05f870018c61-x86_64-linux-gnu.tar.gz
20e22ff03b8e17f6afb7c1433ce7af75ead3ae482fccbfba00bb37d76286b4efe7  output/src/bitcoin-05f870018c61.tar.gz

in contrib/devtools/symbol-check.py:55 in 05f870018c outdated

50@@ -51,9 +51,10 @@
51 '_edata', '_end', '__end__', '_init', '__bss_start', '__bss_start__', '_bss_end__', '__bss_end__', '_fini', '_IO_stdin_used', 'stdin', 'stdout', 'stderr',
52 'environ', '_environ', '__environ',
53 }
54-CPPFILT_CMD = os.getenv('CPPFILT', '/usr/bin/c++filt')
55-OBJDUMP_CMD = os.getenv('OBJDUMP', '/usr/bin/objdump')
56-OTOOL_CMD = os.getenv('OTOOL', '/usr/bin/otool')
57+
58+CPPFILT_CMD = lambda: determine_wellknown_cmd('CPPFILT', 'c++filt')

laanwj commented at 9:22 am on March 15, 2021:

What is the rationale for making these lambdas instead of evaluating them here and now, once? We don’t expect anything to change over the course of the program do we?

dongcarl commented at 4:10 pm on March 15, 2021:

In order to appease the python type linter, I made determine_wellknown_cmd simply error out if the program is not found

https://github.com/bitcoin/bitcoin/blob/05f870018c618167c4740e7d1381e98bd30ae5f9/contrib/devtools/utils.py#L14-L22

Therefore, if we were to evaluate these immediately, and the tool does not exist in the environment (e.g. otool won’t exist in a build container for Linux), then the script will just crash.

I’m more than happy to adopt some other, better solution. This was just a solution I found which works

laanwj commented at 1:55 pm on March 16, 2021:

I personally think using lambdas here is not a nice construction. If it is only ’to appease the type linter’, please add an ignore pragma # type: ignore. Our purpose for linters is to make the code better not unnecessarily verbose :smile:

DrahtBot added the label Needs rebase on Mar 18, 2021

jarolrod commented at 6:31 am on April 14, 2021: member

I know this needs a rebase, but contributing GUIX hashes for 05f870018c618167c4740e7d1381e98bd30ae5f9. Mine match hebasto:

 0find output -type f -name *$(git rev-parse --short HEAD)*.* -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
 1
 2d975454fdfc02b97ed1acb0168e5cacf736f22fc8eacfc9425b7da0ac31bde4d  output/bitcoin-05f870018c61-aarch64-linux-gnu-debug.tar.gz
 3c3542af7161f6daf1aafa9ef7ee8c6fc648d6d22716f9e4d85b7663e38fbdc8a  output/bitcoin-05f870018c61-aarch64-linux-gnu.tar.gz
 4bddeb0cb201f82d0bc41adabf2c4649b7ee185a3edbf7710e99e03adbd9356be  output/bitcoin-05f870018c61-arm-linux-gnueabihf-debug.tar.gz
 5a10e9ba3a1552a290dce18b826a29b009ec607ccc89ad8d100b32e653ce151b1  output/bitcoin-05f870018c61-arm-linux-gnueabihf.tar.gz
 6bc6a51747e154d7382f38e30490bb03cbe909e787fd6ebe1f5d4e78edf402c5c  output/bitcoin-05f870018c61-osx-unsigned.dmg
 705ce2ed4178b1a92bf5ea0854ab5638faf154a79c26a8ef2b93f23fd5aec3729  output/bitcoin-05f870018c61-osx-unsigned.tar.gz
 8a18ae160f80003616a20f24c397070bc77c64ae5252bb243feabc76b1feb8127  output/bitcoin-05f870018c61-osx64.tar.gz
 9ba1fa80c725c7d3fb58a39b056459f65c90e275eaef4ec871f1aefcbca2629bd  output/bitcoin-05f870018c61-powerpc64-linux-gnu-debug.tar.gz
1077d90228ce1d7e5e38bcc52c284724ca3cce7648d057125d745be9d6fe147cba  output/bitcoin-05f870018c61-powerpc64-linux-gnu.tar.gz
112c05d07b554dde94459b8eba2c8b2b50b71f49e3490edd67be4ece0fb7cb0086  output/bitcoin-05f870018c61-powerpc64le-linux-gnu-debug.tar.gz
1233f6f6f7018b451db9caf113b3df8182f8dd172cbfaa2f695013f77344270f19  output/bitcoin-05f870018c61-powerpc64le-linux-gnu.tar.gz
13a5666c3714caae2fe9ed06557a3a9c0b5a3e1ee9747c8fafa5fcb90949020faf  output/bitcoin-05f870018c61-riscv64-linux-gnu-debug.tar.gz
14f3106f91a3402377ce361fa548656accb73f74c44f86405e611157bb72fd3653  output/bitcoin-05f870018c61-riscv64-linux-gnu.tar.gz
159d688ad34555f0bdc66e41850dacc932907be5c44157ebf0bea1330870696901  output/bitcoin-05f870018c61-win-unsigned.tar.gz
1646d1c5c8c2737ee3deb83d7c4ce3c3b0c2d4606e9d156bb534f6ab9c1b5da439  output/bitcoin-05f870018c61-win64-debug.zip
17317804c393af6cf9ad0c62161f747ee508fc06c2232c72a92c6a0b7ad22b1506  output/bitcoin-05f870018c61-win64-setup-unsigned.exe
1859240e72d6bfee0984a6d77a9429b62ff1056860b4827f457649deab77c5b93b  output/bitcoin-05f870018c61-win64.zip
19be010f6c5e35e46b8605b7d3e5d4c37f64ba8f5f6bcd75dbccbe0a2d151c2e1a  output/bitcoin-05f870018c61-x86_64-linux-gnu-debug.tar.gz
208ac323bd51ae9a2aa856d39f7f571829b7e280b147bf223063dd2b7e249a030b  output/bitcoin-05f870018c61-x86_64-linux-gnu.tar.gz
21e22ff03b8e17f6afb7c1433ce7af75ead3ae482fccbfba00bb37d76286b4efe7  output/src/bitcoin-05f870018c61.tar.gz```

dongcarl force-pushed on Apr 28, 2021

dongcarl commented at 7:54 pm on April 28, 2021: member

Now based on: #21664

My testing of the new LIEF check outputs:

 0(Pdb) call_security_check(cc, source, executable, ['-Wl,--nxcompat','-Wl,--enable-reloc-section','-Wl,--no-dynamicbase','-Wl,--no-high-entropy-va','-no-pie','-fno-PIE'])
 1(1, 'test1.exe: failed PIE DYNAMIC_BASE HIGH_ENTROPY_VA')
 2(Pdb) call_security_check(cc, source, executable, ['-Wl,--nxcompat','-Wl,--enable-reloc-section','-Wl,--no-dynamicbase','-Wl,--no-high-entropy-va','-pie','-fPIE'])
 3(1, 'test1.exe: failed PIE DYNAMIC_BASE HIGH_ENTROPY_VA')
 4(Pdb) call_security_check(cc, source, executable, ['-Wl,--no-nxcompat','-Wl,--disable-reloc-section','-Wl,--no-dynamicbase','-Wl,--no-high-entropy-va','-no-pie','-fno-PIE'])
 5(1, 'test1.exe: failed PIE DYNAMIC_BASE HIGH_ENTROPY_VA NX RELOC_SECTION')
 6(Pdb) call_security_check(cc, source, executable, ['-Wl,--no-nxcompat','-Wl,--disable-reloc-section','-Wl,--no-dynamicbase','-Wl,--no-high-entropy-va','-pie','-fPIE'])
 7(1, 'test1.exe: failed PIE DYNAMIC_BASE HIGH_ENTROPY_VA NX')
 8(Pdb) call_security_check(cc, source, executable, ['-Wl,--no-nxcompat','-Wl,--disable-reloc-section','-Wl,--dynamicbase','-Wl,--no-high-entropy-va','-no-pie','-fno-PIE'])
 9(1, 'test1.exe: failed HIGH_ENTROPY_VA NX')
10(Pdb) call_security_check(cc, source, executable, ['-Wl,--no-nxcompat','-Wl,--enable-reloc-section','-Wl,--no-dynamicbase','-Wl,--no-high-entropy-va','-no-pie','-fno-PIE'])
11(1, 'test1.exe: failed PIE DYNAMIC_BASE HIGH_ENTROPY_VA NX')

Seems that turning on pie forces reloc-section to be turned on, and turning on dynamic-base forces PIE to be turned on. However, turning on PIE without turning on dynamic-base does nothing at all.

DrahtBot removed the label Needs rebase on Apr 28, 2021

DrahtBot commented at 5:25 pm on May 3, 2021: member

🕵️ @sipa @achow101 have been requested to review this pull request as specified in the REVIEWERS file.

dongcarl force-pushed on May 3, 2021

DrahtBot added the label Needs rebase on May 4, 2021

dongcarl force-pushed on May 7, 2021

dongcarl commented at 7:34 pm on May 7, 2021: member

Pushed 69c650b13a…4914a082a4

Rebased over master now that #21664 is merged.

DrahtBot removed the label Needs rebase on May 7, 2021

hebasto commented at 12:41 pm on May 9, 2021: member

https://cirrus-ci.com/task/5154764032835584?logs=lint#L855

0contrib/devtools/symbol-check.py:15:1: F401 'os' imported but unused
1contrib/devtools/test-security-check.py:10:1: F401 'typing.List' imported but unused
2Success: no issues found in 201 source files
3^---- failure generated from test/lint/lint-python.sh

in configure.ac:908 in 4914a082a4 outdated

910@@ -911,6 +911,7 @@ if test x$use_hardening != xno; then
911     ])
912   fi
913 
914+  AX_CHECK_LINK_FLAG([[-Wl,--enable-reloc-section]], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,--enable-reloc-section"],, [[$LDFLAG_WERROR]])

hebasto commented at 1:08 pm on May 9, 2021:

style nit: I know it follows the surrounding style, but the double quoting is really unneeded here:

0  AX_CHECK_LINK_FLAG([-Wl,--enable-reloc-section], [HARDENED_LDFLAGS="$HARDENED_LDFLAGS -Wl,--enable-reloc-section"], [], [$LDFLAG_WERROR])

hebasto commented at 1:09 pm on May 9, 2021: member

Approach ACK 4914a082a4991b2cfee2e934ebabdb890f65d3de

The commit 8a833f32814f35e76439494b6025bd360a7fad58 “devtools: Improve *-check.py tool detection” is broken without the 777eae35ff96174ee3954dce6e02ed854e3df535 “devtools: Pass make $(CC) into test-*-check.py”:

0$ test/lint/lint-python.sh 
1contrib/devtools/test-security-check.py:12:1: F401 'utils.determine_wellknown_cmd' imported but unused
2contrib/devtools/test-symbol-check.py:12:1: F401 'utils.determine_wellknown_cmd' imported but unused
3contrib/devtools/test-symbol-check.py:14:27: F821 undefined name 'List'
4contrib/devtools/test-symbol-check.py:14: error: Name 'List' is not defined
5contrib/devtools/test-symbol-check.py:14: note: Did you forget to import it from "typing"? (Suggestion: "from typing import List")
6Found 1 error in 1 file (checked 201 source files)

Maybe combine them, or reorder changes?

dongcarl force-pushed on May 12, 2021

lint: Run mypy with --show-error-codes

When using mypy ignore directives, the error code needs to be specified.
Somehow mypy doesn't print it by default...

91142806cc

devtools: Improve *-check.py tool detection

This is important to make sure that we're not testing tools different
from the one we're building with.

Introduce determine_wellknown_cmd, which encapsulates how we
should handle well-known tools specification (IFS splitting, env
override, etc.).

d476bfcb4a

guix: Patch binutils to add security-related disable flags

We use these flags in our test-security-check make target, but they are
only available because debian patches them in.

We can patch them in for our Guix builds so that we can check the sanity
of our security/symbol checking suite before running them.

09bc25db64

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

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

d1d20ae917

guix: Test security-check sanity before performing them d9a3d3255d

dongcarl force-pushed on May 12, 2021

dongcarl commented at 7:48 pm on May 12, 2021: member

Pushed 4914a082a4991b2cfee2e934ebabdb890f65d3de -> d9a3d3255d3565181f2e3a7c450aa01ffa8d3163

Squashed a few commits which no longer make sense to be separate
Rebased over master
Addressed: #20980#pullrequestreview-655077115

hebasto commented at 9:33 am on May 13, 2021: member

Testing this PR together with #21871 reveals some kind of incompatibility.

fanquake added this to the milestone 22.0 on Jul 1, 2021

fanquake commented at 8:43 am on July 1, 2021: member

I am fixing the macOS issues with test-security-check, so this can be part of 22.0.

fanquake commented at 1:03 pm on July 1, 2021: member

I’ve opened #22381, which is a rebased version of this, with the macOS fix.

fanquake closed this on Jul 1, 2021

nolim1t referenced this in commit 34d1d6a112 on Jul 9, 2021

fanquake moved this from the "Next (Not based on any other PRs)" to the "Done" column in a project

UdjinM6 referenced this in commit 43ee2ef541 on Oct 23, 2021

UdjinM6 referenced this in commit 1bfb6b9543 on Oct 23, 2021

UdjinM6 referenced this in commit 76528449b5 on Oct 23, 2021

UdjinM6 referenced this in commit 6c797b13e8 on Dec 4, 2021

DrahtBot locked this on Aug 16, 2022

guix: Test security-check sanity before performing them #20980

Conflicts

Guix builds