Memory leak in openssl quic modules

[vanc]

In revision 53dc0cc, a new module was introduced to integrate with quictls (a OpenSSL fork). However, it came with a couple of memory leaks.

Leak 1: ctx of PacketKey in crypto/openssl_quictls.rs.

The ctx field was initialized with EVP_CIPHER_CTX_new. It's never released. The fix is rather trivial. Just implement the Drop trait.

 unsafe impl std::marker::Send for PacketKey {}
 unsafe impl std::marker::Sync for PacketKey {}

+impl Drop for PacketKey {
+    fn drop(&mut self) {
+        unsafe { EVP_CIPHER_CTX_free(self.ctx) }
+    }
+}
+
 extern {
     // EVP
     fn EVP_aes_128_gcm() -> *const EVP_AEAD;

Leak 2: Handshake::peer_cert in tls/openssl_quictls.rs.

The function uses i2d_X509 to convert the X509 certificate into DER format. It returns the heap allocated buffer as slice of u8. The memory is lost forever.

The design of the API was based on BoringSSL which maintains DER encoded buffers internally. It's safe to return the buffer and use it as long as the certificate remains valid. But it's not the case for OpenSSL.

A fix is not trivial as far as I can tell without changing the API.

Leak 3: Handshake::peer_cert_chain in tls/openssl_quictls.rs.

The reasoning is the same as leak 2.

[brbzull0]

Hi @vanc, Interesting, I can have a look at this if you are not doing it already, specially at the leak 2, 3. Will follow up back here.

So, you are right, not a trivial fix.

I am not an expert on Rust, so any help/direction will be appreciated here. After giving this a thought, what do you guys think about this possible(?) solutions.

1. Should we copy and return a copy of the DER as Vec<u8>, I know this changed already from this type, but worth considering maybe.
2. Add a new API to free up the allocated memory by i2d_X509. I don't really like this as it seems we only manually free up memory on some particular cases.

Thanks.

cc: @ghedo @LPardue