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.

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

feat(msvs): add SpectreMitigation attribute #190

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Mar 2, 2023

Backport of https://chromium-review.googlesource.com/c/external/gyp/+/4265144

Description from upstream:

Add SpectreMitigation attribute for msvs

This CL allows gyp to recognize the SpectreMitigation msvs_attribute
and add it to the generated vcxproj file for MSBuild.
Possible values for the attribute are Spectre, SpectreLoad,
SpectreLoadCF, and false.

The /Qspectre compiler option is not enough to add
full Spectre mitigation, because even though it causes MSBuild
to add additional instructions to the generated object files,
it does not cause MSBuild to link against Spectre-mitigated
libraries provided by Visual Studio.

@rzhao271
Copy link
Contributor Author

Hi @cclauss, is this PR ready to be merged?

@cclauss
Copy link
Contributor

cclauss commented Mar 10, 2023

I am not a Windows user so a maintainer with that knowledge will need to approve.
@nodejs/platform-windows

@rzhao271
Copy link
Contributor Author

Could a reviewer from platform-windows take a look? This PR is blocking node-gyp, and in turn, native node modules, from adopting Spectre Mitigation in accordance with microsoft/binskim policies.

@targos
Copy link
Member

targos commented Mar 24, 2023

/cc @joaocgreis @StefanStojanovic

Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

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

LGTM! To add a few things I noticed while testing:

  • Since VS2015 is supported for building Node.JS Addons, and SpectreMitigation is available only in VS2017 and later, if VS2015 is used adding this attribute will have no effect.
  • Using VS that supports SpectreMitigation (I tested on VS2017), compilation will fail if the wrong value is set or a required component is not installed. The messages in these cases are very informative, so users should know what to do.

@targos targos merged commit 853e464 into nodejs:main Mar 30, 2023
@rzhao271 rzhao271 deleted the rzhao271/bp-spectre-mitigation branch March 30, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants