crypto: reduce memory usage of SignFinal by tniessen · Pull Request #23427 · nodejs/node

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

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

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

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

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

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


  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

                                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

  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

  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 <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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

Uh oh!

Conversation

tniessen commented Oct 11, 2018

Checklist

Uh oh!

nodejs-github-bot commented Oct 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 14, 2018

Uh oh!

Guillerbr commented Oct 14, 2018

Uh oh!

tniessen commented Oct 14, 2018

Uh oh!

addaleax commented Oct 14, 2018

Uh oh!

tniessen commented Oct 14, 2018

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tniessen commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Oct 19, 2018

Uh oh!

MylesBorins commented Nov 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Comments

refack Oct 14, 2018 •

edited

Loading

refack Oct 14, 2018 •

edited

Loading

tniessen commented Oct 17, 2018 •

edited

Loading