-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
crypto: reduce memory usage of SignFinal #23427
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.
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
Changes from 1 commit
4b4b71b
e5bd9d0
202d506
27f5947
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
The fixed-size buffer on the stack is unnecessary and way too large for most applications. This change removes it and allocates the required memory directly instead of copying into heap later.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -3517,36 +3517,38 @@ void Sign::SignUpdate(const FunctionCallbackInfo<Value>& args) { | |||
| sign->CheckThrow(err); | ||||
| } | ||||
|
|
||||
| static int Node_SignFinal(EVPMDPointer&& mdctx, unsigned char* md, | ||||
| unsigned int* sig_len, | ||||
| const EVPKeyPointer& pkey, int padding, | ||||
| int pss_salt_len) { | ||||
| static MallocedBuffer<unsigned char> Node_SignFinal(EVPMDPointer&& mdctx, | ||||
| const EVPKeyPointer& pkey, | ||||
| int padding, | ||||
| int pss_salt_len) { | ||||
| unsigned char m[EVP_MAX_MD_SIZE]; | ||||
| unsigned int m_len; | ||||
|
|
||||
| *sig_len = 0; | ||||
| if (!EVP_DigestFinal_ex(mdctx.get(), m, &m_len)) | ||||
| return 0; | ||||
| return MallocedBuffer<unsigned char>(); | ||||
|
|
||||
| int signed_sig_len = EVP_PKEY_size(pkey.get()); | ||||
| CHECK_GE(signed_sig_len, 0); | ||||
| size_t sig_len = static_cast<size_t>(signed_sig_len); | ||||
| MallocedBuffer<unsigned char> sig(sig_len); | ||||
|
|
||||
| size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey.get())); | ||||
| EVPKeyCtxPointer pkctx(EVP_PKEY_CTX_new(pkey.get(), nullptr)); | ||||
| if (pkctx && | ||||
| EVP_PKEY_sign_init(pkctx.get()) > 0 && | ||||
| ApplyRSAOptions(pkey, pkctx.get(), padding, pss_salt_len) && | ||||
| EVP_PKEY_CTX_set_signature_md(pkctx.get(), | ||||
| EVP_MD_CTX_md(mdctx.get())) > 0 && | ||||
| EVP_PKEY_sign(pkctx.get(), md, &sltmp, m, m_len) > 0) { | ||||
| *sig_len = sltmp; | ||||
| return 1; | ||||
| EVP_PKEY_sign(pkctx.get(), sig.data, &sig_len, m, m_len) > 0) { | ||||
| return MallocedBuffer<unsigned char>(sig.release(), sig_len); | ||||
| } | ||||
| return 0; | ||||
|
|
||||
| return MallocedBuffer<unsigned char>(); | ||||
| } | ||||
|
|
||||
| SignBase::Error Sign::SignFinal(const char* key_pem, | ||||
| int key_pem_len, | ||||
| const char* passphrase, | ||||
| unsigned char* sig, | ||||
| unsigned int* sig_len, | ||||
| MallocedBuffer<unsigned char>* buffer, | ||||
|
||||
| int padding, | ||||
| int salt_len) { | ||||
| if (!mdctx_) | ||||
|
|
@@ -3591,10 +3593,8 @@ SignBase::Error Sign::SignFinal(const char* key_pem, | |||
| } | ||||
| #endif // NODE_FIPS_MODE | ||||
|
|
||||
| if (Node_SignFinal(std::move(mdctx), sig, sig_len, pkey, padding, salt_len)) | ||||
| return kSignOk; | ||||
| else | ||||
| return kSignPrivateKey; | ||||
| *buffer = Node_SignFinal(std::move(mdctx), pkey, padding, salt_len); | ||||
| return buffer->is_empty() ? kSignPrivateKey : kSignOk; | ||||
| } | ||||
|
|
||||
|
|
||||
|
|
@@ -3618,22 +3618,20 @@ void Sign::SignFinal(const FunctionCallbackInfo<Value>& args) { | |||
| int salt_len = args[3].As<Int32>()->Value(); | ||||
|
|
||||
| ClearErrorOnReturn clear_error_on_return; | ||||
| unsigned char md_value[8192]; | ||||
| unsigned int md_len = sizeof(md_value); | ||||
| MallocedBuffer<unsigned char> sig; | ||||
|
||||
| return MallocedBuffer<unsigned char>(sig.release(), sig_len); |
And a resizable contiguous container is literally the definition of
std::vector. AFAIU since it's a part of the language compilers can minimize heap allocations
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sig.release() :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhhhh right thank you so much!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very important point... I've been thinking about how to minimize that.
I was thinking about suggesting a unique_ptr<T> in MallocedBuffer instead of the naked pointer. That will make users either buf.get() or buf.release() (or pass by unique_ptr value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you constructing a new
MallocedBuffer?