build: Windows SSP roundup

fanquake commented at 2:46 pm on September 12, 2023: member

I was expecting this to fail to compile somewhere, maybe in the CI, but that doesn’t seem to be the case? Seems workable given the SSP related changes in the newer mingw-w64 headers (which are in Guix):

Implement some of the stack protector functions/variables so -lssp is now optional when _FORTIFY_SOURCE or -fstack-protector-strong is used.

However I think this would still be broken in some older environments, so we might have to wait for a compiler bump, or similar. The optional -lssp also seems to work when using older headers, which doesn’t make sense.

Would fix #28104.

fanquake added the label Windows on Sep 12, 2023

DrahtBot commented at 2:46 pm on September 12, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, TheCharlatan
Concept ACK	theuni

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

hebasto commented at 3:12 pm on September 12, 2023: member

Nice!

I was expecting this to fail to compile somewhere, maybe in the CI, but that doesn’t seem to be the case? Seems workable given the SSP related changes in the newer mingw-w64 headers (which are in Guix):

Implement some of the stack protector functions/variables so -lssp is now optional when _FORTIFY_SOURCE or -fstack-protector-strong is used.

Where is this quote from?

fanquake commented at 8:52 am on September 13, 2023: member

Where is this quote from?

The Mingw-w64 release notes: https://sourceforge.net/p/mingw-w64/mailman/message/37837156/.

fanquake force-pushed on Oct 2, 2023

fanquake force-pushed on Oct 30, 2023

theuni commented at 8:28 pm on October 31, 2023: member

Concept ACK This comment (and the rest of that thread) helped me to understand what was going on here. As I understand it, “-lssp” should’ve never been needed. It was used as a hack for years, but is no longer required after mingw-w64 v11.

I’m confused like @fanquake though. I would have expected us to need to carry the hack for a while, considering that the fix is from this year.

fanquake marked this as ready for review on Nov 1, 2023

fanquake commented at 10:48 am on November 1, 2023: member

cc @TheCharlatan

TheCharlatan commented at 12:42 pm on November 3, 2023: contributor

Guix builds (x86_64)

04be7b005bd9d131dcdd9d0221250431061206f6ca5d882bc3825221d0c223741  guix-build-f7f8522ee3a0/output/dist-archive/bitcoin-f7f8522ee3a0.tar.gz
1114bf9052b7a3e62d7c8a53526783eb3f09ba397e59ac9a40cd854ccecffc4e0  guix-build-f7f8522ee3a0/output/x86_64-w64-mingw32/SHA256SUMS.part
2a7e9d709211a444fa09c30f1ec7a1bb68fdd7b1165696d3c537a8419ec4fd294  guix-build-f7f8522ee3a0/output/x86_64-w64-mingw32/bitcoin-f7f8522ee3a0-win64-debug.zip
38f1ea99f220643f7382882a142709533dd3b5189b6963af8836eaa8f01f45cd0  guix-build-f7f8522ee3a0/output/x86_64-w64-mingw32/bitcoin-f7f8522ee3a0-win64-setup-unsigned.exe
4af7550c0419ee2becc1d2aea763a8cbb5a10a0f6a22cf9bf0216240d748c883d  guix-build-f7f8522ee3a0/output/x86_64-w64-mingw32/bitcoin-f7f8522ee3a0-win64-unsigned.tar.gz
5d1992d288588eb5a5b15dce2665168cd4407797e0cf6abb004bd29ec7ce82b70  guix-build-f7f8522ee3a0/output/x86_64-w64-mingw32/bitcoin-f7f8522ee3a0-win64.zip

in contrib/guix/manifest.scm:489 in f7f8522ee3 outdated

490-          ;; Uses the SSP functions from glibc instead of from libssp.so.
491-          ;; Our 'symbol-check' script will complain if we link against libssp.so,
492-          ;; and thus will ensure that this works properly.
493-          `(cons "gcc_cv_libc_provides_ssp=yes" ,flags))))))
494+                  "--enable-default-ssp=yes",
495+                  "--enable-default-pie=yes",

hebasto commented at 11:19 am on November 4, 2023:

How does this affect binaries? Aren’t position independent executables an ELF thing, not PE?

fanquake commented at 5:00 pm on November 13, 2023:

Pretty sure I did this so we could consolidate the gcc hardening options, doesn’t need to be a blocker for this, so dropped for now.

hebasto commented at 11:19 am on November 4, 2023: member

Approach ACK on stack smashing protection stuff.

build: remove explicit libssp linking from Windows build 8f43302a0a

guix: remove ssp workaround from Windows GCC 95d55b96c2

guix: default ssp for Windows GCC f95af98128

fanquake force-pushed on Nov 13, 2023

hebasto commented at 10:23 am on November 15, 2023: member

My Guix builds:

0x86_64
1c446041204b8b099c0b3e91f34bb783d11f57077ec685a141a766a9c44b917e0  guix-build-f95af98128f1/output/dist-archive/bitcoin-f95af98128f1.tar.gz
2f929fd24af143247ca32a747ec29a7982b808c87d2221a47d257760efa1a216c  guix-build-f95af98128f1/output/x86_64-w64-mingw32/SHA256SUMS.part
34287bb5bdc1ef7253fec176cd18ad1459dcb49705cc6b925aea41694fd5e390a  guix-build-f95af98128f1/output/x86_64-w64-mingw32/bitcoin-f95af98128f1-win64-debug.zip
46109618130b5e240739da023a94d1b93ed05e619cee0ad685bece718b644fef8  guix-build-f95af98128f1/output/x86_64-w64-mingw32/bitcoin-f95af98128f1-win64-setup-unsigned.exe
54033b505ef0a6804ad1f19e597547855390804d88cccd82f1b7a76748d08d61c  guix-build-f95af98128f1/output/x86_64-w64-mingw32/bitcoin-f95af98128f1-win64-unsigned.tar.gz
682d290e2de7eb9e2b55e3040fdd0909c39fa2b619003ac7991ff2590c3bebb5a  guix-build-f95af98128f1/output/x86_64-w64-mingw32/bitcoin-f95af98128f1-win64.zip

hebasto approved

hebasto commented at 11:11 am on November 15, 2023: member

ACK f95af98128f17002bf137a48441167020f3ef9bb, I’ve verified binaries from bitcoin-f95af98128f1-win64.zip on Windows 11 Pro 23H2.

However, the penultimate commit works just fine as well. The diffoscope shows minor changes in binaries.

So do we actually need the last commit at all?

DrahtBot requested review from theuni on Nov 15, 2023

fanquake commented at 11:14 am on November 15, 2023: member

So do we actually need the last commit at all?

The point is to explicitly enable the hardening option. This is the same as we do for the Linux GCC.

TheCharlatan approved

TheCharlatan commented at 3:19 pm on November 20, 2023: contributor

ACK f95af98128f17002bf137a48441167020f3ef9bb

I get the same hashes as @hebasto

fanquake merged this on Nov 22, 2023

fanquake closed this on Nov 22, 2023

fanquake deleted the branch on Nov 22, 2023

bitcoin locked this on Nov 21, 2024

build: Windows SSP roundup #28461

Code Coverage

Reviews