Home Original page

crypto: fix memory leak, reduce heap allocations, clean up by bnoordhuis · Pull Request #14122 · nodejs/node

@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

Jul 7, 2017

addaleax

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

@bnoordhuis 

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.

@bnoordhuis 

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.

@bnoordhuis

@bnoordhuis

@bnoordhuis

@bnoordhuis 

This also renames a misnamed variable `error_` to `success_`.

@bnoordhuis

@bnoordhuis 

Fix a memory leak in dh.setPublicKey() and dh.setPrivateKey() where the
old keys weren't freed.

Fixes: nodejs#8377

addaleax pushed a commit that referenced this pull request

Jul 18, 2017 
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 2017 
The cipher kind doesn't change over the lifetime of the cipher so make
it const.

PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 2017 
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 2017 
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 2017 
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 2017 
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 2017 
This also renames a misnamed variable `error_` to `success_`.

PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 2017 
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax pushed a commit that referenced this pull request

Jul 18, 2017 
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 pushed a commit that referenced this pull request

Jul 19, 2017 
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 pushed a commit that referenced this pull request

Jul 19, 2017 
PR-URL: #14122
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

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 <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>