Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Build with -ffunction-sections -fdata-sections -Wl,--gc-sections ? #18579

Closed
sipa opened this issue Apr 9, 2020 · 8 comments
Closed

Build with -ffunction-sections -fdata-sections -Wl,--gc-sections ? #18579

sipa opened this issue Apr 9, 2020 · 8 comments
Labels
Build system

Comments

@sipa
Copy link
Member

sipa commented Apr 9, 2020

Due to the organization of our codebase we tend to build and link in a bunch of functions into the binaries that aren't actually used.

I've tried building with -ffunction-sections -fdata-sections -Wl,--gc-sections, and it reduces bitcoind (after stripping) from 7.2 MB to 6.9 MB. The bitcoin-wallet tool goes from 3.5 MB to 1.3 MB.

Any reason why we don't do this? I only find a mention in #4042, but it's not clear how relevant that is to me.
@sipa sipa added the Feature label Apr 9, 2020
@sipa
Copy link
Member Author

sipa commented Apr 9, 2020

Ping @theuni.

@sipa sipa removed the Feature label Apr 9, 2020
@fanquake
Copy link
Member

fanquake commented Apr 9, 2020

I've got a branch that does link time garbage collection, using pretty much the same flags, and have seen similar binary size savings. If wanted this is probably something we could add to the Guix / Gitian descriptors.

@ryanofsky
Copy link
Contributor

This seems ok, but IIRC if you build with -ffunction-sections and -fdata-sections it mask link errors that would happen without these flags, because the linker won't complain about missing symbols from the sections that are skipped. It might be better to reserve these flags for release builds, otherwise it could be annoying when local builds succeed but appveyor / travis builds have link errors

@sipa
Copy link
Member Author

sipa commented Apr 9, 2020

@ryanofsky That's a good point. Maybe just something to do in Gitian/Guix.

@fanquake fanquake mentioned this issue Apr 12, 2020
Closed
@laanwj
Copy link
Member

laanwj commented Apr 13, 2020

@ryanofsky That's a good point. Maybe just something to do in Gitian/Guix.

Agree.
I kind of like this, it's a safer (and link-time much faster) option than going full LTO but still manages to get rid of a lot of cruft in the binary.

@practicalswift
Copy link
Contributor

I kind of like this, it's a safer (and link-time much faster) option than going full LTO but still manages to get rid of a lot of cruft in the binary.

Just to fully understand the argument: safer than full LTO in what regard?

@sipa
Copy link
Member Author

sipa commented Apr 13, 2020 •
edited
Loading

LTO has historically be a source of bugs in compilers - it's improved a lot of course, and we should evaluate when it's mature enough to enable. But that's a far bigger change than enabling link-time GC.

In addition, experiments in the past have shown that LTO wasn't always a performance gain (it has been a while, the situation may be better now, but even then, maybe we only want it for certain compiler versions etc...).

@fanquake fanquake mentioned this issue Apr 14, 2020
Merged
fanquake added a commit that referenced this issue Apr 28, 2020
@fanquake
Merge #18629: scripts: add PE .reloc section check to security-check.py
ac21090 
3e38023 scripts: add PE .reloc section check to security-check.py (fanquake)

