Skip to content
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

crypto: reduce memory usage of SignFinal #23427

Closed
wants to merge 4 commits into from
Closed

crypto: reduce memory usage of SignFinal #23427

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b4b71b
crypto: reduce memory usage of SignFinal
tniessen Oct 11, 2018
e5bd9d0
fixup! crypto: reduce memory usage of SignFinal
tniessen Oct 12, 2018
202d506
fixup! crypto: reduce memory usage of SignFinal
tniessen Oct 15, 2018
27f5947
fixup! crypto: reduce memory usage of SignFinal
tniessen Oct 17, 2018
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
42 changes: 20 additions & 22 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

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?

}
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,
Copy link
Contributor

@refack refack Oct 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Could you return an std::pair instead of the out-param?
Or even leverage move semantic, to pass by value.

int padding,
int salt_len) {
if (!mdctx_)
Expand Down Expand Up @@ -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;
}


Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the MallocedBuffer abstraction fails to fit, maybe create an std::vector<unsigned char> here and conditionally fill it in SignFinal.
If you know the maximal size you could even use an std::array<unsigned char> and save all the memory allocations.

Copy link
Member

Choose a reason for hiding this comment

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

@refack That would undo the purpose of this change, which is to avoid extra allocations

Copy link
Contributor

@refack refack Oct 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

  1. If we can do this on the stack it's free.
  2. If we use a vector with move semantics, there's no allocation until we resize.
  3. As you mentioned, this change's main benefit is readability.

And we have several guidelines that this breaks:
Google's:

  1. Local Variables - Place a function's variables in the narrowest scope possible, and initialize variables in the declaration.
  2. Output Parameters - Prefer using return values rather than output parameters. If output-only parameters are used they should appear after input parameters.
  3. C++11 Use libraries and language extensions from C++11 when appropriate.

C++CG:

  1. ES.20: Always initialize an object
  2. F.20: For “out” output values, prefer return values to output parameters
  3. ES.1: Prefer the standard library to other libraries and to “handcrafted code”

Copy link
Member

Choose a reason for hiding this comment

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

Local Variables - Place a function's variables in the narrowest scope possible, and initialize variables in the declaration.

I don’t see that being broken here.

Output Parameters - Prefer using return values rather than output parameters. If output-only parameters are used they should appear after input parameters.

That would go away if we implemented a std::pair return type, as you suggested. I 👍’ed that comment.

C++11 Use libraries and language extensions from C++11 when appropriate.

I don’t see that being broken here.

ES.20: Always initialize an object

I don’t see that being broken here. Again, the rule explicitly lists it as a valid exception if the object is about to be initialized.

F.20: For “out” output values, prefer return values to output parameters

(same as above, std::pair would “solve” this)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, if we use a pair we get rid of most of the anti-patterns. That's my requested-change.

As for whether MallocedBuffer is the best abstraction for this case, that's an open question I pose to @tniessen.

IMHO it breaks since we need to resize in:

node/src/node_crypto.cc

Line 3542 in e5bd9d0

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


Error err = sign->SignFinal(
buf,
buf_len,
len >= 2 && !args[1]->IsNull() ? *passphrase : nullptr,
md_value,
&md_len,
&sig,
padding,
salt_len);
if (err != kSignOk)
return sign->CheckThrow(err);

Local<Object> rc =
Buffer::Copy(env, reinterpret_cast<char*>(md_value), md_len)
Buffer::New(env, reinterpret_cast<char*>(sig.data), sig.size)
Copy link
Member

Choose a reason for hiding this comment

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

sig.release() :)

Copy link
Member Author

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!!!

Copy link
Contributor

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

.ToLocalChecked();
args.GetReturnValue().Set(rc);
}
Expand Down
3 changes: 1 addition & 2 deletions src/node_crypto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,8 +521,7 @@ class Sign : public SignBase {
Error SignFinal(const char* key_pem,
int key_pem_len,
const char* passphrase,
unsigned char* sig,
unsigned int* sig_len,
MallocedBuffer<unsigned char>* sig,
int padding,
int saltlen);

Expand Down