crypto: fix handling of null BUF_MEM* in ToV8Value()

[ndossche]

The assignment to bptr calls BIO_get_mem_ptr which can fail and leave the bptr as nullptr. This then later causes a null pointer deref.
This is inconsistent with uses of the similar function BIO_get_mem_data that do check its return value, e.g. node::crypto::X509sToArrayOfStrings().
Solve it by checking for a null pointer and handling the Nothing return value at the call sites.

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 29.41176% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.74%. Comparing base (4f13746) to head (5632d1f).
⚠️ Report is 23 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61885      +/-   ##
==========================================
+ Coverage   89.72%   89.74%   +0.02%     
==========================================
  Files         675      674       -1     
  Lines      204806   204823      +17     
  Branches    39355    39368      +13     
==========================================
+ Hits       183761   183821      +60     
+ Misses      13330    13277      -53     
- Partials     7715     7725      +10     

Files with missing lines	Coverage Δ
src/crypto/crypto_keys.cc	73.88% <45.45%> (-0.20%)	⬇️
src/crypto/crypto_x509.cc	72.44% <0.00%> (-0.64%)	⬇️

... and 40 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]

The BUF_MEM* changes are fundamentally reasonable, although they are a strong argument why they really should not be using implicit casting operators. However, both looking at the OpenSSL source code and documentation, there seems to be no indication that these would be able to fail.

The ToV8Value() changes are definitely incorrect; if ToV8Value() does not already schedule an exception to be thrown when returning an empty value, that's a bug in ToV8Value() itself.

[ndossche]

However, both looking at the OpenSSL source code and documentation, there seems to be no indication that these would be able to fail.

I suppose it depends. The OpenSSL documentation is incomplete as usual and doesn't indicate what the return value means. Looking at the source, a registered callback could still trigger a failure. LibreSSL puts the failure more explicitly: https://man.openbsd.org/BIO_s_mem.3 where it indicates "return 1 on success or a value less than or equal to 0 if an error occurred."

[addaleax]

@ndossche Yeah, and like I said, it's a fundamentally reasonable change. My "request changes" bit primarily refers to the ToV8Value() error handling.