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

msi: enable CFG in node.exe #42126

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

msi: enable CFG in node.exe #42126

wants to merge 4 commits into from

Conversation

ahtrahdis7
Copy link

Links Issue : #42100
#42100

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. windows Issues and PRs related to the Windows platform. labels Feb 25, 2022
@targos
Copy link
Member

targos commented Feb 25, 2022

Windows CI (including tests on arm64): https://ci.nodejs.org/job/node-test-commit-windows-fanned/47188/

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

There is a compilation error:

MSBUILD : error MSB1001: Unknown switch.
Switch: /guard:cf

Added some suggestions which should hopefully help you fix that.

vcbuild.bat Outdated Show resolved Hide resolved
vcbuild.bat Outdated
@@ -508,7 +509,7 @@ if not defined msi goto install-doctools
echo Building node-v%FULLVERSION%-%target_arch%.msi
set "msbsdk="
if defined WindowsSDKVersion set "msbsdk=/p:WindowsTargetPlatformVersion=%WindowsSDKVersion:~0,-1%"
msbuild "%~dp0tools\msvs\msi\nodemsi.sln" /m /t:Clean,Build %msbsdk% /p:PlatformToolset=%PLATFORM_TOOLSET% /p:WixSdkDir="%WIXSDKDIR%" /p:Configuration=%config% /p:Platform=%target_arch% /p:NodeVersion=%NODE_VERSION% /p:FullVersion=%FULLVERSION% /p:DistTypeDir=%DISTTYPEDIR% %noetw_msi_arg% /clp:NoSummary;NoItemAndPropertyList;Verbosity=minimal /nologo
msbuild "%~dp0tools\msvs\msi\nodemsi.sln" /m /t:Clean,Build %msbsdk% /p:PlatformToolset=%PLATFORM_TOOLSET% /p:WixSdkDir="%WIXSDKDIR%" /p:Configuration=%config% /p:Platform=%target_arch% /p:NodeVersion=%NODE_VERSION% /p:FullVersion=%FULLVERSION% /p:DistTypeDir=%DISTTYPEDIR% %noetw_msi_arg% /clp:NoSummary;NoItemAndPropertyList;Verbosity=minimal /nologo /guard:cf /DYNAMICBASE
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, /DYNAMICBASE is something that should be passed to link. To do that, you can try adding <AdditionalOptions>/DYNAMICBASE /GUARD:CF</AdditionalOptions> to the list of <Link> options.

@ahtrahdis7
Copy link
Author

@RaisinTen How can I get the binary built for this PR. I need to run test for CFG enablement.

@ahtrahdis7 ahtrahdis7 changed the title add changes to vcbuild.bat to enable CFG in node.exe msi: enable CFG in node.exe Feb 28, 2022
@ahtrahdis7
Copy link
Author

The suggested solution ain't working. I tried multiple combinations.
Did generate a *.vcxproj file from visual studio with CFG enabled as mentioned in this link https://docs.microsoft.com/en-us/windows/win32/secbp/control-flow-guard . Didn't get any satisfactory results.

@RaisinTen
Copy link
Contributor

RaisinTen commented Feb 28, 2022

@RaisinTen How can I get the binary built for this PR. I need to run test for CFG enablement.

I don't think we provide a way to download the artifacts generated from PRs. You probably need to build it yourself. Here is the building guide for Windows - https://github.com/nodejs/node/blob/HEAD/BUILDING.md#windows.

The suggested solution ain't working. I tried multiple combinations.
Did generate a *.vcxproj file from visual studio with CFG enabled as mentioned in this link https://docs.microsoft.com/en-us/windows/win32/secbp/control-flow-guard . Didn't get any satisfactory results.

I guess Richard is right in #42126 (comment), this needs a change in the gyp files. The vcxproj file is only used to build the msi, not the node.exe executable.

@ahtrahdis7
Copy link
Author

Can anyone suggest something more concrete ??

@bjendyk
Copy link

bjendyk commented Oct 13, 2022

@richardlau Which gyp files should be modified to enable CFG when compiling node.js sources?

@richardlau
Copy link
Member

Probably common.gypi:

node/common.gypi

Lines 279 to 318 in 03fb789

'msvs_settings': {
'VCCLCompilerTool': {
'AdditionalOptions': [
'/Zc:__cplusplus',
'-std:c++17'
],
'BufferSecurityCheck': 'true',
'target_conditions': [
['_toolset=="target"', {
'DebugInformationFormat': 1 # /Z7 embed info in .obj files
}],
],
'ExceptionHandling': 0, # /EHsc
'MultiProcessorCompilation': 'true',
'StringPooling': 'true', # pool string literals
'SuppressStartupBanner': 'true',
'WarnAsError': 'false',
'WarningLevel': 3, # /W3
},
'VCLinkerTool': {
'target_conditions': [
['_type=="executable"', {
'SubSystem': 1, # /SUBSYSTEM:CONSOLE
}],
],
'conditions': [
['target_arch=="ia32"', {
'TargetMachine' : 1, # /MACHINE:X86
}],
['target_arch=="x64"', {
'TargetMachine' : 17, # /MACHINE:X64
}],
['target_arch=="arm64"', {
'TargetMachine' : 0, # NotSet. MACHINE:ARM64 is inferred from the input files.
}],
],
'GenerateDebugInformation': 'true',
'SuppressStartupBanner': 'true',
},
},

I think /dynamicbase can be enabled via RandomizedBaseAddress:
https://github.com/nodejs/gyp-next/blob/52de08ab53ed5e96a4061aec42d10737f6a8cc91/pylib/gyp/msvs_emulation.py#L723
I didn't see anything in gyp-next for enabling /guard so you'd probably need to add AdditionalOptions (you may need to do this twice -- once for the compiler and once for the liner settings).

cc @nodejs/platform-windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants