build: Windows SSP roundup #28461

1. 
   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.

2. fanquake added the label Windows on Sep 12, 2023
3. 
   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.

4. 
   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?

5. 
   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/.

6. fanquake force-pushed on Oct 2, 2023
7. fanquake force-pushed on Oct 30, 2023
8. 
   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.

9. fanquake marked this as ready for review on Nov 1, 2023
10. 
    fanquake commented at 10:48 am on November 1, 2023: member

    cc @TheCharlatan
11. 
    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
    

12. 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.
13. 
    hebasto commented at 11:19 am on November 4, 2023: member

    Approach ACK on stack smashing protection stuff.
14. build: remove explicit libssp linking from Windows build 8f43302a0a
15. guix: remove ssp workaround from Windows GCC 95d55b96c2
16. guix: default ssp for Windows GCC f95af98128
17. fanquake force-pushed on Nov 13, 2023
18. 
    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
    

19. hebasto approved
20. 
    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?

21. DrahtBot requested review from theuni on Nov 15, 2023
22. 
    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.

23. TheCharlatan approved
24. 
    TheCharlatan commented at 3:19 pm on November 20, 2023: contributor

    ACK f95af98128f17002bf137a48441167020f3ef9bb

    I get the same hashes as @hebasto

25. fanquake merged this on Nov 22, 2023
26. fanquake closed this on Nov 22, 2023

    ---

27. fanquake deleted the branch on Nov 22, 2023

Contributors
fanquake DrahtBot hebasto theuni TheCharlatan

Review Requested
theuni

Labels
Windows