fix: Added deallocate functions #36
Conversation
Andrew-Lees11
force-pushed
the
deallocate
branch
from
76d80d9 to
85b0483
Compare
|
This PR also fixes issue #8 to store the EVP key instead of the RSA key. |
| defer { | ||
| RSA_free(rsaKey) | ||
| } | ||
|
|
There was a problem hiding this comment.
This additional block exists now because we are storing the envelope key rather than the RSA key.
Changing the key to store the envelope key rather than the RSA key it contains is beneficial because the sign, verify and CBC decrypt functions no longer need to construct a new one (and copy the RSA key in), which should shorten their pathlength. (caveat: we did not benchmark this)
| } | ||
|
|
||
| EVP_CIPHER_CTX_set_padding(rsaEncryptCtx, padding) | ||
|
|
||
| // Initialize the AES encryption key array (of size 1) | ||
| typealias UInt8Ptr = UnsafeMutablePointer<UInt8>? | ||
| var ek: UInt8Ptr | ||
| ek = UnsafeMutablePointer<UInt8>.allocate(capacity: Int(EVP_PKEY_size(evp_key))) | ||
| ek = UnsafeMutablePointer<UInt8>.allocate(capacity: Int(EVP_PKEY_size(.make(optional: key.reference)))) |
There was a problem hiding this comment.
Can you explain why this change is correct / necessary?
There was a problem hiding this comment.
Nevermind, I get it - key is (now) a pointer to an envelope key, so this is equivalent to what we had before
|
|
||
| // Now that Init has initialized pkey_ctx, set the padding option | ||
| EVP_PKEY_CTX_ctrl(pkey_ctx, EVP_PKEY_RSA, -1, EVP_PKEY_CTRL_RSA_PADDING, padding, nil) | ||
| EVP_DigestSignInit(md_ctx, nil, .make(optional: md), nil, .make(optional: key.reference)) |
There was a problem hiding this comment.
So, as a note to other readers, we discovered through experimentation that the attempt to set the padding algorithm (that is being removed here) 1. fails to actually alter the padding used, and 2. causes the memory allocated by other openssl routines to be leaked.
The default padding algorithm is already RSA_PKCS1_PADDING, so there's no need to set it. Ideally we would be explicit about it (in case the default changes in the future) but for now, this achieves the same as the existing code and does not leak.
|
We tested the leak by running signing,verifying encryption and decryption of a JWT. This produced the following memory report: leaks.txt which highlights leaks at: This was due to the evp_key not being properly freed after it was referenced by: (Mirrored for private key). There were also minor leaks in encrypt and decrypt where allocated memory from pointers wasn't being deallocated. The same tests now show no memory leaks with the changes from this PR. |
|
@billabt Would you be able to take a look at this and merge/tag if you are happy? |
|
Please squash and merge though since there are a fair amount of commits. |
|
I’ll take a look. |
|
@billabt The problem with not squash-merging is that the master branch history now has a bunch of intermediate commits in it. Github has a drop-down which lets you squash-merge rather than creating a merge commit (so it's trivial to do when merging). You don't lose the individual commits for the PR either - they are immortalised in the PR. |
|
Ok. I'll keep that in mind for future changes. Too late for this one. |
We had reports of a memory leak when using this repo on Linux.
I have tracked down places where we allocate memory and added deallocate functions to fix the leaks.