Skip to content

crypto,quic: add NULL checks for OpenSSL allocation functions#63040

Open
armorbreak001 wants to merge 2 commits into
nodejs:mainfrom
armorbreak001:fix/crypto-null-checks
Open

crypto,quic: add NULL checks for OpenSSL allocation functions#63040
armorbreak001 wants to merge 2 commits into
nodejs:mainfrom
armorbreak001:fix/crypto-null-checks

Conversation

@armorbreak001
Copy link
Copy Markdown

@armorbreak001 armorbreak001 commented Apr 29, 2026

Fixes: #62774

This PR adds graceful NULL checks for OpenSSL allocation function return values, replacing CHECK() assertions that would abort the process on allocation failure.

Changes

src/crypto/crypto_aes.ccAES_Cipher()

Replaced CHECK(ctx) with:

if (!ctx) {
  return WebCryptoCipherStatus::FAILED;
}

This matches the pattern already used in AES_CTR_Cipher2() in the same file.

src/crypto/crypto_cipher.ccCipherBase::CommonInit()

Replaced CHECK(ctx_) with:

if (!ctx_) {
  return ThrowCryptoError(env(),
                          mark_pop_error_on_return.peekError(),
                          "Failed to allocate cipher context");
}

This matches the error handling pattern used elsewhere in the same function.

Note on other locations from #62774

  • AES_CTR_Cipher2() — Already has a proper if (!ctx) check (line 238)
  • TLSSession::Initialize() — Already has a proper if (!ssl) check (line 794)
  • ECKeyExportTraits::DoExport() — This function no longer exists in the current codebase; the EC key handling code has been refactored

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@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 Apr 29, 2026
@armorbreak001 armorbreak001 force-pushed the fix/crypto-null-checks branch from 234ce29 to 78ab09a Compare April 29, 2026 20:59
@armorbreak001 armorbreak001 changed the title crypto: add NULL checks for OpenSSL allocation functions crypto,quic: add NULL checks for OpenSSL allocation functions Apr 29, 2026
@armorbreak001 armorbreak001 force-pushed the fix/crypto-null-checks branch from 78ab09a to cb76fb6 Compare May 16, 2026 01:19
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@codecov
Copy link
Copy Markdown

codecov Bot commented May 16, 2026

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.06%. Comparing base (5d578c5) to head (cb76fb6).
⚠️ Report is 138 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_aes.cc 0.00% 1 Missing and 1 partial ⚠️
src/crypto/crypto_cipher.cc 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63040      +/-   ##
==========================================
+ Coverage   89.65%   90.06%   +0.41%     
==========================================
  Files         708      714       +6     
  Lines      220402   225518    +5116     
  Branches    42269    42645     +376     
==========================================
+ Hits       197597   203110    +5513     
+ Misses      14671    14198     -473     
- Partials     8134     8210      +76     
Files with missing lines Coverage Δ
src/crypto/crypto_aes.cc 53.63% <0.00%> (-0.19%) ⬇️
src/crypto/crypto_cipher.cc 77.28% <0.00%> (-0.16%) ⬇️

... and 146 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Replace CHECK() assertions with graceful error handling for
EVP_CIPHER_CTX_new() allocations that could fail under memory
pressure:

- crypto_aes.cc (AES_Cipher): return FAILED status
- crypto_cipher.cc (CommonInit): throw JS error via ThrowCryptoError

Fixes nodejs#62774

Signed-off-by: armorbreak001 <contact@agentvote.cc>
@armorbreak001 armorbreak001 force-pushed the fix/crypto-null-checks branch from cb76fb6 to 3899839 Compare May 16, 2026 07:07
Signed-off-by: armorbreak001 <contact@agentvote.cc>
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. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

crypto,quic: missing NULL checks for OpenSSL allocation functions

3 participants