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

build: fix C string encoding for PRODUCT_DIR_ABS #56111

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Dec 2, 2024

Since the PRODUCT_DIR_ABS gyp variable is meant to be used in a C string in the OpenSSL config, provide a version of it that actually provides it in a way that is always usable as a C string. Otherwise, unescaped characters in the path can mess with the string definitions; for example, building in paths on Windows whose directories start with valid or invalid escape sequences (e.g.: C:\...\x61foobar\... or C:\...\456789\...) can result in failing builds or incorrect paths provided to OpenSSL.

Needs: nodejs/gyp-next#271

@addaleax addaleax marked this pull request as draft December 2, 2024 13:52
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. gyp Issues and PRs related to the GYP tool and .gyp build files needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. tools Issues and PRs related to the tools directory. labels Dec 2, 2024
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (f184a0a) to head (486ff15).
Report is 26 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #56111   +/-   ##
=======================================
  Coverage   88.53%   88.54%           
=======================================
  Files         657      657           
  Lines      189858   189858           
  Branches    36450    36447    -3     
=======================================
+ Hits       168089   168103   +14     
+ Misses      14973    14972    -1     
+ Partials     6796     6783   -13     

see 32 files with indirect coverage changes

@addaleax addaleax force-pushed the gyp-handle-cstring-productdir branch from e0c8ce0 to c2bec3f Compare December 4, 2024 11:41
@addaleax addaleax marked this pull request as ready for review December 4, 2024 11:41
@addaleax addaleax added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Dec 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 4, 2024
@nodejs-github-bot
Copy link
Collaborator

@gengjiawen
Copy link
Member

@addaleax The build is broken, can you take a look ? also long time no see :)

@addaleax
Copy link
Member Author

addaleax commented Dec 9, 2024

@gengjiawen 👋

Yeah, thanks for pointing this out – nodejs/gyp-next#274 should fix this again (sorry, I applied the suggestion provided there without checking that it works when integrated into Node.js again)

Since the `PRODUCT_DIR_ABS` gyp variable is meant to be used
in a C string in the OpenSSL config, provide a version of it
that actually provides it in a way that is always usable as a
C string. Otherwise, unescaped characters in the path can mess
with the string definitions; for example, building in paths
on Windows whose directories start with valid or invalid escape
sequences (e.g.: `C:\...\x61foobar\...` or `C:\...\456789\...`)
can result in failing builds or incorrect paths provided to
OpenSSL.
@addaleax addaleax force-pushed the gyp-handle-cstring-productdir branch from c2bec3f to 486ff15 Compare December 10, 2024 19:06
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2024
@addaleax
Copy link
Member Author

@gengjiawen Updated! 🙂

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2024
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 12, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 12, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in e5524ea...203398d

nodejs-github-bot pushed a commit that referenced this pull request Dec 12, 2024
PR-URL: #56111
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Dec 12, 2024
Since the `PRODUCT_DIR_ABS` gyp variable is meant to be used
in a C string in the OpenSSL config, provide a version of it
that actually provides it in a way that is always usable as a
C string. Otherwise, unescaped characters in the path can mess
with the string definitions; for example, building in paths
on Windows whose directories start with valid or invalid escape
sequences (e.g.: `C:\...\x61foobar\...` or `C:\...\456789\...`)
can result in failing builds or incorrect paths provided to
OpenSSL.

PR-URL: #56111
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dependencies Pull requests that update a dependency file. gyp Issues and PRs related to the GYP tool and .gyp build files needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants