crypto: fix missing nullptr check on RSA_new()

[ndossche]

Not checking this can cause a null deref. Since there is already a null check at the bottom of the function with NewRSA().

Note: this was found by a static-dynamic analyser I'm developing.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.74%. Comparing base (4a13a62) to head (00b4cf8).
⚠️ Report is 41 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_rsa.cc	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61888      +/-   ##
==========================================
- Coverage   89.76%   89.74%   -0.02%     
==========================================
  Files         675      674       -1     
  Lines      204674   204809     +135     
  Branches    39330    39361      +31     
==========================================
+ Hits       183716   183808      +92     
- Misses      13235    13272      +37     
- Partials     7723     7729       +6     

Files with missing lines	Coverage Δ
src/crypto/crypto_rsa.cc	63.39% <0.00%> (-0.21%)	⬇️

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

[addaleax]

Why does this not throw an exception like the other return {}; sites?

[ndossche]

I was confused about the exception handling. Line 438 with the EVPKeyPointer::NewRSA also does not do any exception throwing. Yet when I forced this rsa check to fail at this line during testing I did get an exception when running some tests using ./tools/test.py.

[addaleax]

Right, I'd still expect some tests to fail in that case, maybe because something down the call stack throws an exception or just because the test doesn't properly work if key parsing doesn't work.

But that's bad practice, and methods should be consistent about whether they schedule an exception or not along with an empty return value (ideally signified by returning Maybe<> or MaybeLocal<> values)