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

tls: fix error stack conversion in cryptoErrorListToException() #56554

Merged
merged 1 commit into from
Jan 12, 2025

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 10, 2025

The ncrypto move introduced regressions in
cryptoErrorListToException() by passing in the size of the vector unnecessarily into the vector constructor and then use push_back() (which would result in a crash on dereferencing empty handles during later iteration) and having incorrect logic for checking the presence of an exception. This patch fixes it.

Fixes: #56375
Refs: #53803

The ncrypto move introduced regressions in
cryptoErrorListToException() by passing in the size of the
vector unnecessarily into the vector constructor and then use
push_back() (which would result in a crash on dereferencing empty
handles during later iteration) and having incorrect logic for
checking the presence of an exception. This patch fixes it.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2025
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Jan 10, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 10, 2025

Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.18%. Comparing base (19c8cc1) to head (c666e67).
Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_util.cc 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56554      +/-   ##
==========================================
+ Coverage   89.16%   89.18%   +0.01%     
==========================================
  Files         662      662              
  Lines      191745   191752       +7     
  Branches    36902    36902              
==========================================
+ Hits       170971   171013      +42     
+ Misses      13627    13577      -50     
- Partials     7147     7162      +15     
Files with missing lines Coverage Δ
src/crypto/crypto_util.cc 72.47% <80.00%> (+4.87%) ⬆️

... and 32 files with indirect coverage changes

@jasnell jasnell added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jan 12, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 12, 2025
@nodejs-github-bot nodejs-github-bot merged commit f4fcf0e into nodejs:main Jan 12, 2025
74 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in f4fcf0e

targos pushed a commit that referenced this pull request Jan 13, 2025
The ncrypto move introduced regressions in
cryptoErrorListToException() by passing in the size of the
vector unnecessarily into the vector constructor and then use
push_back() (which would result in a crash on dereferencing empty
handles during later iteration) and having incorrect logic for
checking the presence of an exception. This patch fixes it.

PR-URL: #56554
Fixes: #56375
Refs: #53803
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jan 13, 2025
The ncrypto move introduced regressions in
cryptoErrorListToException() by passing in the size of the
vector unnecessarily into the vector constructor and then use
push_back() (which would result in a crash on dereferencing empty
handles during later iteration) and having incorrect logic for
checking the presence of an exception. This patch fixes it.

PR-URL: nodejs#56554
Fixes: nodejs#56375
Refs: nodejs#53803
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tls.createSecureContext results in an abort
7 participants