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 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.77%. Comparing base (4a13a62) to head (09ed002).
⚠️ Report is 68 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_rsa.cc	0.00%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61888      +/-   ##
==========================================
+ Coverage   89.76%   89.77%   +0.01%     
==========================================
  Files         675      674       -1     
  Lines      204674   205610     +936     
  Branches    39330    39413      +83     
==========================================
+ Hits       183716   184583     +867     
- Misses      13235    13287      +52     
- Partials     7723     7740      +17     

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

... and 152 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)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71437/