search

ponylang / crypto

Library of common cryptographic algorithms and functions for Pony
https://ponylang.io
BSD 2-Clause "Simplified" License
11 stars 8 forks source link

Mismatch of context cleanup in Digest.final() #54

Closed ergl closed 3 years ago

ergl commented 3 years ago

In Digest.final(), after finishing with the context, it is cleaned up:

https://github.com/ponylang/crypto/blob/e58e4ab5ffe7adeb9e25ead3544fb666d51418fc/crypto/digest.pony#L157-L161

Depending on the SSL version, it will either use *_free or *_cleanup, however, those two functions are not equivalent (from the docs):

EVP_MD_CTX_create(), EVP_MD_CTX_cleanup(), and EVP_MD_CTX_destroy() are deprecated aliases for EVP_MD_CTX_new(), EVP_MD_CTX_reset(), and EVP_MD_CTX_free(), respectively.

This means that on OpenSSL 1.1.x, the code is freeing _ctx, while on OpenSSL 0.9.0 it is merely resetting it to a state where it can be reused again.

Currently the Digest class can't be reused (calling append after calling final raises an error). Either of the two approaches could work here, but I guess we should use the same behaviour for cleaning up in both versions of OpenSSL.

ergl commented 3 years ago

On second thought, I guess there's a potential memory leak on 0.9.0 since the context won't be freed even if the Digest class itself is GC'd

SeanTAllen commented 3 years ago

@ergl let me know if you disagree with any of the labels I added to this issue.

ergl commented 3 years ago

No, it's fine. I'll think about the issue and submit a patch after I'm done with the FFI PR.