search

Kitura / BlueRSA

RSA public/private key encryption, private key signing and public key verification in Swift using the Swift Package Manager. Works on iOS, macOS, and Linux (work in progress).
Apache License 2.0
131 stars 58 forks source link

fix: Added deallocate functions #36

Closed Andrew-Lees11 closed 5 years ago

Andrew-Lees11 commented 5 years ago

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.

Andrew-Lees11 commented 5 years ago

This PR also fixes issue #8 to store the EVP key instead of the RSA key.

Andrew-Lees11 commented 5 years ago

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:

evp_key = .init(PEM_read_bio_PUBKEY(bio, nil, nil, nil))

This was due to the evp_key not being properly freed after it was referenced by:

var pkey_ctx = EVP_PKEY_CTX_new(evp_key, nil)

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

djones6 commented 5 years ago

@billabt Would you be able to take a look at this and merge/tag if you are happy?

Andrew-Lees11 commented 5 years ago

Please squash and merge though since there are a fair amount of commits.

billabt commented 5 years ago

I’ll take a look.

djones6 commented 5 years ago

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

billabt commented 5 years ago

Ok. I'll keep that in mind for future changes. Too late for this one.