crypto: reduce memory usage of SignFinal #23427

tniessen · 2018-10-11T18:46:17Z

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.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2018-10-11T18:46:19Z

@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1204/pipeline

sam-github · 2018-10-11T19:27:32Z

src/node_crypto.cc


  Error err = sign->SignFinal(
      buf,
      buf_len,
      len >= 2 && !args[1]->IsNull() ? *passphrase : nullptr,
-      md_value,
+      &md_value,


is this function, SignFinal overloaded? because an array value, and a pointer to an uninitialized pointer are very different.

sam-github · 2018-10-11T19:29:50Z

src/node_crypto.cc

@@ -3597,22 +3594,22 @@ 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);
+  unsigned char* md_value;


I suggest initializing to nullptr, even though its going to be allocated by SignFinal, unless leaving floating pointers around is the convention in node_crypto.cc.

sam-github · 2018-10-11T19:33:04Z

src/node_crypto.cc

      &md_len,
      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*>(md_value), md_len)


Sorry Tobias, I'm just looking at the PR, not digging into the APIs. Is md_value a pointer to memory owned elsewhere? Who is responsible for deallocating it? I can't tell from these APIs calls if its leaking, if it is a pointer to memory that is managed as part of sign so doesn't need deallocating, or whether its allocated and Buffer:New() is accepting responsibility for deallocating it.

bnoordhuis · 2018-10-11T21:08:15Z

src/node_crypto.cc

-    return 0;
+    return false;
+
+  *sig_len = static_cast<size_t>(EVP_PKEY_size(pkey.get()));


EVP_PKEY_size() returns an int. Casting to unsigned means a negative value will wrap around.

While unlikely, there are one or two key types where this function can potentially fail (with RSA and DSA keys.)

Long story short, please CHECK_GE(key_size, 0) first. :-)

bnoordhuis · 2018-10-11T21:12:43Z

src/node_crypto.cc

+    return false;
+
+  *sig_len = static_cast<size_t>(EVP_PKEY_size(pkey.get()));
+  *md = reinterpret_cast<unsigned char*>(node::Malloc(*sig_len));


This allocation appears to leak when returning false below.

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.

addaleax · 2018-10-12T21:40:11Z

src/node_crypto.cc

      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)


sig.release() :)

Ohhhhh right thank you so much!!!

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

addaleax · 2018-10-14T20:20:19Z

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

Btw, I’m not sure about the commit message, because it seems unlikely that this actually reduces memory usage. This does clean the code up a bit, and it saves a small extra copy operation, though :)

Guillerbr · 2018-10-14T20:36:22Z

good!

tniessen · 2018-10-14T21:41:20Z

Btw, I’m not sure about the commit message, because it seems unlikely that this actually reduces memory usage.

In theory, it should (8KB is quite a large buffer compared to usual memory page sizes). Whether that actually happens is up to the OS / compiler. I can change the commit message if you'd like.

addaleax · 2018-10-14T21:48:16Z

@tniessen Stack pages should only be initialized once, when the stack exceeds a certain threshold, and there should be no other runtime allocation costs or extra memory usage – so, essentially cost-free.

tniessen · 2018-10-14T22:05:33Z

Stack pages should only be initialized once, when the stack exceeds a certain threshold, and there should be no other runtime allocation costs or extra memory usage – so, essentially cost-free.

As far as I know, that's true for most if not all kernels, but ultimately, it is an implementation detail. I agree that this PR is unlikely to change the number of allocated pages in practice, but the commit message only says that this reduces the memory usage of SignFinal, not of the whole process, and I think that's correct.

refack

Some changes

refack · 2018-10-14T22:53:16Z

src/node_crypto.cc

 }

 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,


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

refack · 2018-10-14T22:54:13Z

src/node_crypto.cc

-    *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);


why are you constructing a new MallocedBuffer?

refack · 2018-10-14T23:04:42Z

src/node_crypto.cc

@@ -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;


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.

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

If we can do this on the stack it's free.

If we use a vector with move semantics, there's no allocation until we resize.

As you mentioned, this change's main benefit is readability.

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

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

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

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

C++CG:

ES.20: Always initialize an object

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

ES.1: Prefer the standard library to other libraries and to “handcrafted code”

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)

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

refack · 2018-10-17T18:21:06Z

src/node_crypto.cc

+    return sign->CheckThrow(std::get<Error>(ret));
+
+  MallocedBuffer<unsigned char> sig =
+      std::get<MallocedBuffer<unsigned char>>(std::move(ret));


I think you need to move the buffer not the ret:

MallocedBuffer<unsigned char> sig = std::move(std::get<MallocedBuffer<unsigned char>>(ret));

Or:

MallocedBuffer<unsigned char> sig(std::get<MallocedBuffer<unsigned char>>(ret));

Or my favorite:

const auto& sig = std::get<MallocedBuffer<unsigned char>>(ret);

refack · 2018-10-17T18:25:58Z

src/node_crypto.cc

