win,build: add option to enable cfg #56605
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review requested:
|
nodejs-github-bot
added
build
jasnell
approved these changes
Jan 15, 2025
richardlau
reviewed
Jan 15, 2025
No plan yet. CFG adds extra checks to the code to increase security. Therefore, these checks have a runtime cost in certain situations. Instead of enabling it by default, it might be enabled and released as a separate executable. |
huseyinacacak-janea
force-pushed
the
huseyin-10270-enable-cfg
branch
from
ebeaa73 to
9ddff4a
Compare
I've updated the PR description to give more information about CFG. |
StefanStojanovic
added
the
request-ci
github-actions
bot
removed
the
request-ci
|
Is there anything else I can do to help this PR move forward? |
|
Hey all, as I see there are no unresolved comments. In addition, this just enables Control Flow Guard, it doesn't enforce it in release builds (might open a follow-up PR for that). With all that in mind, I'd like to land this, so unless there is opposition, I'll land it on Monday (March 3). |
richardlau
approved these changes
Feb 28, 2025
StefanStojanovic
added
the
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/56605 ✔ Done loading data for nodejs/node/pull/56605 ----------------------------------- PR info ------------------------------------ Title win,build: add option to enable cfg (#56605) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch huseyinacacak-janea:huseyin-10270-enable-cfg -> nodejs:main Labels windows, build, needs-ci Commits 5 - win,build: add option to enable Control Flow Guard - Update common.gypi - Update configure.py - fix inconsistency in variables - Merge branch 'main' into huseyin-10270-enable-cfg Committers 2 - Hüseyin Açacak <huseyin@janeasystems.com> - GitHub <noreply@github.com> PR-URL: https://github.com/nodejs/node/pull/56605 Fixes: https://github.com/nodejs/node/issues/42100 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/56605 Fixes: https://github.com/nodejs/node/issues/42100 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 15 Jan 2025 09:50:33 GMT ✔ Approvals: 2 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56605#pullrequestreview-2552730898 ✔ - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/56605#pullrequestreview-2650622770 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-01-27T12:22:57Z: https://ci.nodejs.org/job/node-test-pull-request/64765/ ⚠ Commits were pushed after the last Full PR CI run: ⚠ - Merge branch 'main' into huseyin-10270-enable-cfg - Querying data for job/node-test-pull-request/64765/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/13628619689 |
StefanStojanovic
added
commit-queue-squash
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/56605 ✔ Done loading data for nodejs/node/pull/56605 ----------------------------------- PR info ------------------------------------ Title win,build: add option to enable cfg (#56605) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch huseyinacacak-janea:huseyin-10270-enable-cfg -> nodejs:main Labels windows, build, needs-ci, commit-queue-squash Commits 5 - win,build: add option to enable Control Flow Guard - Update common.gypi - Update configure.py - fix inconsistency in variables - Merge branch 'main' into huseyin-10270-enable-cfg Committers 2 - Hüseyin Açacak <huseyin@janeasystems.com> - GitHub <noreply@github.com> PR-URL: https://github.com/nodejs/node/pull/56605 Fixes: https://github.com/nodejs/node/issues/42100 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/56605 Fixes: https://github.com/nodejs/node/issues/42100 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 15 Jan 2025 09:50:33 GMT ✔ Approvals: 2 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/56605#pullrequestreview-2552730898 ✔ - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/56605#pullrequestreview-2650622770 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-03-03T10:26:06Z: https://ci.nodejs.org/job/node-test-pull-request/64765/ - Querying data for job/node-test-pull-request/64765/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 56605 From https://github.com/nodejs/node * branch refs/pull/56605/merge -> FETCH_HEAD ✔ Fetched commits as 47a59bde2aad..05b3a8aad7ef -------------------------------------------------------------------------------- Auto-merging common.gypi CONFLICT (content): Merge conflict in common.gypi Auto-merging configure.py Auto-merging vcbuild.bat error: could not apply 07f6da193f... win,build: add option to enable Control Flow Guard hint: After resolving the conflicts, mark them with hint: "git add/rm <pathspec>", then run hint: "git cherry-pick --continue". hint: You can instead skip this commit with "git cherry-pick --skip". hint: To abort and get back to the state before "git cherry-pick", hint: run "git cherry-pick --abort". hint: Disable this message with "git config set advice.mergeConflict false" ✘ Failed to apply patcheshttps://github.com/nodejs/node/actions/runs/13633314973 |
|
@StefanStojanovic This is failing to merge via commit queue because of the merge commit in this PR. |
Co-authored-by: Richard Lau <rlau@redhat.com>
Co-authored-by: Richard Lau <rlau@redhat.com>
StefanStojanovic
force-pushed
the
huseyin-10270-enable-cfg
branch
from
05b3a8a to
fd6368b
Compare
StefanStojanovic
removed
commit-queue-failed
Thanks for letting me know. I've resolved that and pushed the branch. Will run the CI later. When you get time, please take a look at it and reapprove if all seems good to you, as I'll need approval after the last push to land it. |
StefanStojanovic
added
the
request-ci
github-actions
bot
removed
the
request-ci
richardlau
approved these changes
Mar 4, 2025
StefanStojanovic
added
commit-queue-squash
nodejs-github-bot
removed
the
commit-queue
|
Landed in f161033 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This adds an option to
vcbuild.batto compile with CFG (Control Flow Guard).CFG is a programming or security concept used to ensure that the execution of a program follows its intended flow and prevents unauthorized or unintended behavior. It is often used to detect and mitigate attacks that exploit vulnerabilities, such as control flow hijacking, which occurs when an attacker manipulates the program’s control flow to execute malicious code.
More information about the CFG can be found at https://learn.microsoft.com/en-us/windows/win32/secbp/control-flow-guard
Fixes: #42100