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

@armorbreak001 armorbreak001 commented Apr 29, 2026

Copy link
Copy Markdown

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

codecov Bot commented May 16, 2026

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Patch % Lines
src/crypto/crypto_cipher.cc 0.00% 2 Missing and 1 partial ⚠️
src/crypto/crypto_aes.cc 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63040      +/-   ##
==========================================
+ Coverage   89.65%   90.05%   +0.39%     
==========================================
  Files         708      714       +6     
  Lines      220402   225525    +5123     
  Branches    42269    42642     +373     
==========================================
+ Hits       197597   203087    +5490     
+ Misses      14671    14205     -466     
- Partials     8134     8233      +99     
Files with missing lines Coverage Δ
src/crypto/crypto_aes.cc 53.63% <0.00%> (-0.19%) ⬇️
src/crypto/crypto_cipher.cc 77.13% <0.00%> (-0.31%) ⬇️

... and 150 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