GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — libpsl-native: Fix _FORTIFY_SOURCE macros by disconnect3d · Pull Request #88 · PowerShell/PowerShell-Native

Conversation

@disconnect3d

@disconnect3d Disconnect3d (disconnect3d) commented Dec 27, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This commit fixes the mistake in the _FORTIFY_SOURCE macro where it was not prefixed with underscore while it has to be (see e.g. https://github.com/search?q=repo%3Abminor%2Fglibc%20FORTIFY_SOURCE&type=code).

Additionally, to make this macro add extra security, one has to enable optimizations. I am not sure if the build system enables them, but it is worth double checking that as well.

Overall, I would recommend using -D_FORTIFY_SOURCE=3 with -O2 or -O3. (The fortify source level 3 was added recently and you can read more about it here: https://developers.redhat.com/blog/2021/04/16/broadening-compiler-checks-for-buffer-overflows-in-_fortify_source).

You can also see the result of the correct vs incorrect macro along with optimizations and no optimizations on this screenshot (source): image

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@disconnect3d
libpsl-native: Fix _FORTIFY_SOURCE macros
654b0c1 
This commit fixes the mistake in the `_FORTIFY_SOURCE` macro where it was not prefixed with underscore while it has to be (see e.g. https://github.com/search?q=repo%3Abminor%2Fglibc%20FORTIFY_SOURCE&type=code).

Additionally, to make this macro add extra security, one has to enable optimizations. I am not sure if the build system enables them, but it is worth double checking that as well.

Overall, I would recommend using `-D_FORTIFY_SOURCE=3` with  `-O2` or `-O3`. (The fortify source level 3 was added recently and you can read more about it here: https://developers.redhat.com/blog/2021/04/16/broadening-compiler-checks-for-buffer-overflows-in-_fortify_source).

You can also see the result of the correct vs incorrect macro along with optimizations and no optimizations on this screenshot ([source](https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGIM6SuADJ4DJgAcj4ARpjEIABspAAOqAqETgwe3r7%2ByanpAiFhkSwxcYl2mA4ZQgRMxARZPn4BldUCtfUERRHRsQm2dQ1NOa1D3aG9pf3xAJS2qF7EyOwc5gDMocjeWADUJutuCgTEocAAdAgH2CYaAIK3d6EEuyxMoRDPu8ik3wj1ACpdvVgAA3EwAVisEIAIrN9gB2Kz3XZ/eq7KJeKiQixcDSQmEHZF3VHHZZJACeEExVF%2BIPBUPxsNmRMeJgRMI481onAhvD8HC0pFQnDc1msuwUi2WmH2ZnWPFIBE0XPmAGsJPFzhD1gAOLgKgCc8TMZmNkkN%2Bk4kn5KuFnF4ChAGiVKvmcFgMCgXogSDQLCSdFi5Eo/sD9DiwC4ptIWFBeBWADU8JgAO4AeSSjE4ipotAIsSd1LtUVC9QpOd4/rYgnTDFoFcFvCwbyM4ibsbwxCqjlBmCdHcwqiqXgLduemB5HdoeCixHLHiwdpOeBYlfmVAMwAUybTmez3F4/EEIjE7CkMkEihU6g7umkBiMKHFln0s6dkHmqCSjgEA4AtDCAD6ABi6YAEoACoAJIgQAmkBQjpnI4FuDc6wwlwuz/umiptL%2BfgQK4Ix%2BFwgQMOgPQlGUegpGkBEkbR%2BQEVRfRxGR%2BE1OMjEcZOPZcV0rHTOxgxdDxokNEJNFcPMUpLCsEjcrytodiKHC7KourxP%2B8SSLswDIMguzRucZi7BAuCECQcoKrMvDKk2szzAgmBMFgcQQOqIAQi6U42qQa7rFqhoaPEXC6VwUi6ZI8TGqQApCmpjrOq6jmkB6iAoKgAZBmQFAQGGuUgFGMZxgmmC7hmWYCrmdAFsQRZRCWZbEI2irVowBB1g2dotoYwDtkK%2BDdtUfYDkKQ4jmOHYTlOQoznOC4YKsQormuh4bluO4plVB65rIp7iBex7yEoah2roEL6P1z6WNYb5RB%2BnnCj%2BGQAcBYFQbBCFIShaEHJh2HpkKnHOERFHicEkzUf0ZF0QUmSeM0TH0RkUmw7YfHtAwnTDEjOS8fYBG4xMxRsXoxxifjpESaTUzSbJ0oKTJVocHy8V2mpGlaTpekGUZJlmRZ%2BBEMQNkyfZbrOa57mUEpHD%2BWuZrnAi8QaD5ZgQhCZgaGYXCGoa6wc6pDq2ClDlaO6Pp%2Btl4bBvlhURsV0YBGVSY7fuNVHnVhaUE1HalswrWVqQHW1vWjZDZgrYDStzZdvxY12pNyCjnH5CCJOdoLfOrWLuna3rnwW2VZ7IcnYd57SCd17nXeAyPsYL42AtT1fq9f6cIBn0wfBiHIah6GAzheFYwRLgQ9TehQ2Twkowj4nwyx0Pk4T/EdNxU9r9jJPoyJlN49kNMH3TMPsYz8nnvL7MJbwXOadpun6YZxlmKZ5mWaL4t2allvy4rIB1jrHOEA0BYDwHG0SqbJ0LoLaqlIBqHyrN1gqSgRwSWaUMpYJtjlCMIYCq2yKiVV2mB4zuz3NVcuPsGp%2B2akHNqVZso1i6hHXq0d%2BqDXjiNXs/Zk7DlTtNIUs1s6zlzhSfOy5TjrR4JtJg25S6UMPJeYQogjrV1kLXW8QpdBkUbrdKwr5W7wHbgRd6oEIK9x%2BgPf6GEzI4TMI6MeGQJ7uC3uRSiK855w2YhkRePjCieOkpjImAlD7I23sTcYe8KabyPjEwSgTYYXxlIpVmN9OacG5o/PmL9BYfxFtZDYEtf7wMQb5a0vBApagRIaMwkhta6nWBoLguotK6kgXfaB5s3TpWtllXB9tQyEKdsQ2MpDyoKL2t7fMvtiwBxagw0OTDOrdUjs2dhbZ07DUTrwwc/C07jkznNXgOclpLg7IXDaxc5HbQoVM5RlcJDqKvGdLROhch6NMHdQx75jEvVMV3D6Fjvr9z%2BkPOxwNHEhLBsRNxM96YYyXr4txSKAmzyCaDHGsTwnBPXlihJ6KMYn3EifaJLM5IpJZlOdJJt1IP15s/AWb8haf0KfKYpcCnKkBcm5foz0/KVP8Lqc4xp4i6hirrLWXAEQQnCh0%2B06CzawJ6dg/pds8pDIGZGF2YyyEVQ9oo2qMyaFzKFIHcsIcw4sJ6h2PqmzeoJ1Grsia%2BzBG8GEdOURZyC5SKLpuG5kyvYPNUVXZRmiLqAOuk%2BL5BiW6/M/P8t6gKe4gt%2BoPAGEKHG4uxi4yGFEyV5FRgIPxRaGAFsxSTElTiN4EoRfvbFBNaZkuScza%2BqDOl0p5k/fmr937CysmLIpP9OXS15R5f%2Bgr1iSFMpIDQCJmlSEkGaPU8qkpKpKVyspyD20KowX/BBIBGnnBii0sVGh1gInnesbWrMs1rjxC6W%2Bu6N3yyzU%2BtdI7SB9gas4yQQA%3D%3D%3D)): ![image](https://user-images.githubusercontent.com/10009354/209678087-977d494f-272c-4779-a3a5-994cf6da097a.png)
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Jan 3, 2023

Copy link
Copy Markdown
Member

Andy Jordan (@andschwa) - can you help with this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andyleejordan

Andy Jordan (andyleejordan) commented Jan 4, 2023

Copy link
Copy Markdown
Member

Aditya Patwardhan (@adityapatwardhan) Yes, taking a look.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andyleejordan

Andy Jordan (andyleejordan) commented Jan 5, 2023

Copy link
Copy Markdown
Member

Ok, I'm confirming that the generated build files (for a release build) are currently including:

-DFORTIFY_SOURCE=2 -O2 -O3 -DNDEBUG

so I think we should edit this PR to remove the -O2 specification, as CMake is already adding -O3 for release builds.

Disconnect3d (@disconnect3d) is correct and this macro is currently doing nothing, as it needs the prepended underscore.

I like the idea of setting _FORTIFY_SOURCE=3 but with it being so new, I don't think it's worth the risk of unforeseen consequences for now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andyleejordan
Remove superfluous -O2 flag
0ad4870 
Since CMake adds `-O3` for `Release` builds anyway.
@andyleejordan

Andy Jordan (andyleejordan) commented Jan 5, 2023

Copy link
Copy Markdown
Member

For reference Aditya Patwardhan (@adityapatwardhan), from the CMakeCache.txt file:

//Flags used by the CXX compiler during RELEASE builds.
CMAKE_CXX_FLAGS_RELEASE:STRING=-O3 -DNDEBUG

//Flags used by the CXX compiler during RELWITHDEBINFO builds.
CMAKE_CXX_FLAGS_RELWITHDEBINFO:STRING=-O2 -g -DNDEBUG

and I confirmed with the commit I just added to Disconnect3d (@disconnect3d)'s branch that the generated flags are now:

CXX_FLAGSarm64 =  -std=c++11 -Wall -Werror -fstack-protector-strong -fpie -D_FORTIFY_SOURCE=2 -O3 -DNDEBUG -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk -fPIC -g -mmacosx-version-min=10.9

so no more superfluous -O2 and the macro is correctly specified.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
andyleejordan
Andy Jordan (andyleejordan) approved these changes Jan 5, 2023
View reviewed changes
@andyleejordan
Andy Jordan (andyleejordan) merged commit af5359b into PowerShell:master Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@andyleejordan Andy Jordan (andyleejordan) andyleejordan approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@disconnect3d @adityapatwardhan @andyleejordan
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.