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: Windows SSP roundup #28461

Merged
merged 3 commits into from
Nov 22, 2023
Merged

build: Windows SSP roundup #28461

merged 3 commits into from
Nov 22, 2023

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Sep 12, 2023 •
edited

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 12, 2023 •
edited

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
Copy link
Member

hebasto commented Sep 12, 2023

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
Copy link
Member Author

Where is this quote from?

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

@fanquake fanquake mentioned this pull request Oct 30, 2023
Closed
@theuni
Copy link
Member

theuni commented Oct 31, 2023

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 fanquake marked this pull request as ready for review November 1, 2023 10:44
@fanquake
Copy link
Member Author

fanquake commented Nov 1, 2023

cc @TheCharlatan

@TheCharlatan
Copy link
Contributor

Guix builds (x86_64)

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

hebasto
hebasto reviewed Nov 4, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK on stack smashing protection stuff.

contrib/guix/manifest.scm Outdated
;; and thus will ensure that this works properly.
`(cons "gcc_cv_libc_provides_ssp=yes" ,flags))))))
"--enable-default-ssp=yes",
"--enable-default-pie=yes",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@fanquake fanquake Nov 13, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

hebasto commented Nov 15, 2023

My Guix builds:

x86_64
c446041204b8b099c0b3e91f34bb783d11f57077ec685a141a766a9c44b917e0  guix-build-f95af98128f1/output/dist-archive/bitcoin-f95af98128f1.tar.gz
f929fd24af143247ca32a747ec29a7982b808c87d2221a47d257760efa1a216c  guix-build-f95af98128f1/output/x86_64-w64-mingw32/SHA256SUMS.part
4287bb5bdc1ef7253fec176cd18ad1459dcb49705cc6b925aea41694fd5e390a  guix-build-f95af98128f1/output/x86_64-w64-mingw32/bitcoin-f95af98128f1-win64-debug.zip
6109618130b5e240739da023a94d1b93ed05e619cee0ad685bece718b644fef8  guix-build-f95af98128f1/output/x86_64-w64-mingw32/bitcoin-f95af98128f1-win64-setup-unsigned.exe
4033b505ef0a6804ad1f19e597547855390804d88cccd82f1b7a76748d08d61c  guix-build-f95af98128f1/output/x86_64-w64-mingw32/bitcoin-f95af98128f1-win64-unsigned.tar.gz
82d290e2de7eb9e2b55e3040fdd0909c39fa2b619003ac7991ff2590c3bebb5a  guix-build-f95af98128f1/output/x86_64-w64-mingw32/bitcoin-f95af98128f1-win64.zip

hebasto
hebasto approved these changes Nov 15, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f95af98, 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 DrahtBot requested a review from theuni November 15, 2023 11:11
@fanquake
Copy link
Member Author

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
TheCharlatan approved these changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f95af98

I get the same hashes as @hebasto

@fanquake fanquake merged commit 5d13b95 into bitcoin:master Nov 22, 2023
16 checks passed
@fanquake fanquake deleted the windows_ssp_roundup branch November 22, 2023 17:17
@hebasto hebasto mentioned this pull request Jan 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheCharlatan TheCharlatan TheCharlatan approved these changes

@hebasto hebasto hebasto approved these changes

@theuni theuni Awaiting requested review from theuni

Assignees
No one assigned
Labels
Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

win: non-static libssp in libbitcoinconsensus DLL
5 participants
@fanquake @DrahtBot @hebasto @theuni @TheCharlatan