Skip to content

crypto: fix missing nullptr check on RSA_new() #61888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ndossche wants to merge 1 commit into
base: main
Choose a base branch
from
+2 −0
Open

crypto: fix missing nullptr check on RSA_new() #61888

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/crypto/crypto_rsa.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,8 @@ KeyObjectData ImportJWKRsaKey(Environment* env,
KeyType type = d_value->IsString() ? kKeyTypePrivate : kKeyTypePublic;

RSAPointer rsa(RSA_new());
if (!rsa) return {};
Copy link
Member

@addaleax addaleax Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@ndossche ndossche Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@addaleax addaleax Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


ncrypto::Rsa rsa_view(rsa.get());

ByteSource n = ByteSource::FromEncodedString(env, n_value.As<String>());
Expand Down
Loading