Skip to content

crypto: replace CHECK with NULL checks for EVP_CIPHER_CTX_new allocations#62816

Open
Jah-yee wants to merge 1 commit intonodejs:mainfrom
Jah-yee:fix/missing-null-check-openssl-alloc
Open

crypto: replace CHECK with NULL checks for EVP_CIPHER_CTX_new allocations#62816
Jah-yee wants to merge 1 commit intonodejs:mainfrom
Jah-yee:fix/missing-null-check-openssl-alloc

Conversation

@Jah-yee
Copy link
Copy Markdown

@Jah-yee Jah-yee commented Apr 19, 2026

Good day

This PR addresses issue #62774, which identifies missing NULL checks for OpenSSL allocation functions in the Node.js crypto module.

Changes

src/crypto/crypto_aes.cc

In the `AES_Cipher` function, replaced `CHECK(ctx)` with a proper NULL check:

```cpp
auto ctx = CipherCtxPointer::New();
if (!ctx) {
return WebCryptoCipherStatus::FAILED;
}
```

src/crypto/crypto_cipher.cc

In the `CipherBase::CommonInit` function, replaced `CHECK(ctx_)` with a proper error throw:

```cpp
ctx_ = CipherCtxPointer::New();
if (!ctx_) {
return ThrowCryptoError(env(),
mark_pop_error_on_return.peekError(),
"EVP_CIPHER_CTX_new");
}
```

Why These Changes

The `CHECK()` macro is an assertion that calls `abort()` on failure. This is not appropriate for recoverable allocation failures (such as out-of-memory conditions). The new code:

  1. Checks for NULL after `EVP_CIPHER_CTX_new()` allocation
  2. Returns a proper error status (`WebCryptoCipherStatus::FAILED` or `ThrowCryptoError`)
  3. Follows the same pattern already used in `AES_CTR_Cipher2` in the same file

This makes the code consistent with the rest of the codebase and handles allocation failures gracefully instead of crashing.

Thank you for your attention. If there are any issues or suggestions, please leave a comment and I will address them promptly.

Warmly,
RoomWithOutRoof

…ions

Replace CHECK() assertions with proper NULL checks and error returns
for EVP_CIPHER_CTX_new() allocations. CHECK() causes an abort()
on failure, which is not appropriate for recoverable allocation failures.
Instead, return a failure status or throw a proper crypto error.

Fixes: nodejs#62774
@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 19, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 19, 2026

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.68%. Comparing base (58a8e1d) to head (b7db0e3).

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   #62816      +/-   ##
==========================================
- Coverage   89.69%   89.68%   -0.02%     
==========================================
  Files         706      706              
  Lines      218222   218225       +3     
  Branches    41768    41766       -2     
==========================================
- Hits       195731   195709      -22     
- Misses      14411    14417       +6     
- Partials     8080     8099      +19     
Files with missing lines Coverage Δ
src/crypto/crypto_aes.cc 53.63% <0.00%> (-0.54%) ⬇️
src/crypto/crypto_cipher.cc 77.13% <0.00%> (-0.31%) ⬇️

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

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.

2 participants