crypto: fix memory leak, reduce heap allocations, clean up#14122
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
nodejs-github-bot added c++ 
addaleax approved these changes Jul 7, 2017
MemberAuthor
I added a commit that fixes the DH memory leak reported in #8377. |
jasnell approved these changes Jul 10, 2017
Member
jasnell left a comment
jasnell left a comment There was a problem hiding this comment.
Largely rubber-stamp LGTM. Looked through the code and didn't see anything that stood out but didn't do a thorough review.
MemberAuthor
bnoordhuis commented Jul 12, 2017
Bumped DH key size in test to 1024 to pacify FIPS. New CI: https://ci.nodejs.org/job/node-test-pull-request/9094/ |
Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.Cipher#final() - crypto.Cipher#update() - crypto.Cipheriv#final() - crypto.Cipheriv#update() - crypto.Decipher#final() - crypto.Decipher#update() - crypto.Decipheriv#final() - crypto.Decipheriv#update() - crypto.privateDecrypt() - crypto.privateEncrypt() - crypto.publicDecrypt() - crypto.publicEncrypt()
Put the 8 kB initial buffer on the stack first and don't copy it to the heap until its exact size is known (which is normally much smaller.)
Fix a memory leak by removing the heap allocation altogether. Fixes: nodejs#13917
The EVP_CIPHER can be reconstructed from the EVP_CIPHER_CTX instance, no need to store it separately. This brought to light the somewhat dubious practice of accessing the EVP_CIPHER after the EVP_CIPHER_CTX instance had been destroyed. It's mostly harmless due to the static nature of built-in EVP_CIPHER instances but it segfaults when the cipher is provided by an ENGINE and the ENGINE is unloaded because its reference count drops to zero.
The cipher kind doesn't change over the lifetime of the cipher so make it const.
Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.CryptoStream#getSession() - tls.TLSSocket#getSession()
Add a test that ensures the second call to .digest() returns an empty HMAC, like it did before. No comment on whether that is the right behavior or not.
Replace allocate + Encode() + free patterns by calls to Malloc + the Buffer::New() overload that takes ownership of the pointer. Avoids unnecessary heap allocations and copying around of data. DRY the accessor functions for the prime, generator, public key and private key properties; deletes about 40 lines of quadruplicated code.
This also renames a misnamed variable `error_` to `success_`.
Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the old keys weren't freed. Fixes: nodejs#8377
MemberAuthor
bnoordhuis commented Jul 17, 2017
One more rebase + CI: https://ci.nodejs.org/job/node-test-pull-request/9192/ |
MemberAuthor
bnoordhuis commented Jul 17, 2017
Stress test to see if the async-hooks/test-tlswrap failure on the osx1010 bot is a flake or not: https://ci.nodejs.org/job/node-stress-single-test/1319/ |
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the old keys weren't freed. Fixes: #8377 PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.Cipher#final() - crypto.Cipher#update() - crypto.Cipheriv#final() - crypto.Cipheriv#update() - crypto.Decipher#final() - crypto.Decipher#update() - crypto.Decipheriv#final() - crypto.Decipheriv#update() - crypto.privateDecrypt() - crypto.privateEncrypt() - crypto.publicDecrypt() - crypto.publicEncrypt() PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Put the 8 kB initial buffer on the stack first and don't copy it to the heap until its exact size is known (which is normally much smaller.) PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Fix a memory leak by removing the heap allocation altogether. Fixes: #13917 PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
The EVP_CIPHER can be reconstructed from the EVP_CIPHER_CTX instance, no need to store it separately. This brought to light the somewhat dubious practice of accessing the EVP_CIPHER after the EVP_CIPHER_CTX instance had been destroyed. It's mostly harmless due to the static nature of built-in EVP_CIPHER instances but it segfaults when the cipher is provided by an ENGINE and the ENGINE is unloaded because its reference count drops to zero. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
The cipher kind doesn't change over the lifetime of the cipher so make it const. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Don't allocate + copy + free; allocate and fill in place, then hand off the pointer to Buffer::New(). Avoids unnecessary heap allocations in the following methods: - crypto.CryptoStream#getSession() - tls.TLSSocket#getSession() PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Add a test that ensures the second call to .digest() returns an empty HMAC, like it did before. No comment on whether that is the right behavior or not. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Replace allocate + Encode() + free patterns by calls to Malloc + the Buffer::New() overload that takes ownership of the pointer. Avoids unnecessary heap allocations and copying around of data. DRY the accessor functions for the prime, generator, public key and private key properties; deletes about 40 lines of quadruplicated code. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
This also renames a misnamed variable `error_` to `success_`. PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the old keys weren't freed. Fixes: #8377 PR-URL: #14122 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Contributor
MylesBorins commented Aug 16, 2017
@bnoordhuis is this something we would want to backport to v6.x? |
MemberAuthor
bnoordhuis commented Aug 16, 2017
Yes. |
Contributor
MylesBorins commented Sep 19, 2017
@bnoordhuis this is going to require a manual backport. Would you be willing to help with this? We could choose to simply not land the commits that have conflicts, up for suggestion on best path forward |
Contributor
MylesBorins commented Oct 16, 2017
ping @bnoordhuis |
MemberAuthor
bnoordhuis commented Oct 29, 2017
Depends on #16587, the new internal APIs need to be back-ported first. |
Contributor
MylesBorins commented Nov 14, 2017
@bnoordhuis#16587 has landed, would you be able to backport this now? |
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #13917
Also of note:
I'll clean up the commit logs some more before landing.
CI: https://ci.nodejs.org/job/node-test-pull-request/9027/