Pull request description:

  The `ld` in binutils has historically had a few issues with PE binaries, there's a good summary in this [thread](https://sourceware.org/bugzilla/show_bug.cgi?id=19011).

  One issue in particular was `ld` stripping the `.reloc` section out of PE binaries, even though it's required for functioning ASLR. This was [reported by a Tor developer in 2014](https://sourceware.org/bugzilla/show_bug.cgi?id=17321) and they have been patching their [own binutils](https://gitweb.torproject.org/builders/tor-browser-build.git/tree/projects/binutils) ever since. However their patch only made it into binutils at the [start of this year](https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=dc9bd8c92af67947db44b3cb428c050259b15cd0). It adds an `--enable-reloc-section` flag, which is turned on by default if you are using `--dynamic-base`. In the mean time this issue has also been worked around by other projects, such as FFmpeg, see [this commit](TheRyuu/FFmpeg@91b668a).

  I have checked our recent supported Windows release binaries, and they do contain a `.reloc` section. From what I understand, we are using all the right compile/linker flags, including `-pie` & `-fPIE`, and have never run into the crashing/entrypoint issues that other projects might have seen.

  One other thing worth noting here, it how Debian/Ubuntu patch the binutils that they distribute, because that's what we end up using in our gitian builds.

  In the binutils-mingw-w64 in Bionic (18.04), which we currently use in gitian, PE hardening options/security flags are enabled by default. See the [changelog](https://changelogs.ubuntu.com/changelogs/pool/universe/b/binutils-mingw-w64/binutils-mingw-w64_8ubuntu1/changelog) and the [relevant commit](https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/452b3013b8280cbe35eaeb166a43621b88d5f8b7).

  However in Focal (20.04), this has now been reversed. PE hardening options are no-longer the default. See the [changelog](https://changelogs.ubuntu.com/changelogs/pool/universe/b/binutils-mingw-w64/binutils-mingw-w64_8.8/changelog) and [relevant commit](https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/7bd8b2fbc242a8c2fc2217f29fd61f94d3babf6f), which cites same .reloc issue mentioned here.

  Given that we explicitly specify/opt-in to everything that we want to use, the defaults aren't necessarily an issue for us. However I think it highlights the importance of continuing to be explicit about what we want, and not falling-back or relying on upstream.

  This was also prompted by the possibility of us doing link time garbage collection, see #18579 & #18605. It seemed some sanity checks would be worthwhile in-case the linker goes haywire while garbage collecting.

  I think Guix is going to bring great benefits when dealing with these kinds of issues. Carl you might have something to say in that regard.

ACKs for top commit:
  dongcarl:
    ACK 3e38023

Tree-SHA512: af14d63bdb334bde548dd7de3e0946556b7e2598d817b56eb4e75b3f56c705c26aa85dd9783134c4b6a7aeb7cb4de567eed996e94d533d31511f57ed332287da
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Apr 28, 2020
@fanquake
Merge bitcoin#18629: scripts: add PE .reloc section check to security…
d861869 
…-check.py

3e38023 scripts: add PE .reloc section check to security-check.py (fanquake)

Pull request description:

  The `ld` in binutils has historically had a few issues with PE binaries, there's a good summary in this [thread](https://sourceware.org/bugzilla/show_bug.cgi?id=19011).

  One issue in particular was `ld` stripping the `.reloc` section out of PE binaries, even though it's required for functioning ASLR. This was [reported by a Tor developer in 2014](https://sourceware.org/bugzilla/show_bug.cgi?id=17321) and they have been patching their [own binutils](https://gitweb.torproject.org/builders/tor-browser-build.git/tree/projects/binutils) ever since. However their patch only made it into binutils at the [start of this year](https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=dc9bd8c92af67947db44b3cb428c050259b15cd0). It adds an `--enable-reloc-section` flag, which is turned on by default if you are using `--dynamic-base`. In the mean time this issue has also been worked around by other projects, such as FFmpeg, see [this commit](TheRyuu/FFmpeg@91b668a).

  I have checked our recent supported Windows release binaries, and they do contain a `.reloc` section. From what I understand, we are using all the right compile/linker flags, including `-pie` & `-fPIE`, and have never run into the crashing/entrypoint issues that other projects might have seen.

  One other thing worth noting here, it how Debian/Ubuntu patch the binutils that they distribute, because that's what we end up using in our gitian builds.

  In the binutils-mingw-w64 in Bionic (18.04), which we currently use in gitian, PE hardening options/security flags are enabled by default. See the [changelog](https://changelogs.ubuntu.com/changelogs/pool/universe/b/binutils-mingw-w64/binutils-mingw-w64_8ubuntu1/changelog) and the [relevant commit](https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/452b3013b8280cbe35eaeb166a43621b88d5f8b7).

  However in Focal (20.04), this has now been reversed. PE hardening options are no-longer the default. See the [changelog](https://changelogs.ubuntu.com/changelogs/pool/universe/b/binutils-mingw-w64/binutils-mingw-w64_8.8/changelog) and [relevant commit](https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/7bd8b2fbc242a8c2fc2217f29fd61f94d3babf6f), which cites same .reloc issue mentioned here.

  Given that we explicitly specify/opt-in to everything that we want to use, the defaults aren't necessarily an issue for us. However I think it highlights the importance of continuing to be explicit about what we want, and not falling-back or relying on upstream.

  This was also prompted by the possibility of us doing link time garbage collection, see bitcoin#18579 & bitcoin#18605. It seemed some sanity checks would be worthwhile in-case the linker goes haywire while garbage collecting.

  I think Guix is going to bring great benefits when dealing with these kinds of issues. Carl you might have something to say in that regard.

ACKs for top commit:
  dongcarl:
    ACK 3e38023

Tree-SHA512: af14d63bdb334bde548dd7de3e0946556b7e2598d817b56eb4e75b3f56c705c26aa85dd9783134c4b6a7aeb7cb4de567eed996e94d533d31511f57ed332287da
@fanquake fanquake mentioned this issue Oct 1, 2021
Merged
fanquake added a commit that referenced this issue Nov 25, 2021
@fanquake
Merge #23152: build: add --enable-lto configuration option
681b25e 
68e5aaf build: add `--enable-lto` configuration option (fanquake)

Pull request description:

  It's been 5 years since using LTO was first suggested for use when building Bitcoin Core, and it's time to revisit it again. Compilers, and their LTO implementations, have matured, and Bitcoin Core has come a long way in terms of pruning dependencies which may have proved troublesome (i.e Boost previously had issues when using LTO). We'll have even less Boost code after moving to `std::filesystem` (#20744).

  Experimenting with LTO came up on IRC last night:
  > sipa: jamesob: i'm interested in knowing whether "-flto" and/or "-fdata-sections -ffunction-sections -Wl,--gc-sections" are possible/beneficial with our current compiler suite; what would be a good way to have your test infrastructure benchmark things?

  So this PR just adds the bare minimum to make it easier to configure, compile and perform some bench-marking using `-flto`. This PR doesn't do anything depends wise, however if we decide this is what we want to do, I'll expand the changes here.

  I had previously had a PR open (#18605) to perform link time garbage collection (`-ffunction-sections -fdata-sections` & `-Wl,--gc-sections`), however moving straight to using LTO would be preferable.

  Note that our minimum required set of compilers, GCC 8.1 and Clang 7, all support the `-flto` option.

  Related #18579.
  Previous discussion: #10616, #14277.
  Previous related PRs: #10800 (`-flto`), #16791 (ThinLTO).

  Guix build:
  ```bash
  bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
  1f3a7c5be4169aaa444b481d3e65a7bb72da9007fee6e6c416ded2e70f97374b  guix-build-68e5aafde3e8/output/aarch64-linux-gnu/SHA256SUMS.part
  fa8f4cf223d9aaf0b2c1ef55ce61256a19cd1ad7f42b99d0b98c9a52fe6ad8ba  guix-build-68e5aafde3e8/output/aarch64-linux-gnu/bitcoin-68e5aafde3e8-aarch64-linux-gnu-debug.tar.gz
  9a9967078cd1849b4e85db619e1f55d305c6d44e9e013067c0e8d62c1ba54087  guix-build-68e5aafde3e8/output/aarch64-linux-gnu/bitcoin-68e5aafde3e8-aarch64-linux-gnu.tar.gz
  18c71f30722102baaf3dfda67f7c7aac38723510b142e8df8ee7063c5d499368  guix-build-68e5aafde3e8/output/arm-linux-gnueabihf/SHA256SUMS.part
  0854cc0d17c045a118df2a24e4cf36d727e7e7e2dea37c2492ee21b71cb79b4b  guix-build-68e5aafde3e8/output/arm-linux-gnueabihf/bitcoin-68e5aafde3e8-arm-linux-gnueabihf-debug.tar.gz
  215256897dde4e8412ed60473376c694a80c5479fb08039107fb62435f2816ef  guix-build-68e5aafde3e8/output/arm-linux-gnueabihf/bitcoin-68e5aafde3e8-arm-linux-gnueabihf.tar.gz
  5fad0d9d12bc514ec46ed5d66fd29b7da1376a4a69c3b692936f1ab2356e2f85  guix-build-68e5aafde3e8/output/dist-archive/bitcoin-68e5aafde3e8.tar.gz
  4f32989d4ab1946048ca7caee9a983fa875be262282562f5a3e040f4bf92158e  guix-build-68e5aafde3e8/output/powerpc64-linux-gnu/SHA256SUMS.part
  ae45df309ae8ada52891efac0a369a69fed4ab93847a7bc4150a62230df4c8d7  guix-build-68e5aafde3e8/output/powerpc64-linux-gnu/bitcoin-68e5aafde3e8-powerpc64-linux-gnu-debug.tar.gz
  0ced227de15cb578567131271e2effe80681b4d7a436c92bf1caec735a576fa4  guix-build-68e5aafde3e8/output/powerpc64-linux-gnu/bitcoin-68e5aafde3e8-powerpc64-linux-gnu.tar.gz
  26fc5d2ccc1bc17ee0a146cacada6f4909d90c136ae640c8337332adce414ee0  guix-build-68e5aafde3e8/output/powerpc64le-linux-gnu/SHA256SUMS.part
  9956b544d90a62a8ba9fc9dc6b6b7f0efe193357332ec19e88053a89d4aab37e  guix-build-68e5aafde3e8/output/powerpc64le-linux-gnu/bitcoin-68e5aafde3e8-powerpc64le-linux-gnu-debug.tar.gz
  be8e39ceea1d36086ce5fa93bfb138c68d3bdf0dd6950b192dfa27a65cce3836  guix-build-68e5aafde3e8/output/powerpc64le-linux-gnu/bitcoin-68e5aafde3e8-powerpc64le-linux-gnu.tar.gz
  a7755edc394972885c4c77a7798007e5ba4126b177c4ff6224275c4fb8f3b1c4  guix-build-68e5aafde3e8/output/riscv64-linux-gnu/SHA256SUMS.part
  b6d252993d8aae7582ad6385fe53c61c54c284c68ece6cb2b2d1ac9554e06139  guix-build-68e5aafde3e8/output/riscv64-linux-gnu/bitcoin-68e5aafde3e8-riscv64-linux-gnu-debug.tar.gz
  bb4860f3bbd815f800333124ff901d880741792ab47097f49bda3a6931144da0  guix-build-68e5aafde3e8/output/riscv64-linux-gnu/bitcoin-68e5aafde3e8-riscv64-linux-gnu.tar.gz
  3dd17deed5c5935fb28b62dfc7afca5caab0d67862cdcbf3337edae73e1d0c4c  guix-build-68e5aafde3e8/output/x86_64-apple-darwin19/SHA256SUMS.part
  fa2d68c54fda0816188c81ce2201a77340b82645da2ffe412526f92c297a82df  guix-build-68e5aafde3e8/output/x86_64-apple-darwin19/bitcoin-68e5aafde3e8-osx-unsigned.dmg
  f6e5accdcd201f522b6426e4d8cc9b3643d4d43a57d268fa0e79ea9a34cfac01  guix-build-68e5aafde3e8/output/x86_64-apple-darwin19/bitcoin-68e5aafde3e8-osx-unsigned.tar.gz
  4e5a127df957d1c73b65925d685f6620e7bc5667efcb6dcd98be76effc22fc12  guix-build-68e5aafde3e8/output/x86_64-apple-darwin19/bitcoin-68e5aafde3e8-osx64.tar.gz
  56ccd216a69acafacbdc6bae0bdcc1faa50b6a51be1aebfa7068206c88b3241a  guix-build-68e5aafde3e8/output/x86_64-linux-gnu/SHA256SUMS.part
  77b93dd5fad322636853e5b0244ffafd97cc97f3b4b4ee755d5f830b75d77d13  guix-build-68e5aafde3e8/output/x86_64-linux-gnu/bitcoin-68e5aafde3e8-x86_64-linux-gnu-debug.tar.gz
  1feda932fc127b900316a232432b91e46e57ee12a81e12a7d888fdc3296219c1  guix-build-68e5aafde3e8/output/x86_64-linux-gnu/bitcoin-68e5aafde3e8-x86_64-linux-gnu.tar.gz
  aa7c53ab4164b3736049065c3c24391fc5bd7f26b4bda4aa877c378f0636a125  guix-build-68e5aafde3e8/output/x86_64-w64-mingw32/SHA256SUMS.part
  5e76148e67aef7e91e70074bfadc08e94373449ac3b966f4343b04d230c778fd  guix-build-68e5aafde3e8/output/x86_64-w64-mingw32/bitcoin-68e5aafde3e8-win-unsigned.tar.gz
  34123e3d818beeb70113caeda66945bc7cb9d9e987515d5b149bd17b4b38da90  guix-build-68e5aafde3e8/output/x86_64-w64-mingw32/bitcoin-68e5aafde3e8-win64-debug.zip
  2bba7f40a2b23c6ea3d47c4f564ab54201bf27f7f57103a98cc9bceea4e70c4d  guix-build-68e5aafde3e8/output/x86_64-w64-mingw32/bitcoin-68e5aafde3e8-win64-setup-unsigned.exe
  0e7e124144af4a92a4344cf70a3b7c06fbd2b8782aee7ede7263893afa3a5ef0  guix-build-68e5aafde3e8/output/x86_64-w64-mingw32/bitcoin-68e5aafde3e8-win64.zip
  ```

ACKs for top commit:
  laanwj:
    Code review ACK 68e5aaf

Tree-SHA512: 5c25249cc178b9d54159e268390c974b739df9458d773e23c14b14d808f87f7afe314058b3c068601a9132042321973b0c9b6f81becb925665eca2738ae9a613
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Nov 25, 2021
@fanquake
Merge bitcoin#23152: build: add --enable-lto configuration option
67658ee 
68e5aaf build: add `--enable-lto` configuration option (fanquake)

Pull request description:

  It's been 5 years since using LTO was first suggested for use when building Bitcoin Core, and it's time to revisit it again. Compilers, and their LTO implementations, have matured, and Bitcoin Core has come a long way in terms of pruning dependencies which may have proved troublesome (i.e Boost previously had issues when using LTO). We'll have even less Boost code after moving to `std::filesystem` (bitcoin#20744).

  Experimenting with LTO came up on IRC last night:
  > sipa: jamesob: i'm interested in knowing whether "-flto" and/or "-fdata-sections -ffunction-sections -Wl,--gc-sections" are possible/beneficial with our current compiler suite; what would be a good way to have your test infrastructure benchmark things?

  So this PR just adds the bare minimum to make it easier to configure, compile and perform some bench-marking using `-flto`. This PR doesn't do anything depends wise, however if we decide this is what we want to do, I'll expand the changes here.

  I had previously had a PR open (bitcoin#18605) to perform link time garbage collection (`-ffunction-sections -fdata-sections` & `-Wl,--gc-sections`), however moving straight to using LTO would be preferable.

  Note that our minimum required set of compilers, GCC 8.1 and Clang 7, all support the `-flto` option.

  Related bitcoin#18579.
  Previous discussion: bitcoin#10616, bitcoin#14277.
  Previous related PRs: bitcoin#10800 (`-flto`), bitcoin#16791 (ThinLTO).

  Guix build:
  ```bash
  bash-5.1# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
  1f3a7c5be4169aaa444b481d3e65a7bb72da9007fee6e6c416ded2e70f97374b  guix-build-68e5aafde3e8/output/aarch64-linux-gnu/SHA256SUMS.part
  fa8f4cf223d9aaf0b2c1ef55ce61256a19cd1ad7f42b99d0b98c9a52fe6ad8ba  guix-build-68e5aafde3e8/output/aarch64-linux-gnu/bitcoin-68e5aafde3e8-aarch64-linux-gnu-debug.tar.gz
  9a9967078cd1849b4e85db619e1f55d305c6d44e9e013067c0e8d62c1ba54087  guix-build-68e5aafde3e8/output/aarch64-linux-gnu/bitcoin-68e5aafde3e8-aarch64-linux-gnu.tar.gz
  18c71f30722102baaf3dfda67f7c7aac38723510b142e8df8ee7063c5d499368  guix-build-68e5aafde3e8/output/arm-linux-gnueabihf/SHA256SUMS.part
  0854cc0d17c045a118df2a24e4cf36d727e7e7e2dea37c2492ee21b71cb79b4b  guix-build-68e5aafde3e8/output/arm-linux-gnueabihf/bitcoin-68e5aafde3e8-arm-linux-gnueabihf-debug.tar.gz
  215256897dde4e8412ed60473376c694a80c5479fb08039107fb62435f2816ef  guix-build-68e5aafde3e8/output/arm-linux-gnueabihf/bitcoin-68e5aafde3e8-arm-linux-gnueabihf.tar.gz
  5fad0d9d12bc514ec46ed5d66fd29b7da1376a4a69c3b692936f1ab2356e2f85  guix-build-68e5aafde3e8/output/dist-archive/bitcoin-68e5aafde3e8.tar.gz
  4f32989d4ab1946048ca7caee9a983fa875be262282562f5a3e040f4bf92158e  guix-build-68e5aafde3e8/output/powerpc64-linux-gnu/SHA256SUMS.part
  ae45df309ae8ada52891efac0a369a69fed4ab93847a7bc4150a62230df4c8d7  guix-build-68e5aafde3e8/output/powerpc64-linux-gnu/bitcoin-68e5aafde3e8-powerpc64-linux-gnu-debug.tar.gz
  0ced227de15cb578567131271e2effe80681b4d7a436c92bf1caec735a576fa4  guix-build-68e5aafde3e8/output/powerpc64-linux-gnu/bitcoin-68e5aafde3e8-powerpc64-linux-gnu.tar.gz
  26fc5d2ccc1bc17ee0a146cacada6f4909d90c136ae640c8337332adce414ee0  guix-build-68e5aafde3e8/output/powerpc64le-linux-gnu/SHA256SUMS.part
  9956b544d90a62a8ba9fc9dc6b6b7f0efe193357332ec19e88053a89d4aab37e  guix-build-68e5aafde3e8/output/powerpc64le-linux-gnu/bitcoin-68e5aafde3e8-powerpc64le-linux-gnu-debug.tar.gz
  be8e39ceea1d36086ce5fa93bfb138c68d3bdf0dd6950b192dfa27a65cce3836  guix-build-68e5aafde3e8/output/powerpc64le-linux-gnu/bitcoin-68e5aafde3e8-powerpc64le-linux-gnu.tar.gz
  a7755edc394972885c4c77a7798007e5ba4126b177c4ff6224275c4fb8f3b1c4  guix-build-68e5aafde3e8/output/riscv64-linux-gnu/SHA256SUMS.part
  b6d252993d8aae7582ad6385fe53c61c54c284c68ece6cb2b2d1ac9554e06139  guix-build-68e5aafde3e8/output/riscv64-linux-gnu/bitcoin-68e5aafde3e8-riscv64-linux-gnu-debug.tar.gz
  bb4860f3bbd815f800333124ff901d880741792ab47097f49bda3a6931144da0  guix-build-68e5aafde3e8/output/riscv64-linux-gnu/bitcoin-68e5aafde3e8-riscv64-linux-gnu.tar.gz
  3dd17deed5c5935fb28b62dfc7afca5caab0d67862cdcbf3337edae73e1d0c4c  guix-build-68e5aafde3e8/output/x86_64-apple-darwin19/SHA256SUMS.part
  fa2d68c54fda0816188c81ce2201a77340b82645da2ffe412526f92c297a82df  guix-build-68e5aafde3e8/output/x86_64-apple-darwin19/bitcoin-68e5aafde3e8-osx-unsigned.dmg
  f6e5accdcd201f522b6426e4d8cc9b3643d4d43a57d268fa0e79ea9a34cfac01  guix-build-68e5aafde3e8/output/x86_64-apple-darwin19/bitcoin-68e5aafde3e8-osx-unsigned.tar.gz
  4e5a127df957d1c73b65925d685f6620e7bc5667efcb6dcd98be76effc22fc12  guix-build-68e5aafde3e8/output/x86_64-apple-darwin19/bitcoin-68e5aafde3e8-osx64.tar.gz
  56ccd216a69acafacbdc6bae0bdcc1faa50b6a51be1aebfa7068206c88b3241a  guix-build-68e5aafde3e8/output/x86_64-linux-gnu/SHA256SUMS.part
  77b93dd5fad322636853e5b0244ffafd97cc97f3b4b4ee755d5f830b75d77d13  guix-build-68e5aafde3e8/output/x86_64-linux-gnu/bitcoin-68e5aafde3e8-x86_64-linux-gnu-debug.tar.gz
  1feda932fc127b900316a232432b91e46e57ee12a81e12a7d888fdc3296219c1  guix-build-68e5aafde3e8/output/x86_64-linux-gnu/bitcoin-68e5aafde3e8-x86_64-linux-gnu.tar.gz
  aa7c53ab4164b3736049065c3c24391fc5bd7f26b4bda4aa877c378f0636a125  guix-build-68e5aafde3e8/output/x86_64-w64-mingw32/SHA256SUMS.part
  5e76148e67aef7e91e70074bfadc08e94373449ac3b966f4343b04d230c778fd  guix-build-68e5aafde3e8/output/x86_64-w64-mingw32/bitcoin-68e5aafde3e8-win-unsigned.tar.gz
  34123e3d818beeb70113caeda66945bc7cb9d9e987515d5b149bd17b4b38da90  guix-build-68e5aafde3e8/output/x86_64-w64-mingw32/bitcoin-68e5aafde3e8-win64-debug.zip
  2bba7f40a2b23c6ea3d47c4f564ab54201bf27f7f57103a98cc9bceea4e70c4d  guix-build-68e5aafde3e8/output/x86_64-w64-mingw32/bitcoin-68e5aafde3e8-win64-setup-unsigned.exe
  0e7e124144af4a92a4344cf70a3b7c06fbd2b8782aee7ede7263893afa3a5ef0  guix-build-68e5aafde3e8/output/x86_64-w64-mingw32/bitcoin-68e5aafde3e8-win64.zip
  ```

ACKs for top commit:
  laanwj:
    Code review ACK 68e5aaf

Tree-SHA512: 5c25249cc178b9d54159e268390c974b739df9458d773e23c14b14d808f87f7afe314058b3c068601a9132042321973b0c9b6f81becb925665eca2738ae9a613
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Jun 19, 2022
@fanquake @PastaPastaPasta
Merge bitcoin#18629: scripts: add PE .reloc section check to security…
0ec2a3b 
…-check.py

3e38023 scripts: add PE .reloc section check to security-check.py (fanquake)

Pull request description:

  The `ld` in binutils has historically had a few issues with PE binaries, there's a good summary in this [thread](https://sourceware.org/bugzilla/show_bug.cgi?id=19011).

  One issue in particular was `ld` stripping the `.reloc` section out of PE binaries, even though it's required for functioning ASLR. This was [reported by a Tor developer in 2014](https://sourceware.org/bugzilla/show_bug.cgi?id=17321) and they have been patching their [own binutils](https://gitweb.torproject.org/builders/tor-browser-build.git/tree/projects/binutils) ever since. However their patch only made it into binutils at the [start of this year](https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=dc9bd8c92af67947db44b3cb428c050259b15cd0). It adds an `--enable-reloc-section` flag, which is turned on by default if you are using `--dynamic-base`. In the mean time this issue has also been worked around by other projects, such as FFmpeg, see [this commit](TheRyuu/FFmpeg@91b668a).

  I have checked our recent supported Windows release binaries, and they do contain a `.reloc` section. From what I understand, we are using all the right compile/linker flags, including `-pie` & `-fPIE`, and have never run into the crashing/entrypoint issues that other projects might have seen.

  One other thing worth noting here, it how Debian/Ubuntu patch the binutils that they distribute, because that's what we end up using in our gitian builds.

  In the binutils-mingw-w64 in Bionic (18.04), which we currently use in gitian, PE hardening options/security flags are enabled by default. See the [changelog](https://changelogs.ubuntu.com/changelogs/pool/universe/b/binutils-mingw-w64/binutils-mingw-w64_8ubuntu1/changelog) and the [relevant commit](https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/452b3013b8280cbe35eaeb166a43621b88d5f8b7).

  However in Focal (20.04), this has now been reversed. PE hardening options are no-longer the default. See the [changelog](https://changelogs.ubuntu.com/changelogs/pool/universe/b/binutils-mingw-w64/binutils-mingw-w64_8.8/changelog) and [relevant commit](https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/7bd8b2fbc242a8c2fc2217f29fd61f94d3babf6f), which cites same .reloc issue mentioned here.

  Given that we explicitly specify/opt-in to everything that we want to use, the defaults aren't necessarily an issue for us. However I think it highlights the importance of continuing to be explicit about what we want, and not falling-back or relying on upstream.

  This was also prompted by the possibility of us doing link time garbage collection, see bitcoin#18579 & bitcoin#18605. It seemed some sanity checks would be worthwhile in-case the linker goes haywire while garbage collecting.

  I think Guix is going to bring great benefits when dealing with these kinds of issues. Carl you might have something to say in that regard.

ACKs for top commit:
  dongcarl:
    ACK 3e38023

Tree-SHA512: af14d63bdb334bde548dd7de3e0946556b7e2598d817b56eb4e75b3f56c705c26aa85dd9783134c4b6a7aeb7cb4de567eed996e94d533d31511f57ed332287da
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Jun 19, 2022
@fanquake @PastaPastaPasta
Merge bitcoin#18629: scripts: add PE .reloc section check to security…
f7a94b5 
…-check.py

3e38023 scripts: add PE .reloc section check to security-check.py (fanquake)

Pull request description:

  The `ld` in binutils has historically had a few issues with PE binaries, there's a good summary in this [thread](https://sourceware.org/bugzilla/show_bug.cgi?id=19011).

  One issue in particular was `ld` stripping the `.reloc` section out of PE binaries, even though it's required for functioning ASLR. This was [reported by a Tor developer in 2014](https://sourceware.org/bugzilla/show_bug.cgi?id=17321) and they have been patching their [own binutils](https://gitweb.torproject.org/builders/tor-browser-build.git/tree/projects/binutils) ever since. However their patch only made it into binutils at the [start of this year](https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=dc9bd8c92af67947db44b3cb428c050259b15cd0). It adds an `--enable-reloc-section` flag, which is turned on by default if you are using `--dynamic-base`. In the mean time this issue has also been worked around by other projects, such as FFmpeg, see [this commit](TheRyuu/FFmpeg@91b668a).

  I have checked our recent supported Windows release binaries, and they do contain a `.reloc` section. From what I understand, we are using all the right compile/linker flags, including `-pie` & `-fPIE`, and have never run into the crashing/entrypoint issues that other projects might have seen.

  One other thing worth noting here, it how Debian/Ubuntu patch the binutils that they distribute, because that's what we end up using in our gitian builds.

  In the binutils-mingw-w64 in Bionic (18.04), which we currently use in gitian, PE hardening options/security flags are enabled by default. See the [changelog](https://changelogs.ubuntu.com/changelogs/pool/universe/b/binutils-mingw-w64/binutils-mingw-w64_8ubuntu1/changelog) and the [relevant commit](https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/452b3013b8280cbe35eaeb166a43621b88d5f8b7).

  However in Focal (20.04), this has now been reversed. PE hardening options are no-longer the default. See the [changelog](https://changelogs.ubuntu.com/changelogs/pool/universe/b/binutils-mingw-w64/binutils-mingw-w64_8.8/changelog) and [relevant commit](https://salsa.debian.org/mingw-w64-team/binutils-mingw-w64/-/commit/7bd8b2fbc242a8c2fc2217f29fd61f94d3babf6f), which cites same .reloc issue mentioned here.

  Given that we explicitly specify/opt-in to everything that we want to use, the defaults aren't necessarily an issue for us. However I think it highlights the importance of continuing to be explicit about what we want, and not falling-back or relying on upstream.

  This was also prompted by the possibility of us doing link time garbage collection, see bitcoin#18579 & bitcoin#18605. It seemed some sanity checks would be worthwhile in-case the linker goes haywire while garbage collecting.

  I think Guix is going to bring great benefits when dealing with these kinds of issues. Carl you might have something to say in that regard.

ACKs for top commit:
  dongcarl:
    ACK 3e38023

Tree-SHA512: af14d63bdb334bde548dd7de3e0946556b7e2598d817b56eb4e75b3f56c705c26aa85dd9783134c4b6a7aeb7cb4de567eed996e94d533d31511f57ed332287da
@fanquake
Copy link
Member

fanquake commented Aug 3, 2022

Going to close this for now, given that hopefully we'll be using in an upcoming release (not 24.0). #18605 was an attempt to add the garbage collection link flags.

@fanquake fanquake closed this as completed Aug 3, 2022
@lyricidal lyricidal mentioned this issue Mar 22, 2023
Merged
@bitcoin bitcoin locked and limited conversation to collaborators Aug 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

build: Link time garbage collection fanquake/bitcoin
6 participants
@laanwj @sipa @fanquake @ryanofsky @practicalswift and others