+    const char* passphrase,
+    int padding,
+    int salt_len) {
+  MallocedBuffer<unsigned char> buffer;


Also possible:

auto ret = std::make_pair<SignBase::Error, MallocedBuffer<unsigned char>>(kSignNotInitialised, nullptr);

then later:

std::get<Error>(ret) = kSignNotInitialised;

etc;

refack · 2018-10-17T18:30:41Z

@tniessen thanks for following up. IMHO this looks much better (no uninitialized variables, and no need for out params.
I left a few suggestions, but they are pseudocode...

refack · 2018-10-17T20:47:06Z

src/util.h

@@ -446,7 +446,7 @@ struct MallocedBuffer {

  inline bool is_empty() const { return data == nullptr; }

-  MallocedBuffer() : data(nullptr) {}
+  MallocedBuffer() : data(nullptr), size(0) {}


👍 I had a patch floating around I wanted to submit with this.

tniessen · 2018-10-17T21:01:41Z

Thanks @refack. CI: https://ci.nodejs.org/job/node-test-pull-request/17950/
Resumed in https://ci.nodejs.org/job/node-test-pull-request/17989/

tniessen · 2018-10-19T22:39:46Z

Landed in 7bd2912, thank you for reviewing.

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. PR-URL: #23427 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Using the non-indexed variant of `std::get<>` broke Travis CI. Also, this allows us to be a bit more concise when returning from `SignFinal()` due to some error condition. Refs: nodejs#23427

Using the non-indexed variant of `std::get<>` broke Travis CI. Also, this allows us to be a bit more concise when returning from `SignFinal()` due to some error condition. Refs: #23427 PR-URL: #23779 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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. PR-URL: #23427 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Using the non-indexed variant of `std::get<>` broke Travis CI. Also, this allows us to be a bit more concise when returning from `SignFinal()` due to some error condition. Refs: #23427 PR-URL: #23779 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

MylesBorins · 2018-11-26T16:46:30Z

Should this land on v10.x? It lands cleanly but seems like perhaps it should be in a release a bit longer first. Thoughts?

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. PR-URL: #23427 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Using the non-indexed variant of `std::get<>` broke Travis CI. Also, this allows us to be a bit more concise when returning from `SignFinal()` due to some error condition. Refs: #23427 PR-URL: #23779 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Oct 11, 2018

sam-github reviewed Oct 11, 2018

View reviewed changes

bnoordhuis reviewed Oct 11, 2018

View reviewed changes

crypto: reduce memory usage of SignFinal

4b4b71b

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.

tniessen force-pushed the crypto-reduce-sign-mem branch from bff5fd8 to 4b4b71b Compare October 12, 2018 21:29

This comment has been minimized.

Sign in to view

addaleax reviewed Oct 12, 2018

View reviewed changes

fixup! crypto: reduce memory usage of SignFinal

e5bd9d0

addaleax approved these changes Oct 14, 2018

View reviewed changes

refack suggested changes Oct 14, 2018

View reviewed changes

refack reviewed Oct 14, 2018

View reviewed changes

fixup! crypto: reduce memory usage of SignFinal

202d506

refack reviewed Oct 17, 2018

View reviewed changes

refack approved these changes Oct 17, 2018

View reviewed changes

fixup! crypto: reduce memory usage of SignFinal

27f5947

refack reviewed Oct 17, 2018

View reviewed changes

refack approved these changes Oct 17, 2018

View reviewed changes

tniessen closed this Oct 19, 2018

addaleax mentioned this pull request Oct 20, 2018

src: use more explicit return type in Sign::SignFinal() #23779

Closed

2 tasks

MylesBorins added the lts-watch-v10.x label Nov 26, 2018

codebytere added backported-to-v10.x and removed lts-watch-v10.x labels Dec 13, 2018

codebytere mentioned this pull request Jan 4, 2019

v10.15.1 proposal #25346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: reduce memory usage of SignFinal #23427

crypto: reduce memory usage of SignFinal #23427

tniessen commented Oct 11, 2018

nodejs-github-bot commented Oct 11, 2018

sam-github Oct 11, 2018

sam-github Oct 11, 2018

sam-github Oct 11, 2018

bnoordhuis Oct 11, 2018

bnoordhuis Oct 11, 2018

This comment has been minimized.

addaleax Oct 12, 2018

tniessen Oct 12, 2018

refack Oct 15, 2018

addaleax commented Oct 14, 2018

Guillerbr commented Oct 14, 2018

tniessen commented Oct 14, 2018

addaleax commented Oct 14, 2018

tniessen commented Oct 14, 2018

refack left a comment

refack Oct 14, 2018 •

edited

Loading

refack Oct 14, 2018

refack Oct 14, 2018

addaleax Oct 14, 2018

refack Oct 14, 2018 •

edited

Loading

addaleax Oct 14, 2018

refack Oct 15, 2018

refack Oct 17, 2018

refack Oct 17, 2018

refack commented Oct 17, 2018

refack Oct 17, 2018

tniessen commented Oct 17, 2018 •

edited

Loading

tniessen commented Oct 19, 2018

MylesBorins commented Nov 26, 2018

crypto: reduce memory usage of SignFinal #23427

crypto: reduce memory usage of SignFinal #23427

Conversation

tniessen commented Oct 11, 2018

Checklist

nodejs-github-bot commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Oct 14, 2018

Guillerbr commented Oct 14, 2018

tniessen commented Oct 14, 2018

addaleax commented Oct 14, 2018

tniessen commented Oct 14, 2018

refack left a comment

Choose a reason for hiding this comment

refack Oct 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Oct 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Oct 17, 2018

Choose a reason for hiding this comment

tniessen commented Oct 17, 2018 • edited Loading

tniessen commented Oct 19, 2018

MylesBorins commented Nov 26, 2018

refack Oct 14, 2018 •

edited

Loading

refack Oct 14, 2018 •

edited

Loading

tniessen commented Oct 17, 2018 •

edited

Loading