certificate: fix memory leak
When the certificate test cases were ran under ASAN, there was reported
to be a large memory leak in the certificate code.
The understanding documented previously in the code with respect to the
relationship between `X509_STORE` and `X509_LOOKUP` did not match the
reality of the OpenSSL source. `LOOKUP` is part-of the `STORE`, not an
owner-of it. It is not appropriate to `X509_LOOKUP_free` the `LOOKUP`
that has become part of the `STORE`, because it is the responsibility of
the `STORE` to do that.
Invert the relationship (and holding std::unique_ptrs) so that the
`X509_STORE` becomes the RAII object and everything contained in it can
be freed when it goes out of scope.
Further explanation of the OpenSSL source is as follows:
* The `X509_LOOKUP_free` only releases the memory held by itself and
calls `method->free`[1]. The `X509_LOOKUP_file` type has no
`method->free`[2] (and confirmed with GDB). This means that the
`X509_LOOKUP_free` does not end up freeing much of any memory and
causes a leak of everything it put into the `X509_STORE`, so
`X509_LOOKUP_free` does not belong as the RAII cleanup function.
* The `X509_STORE_add_lookup` allocates the `X509_LOOKUP`, assigns
itself to the `LOOKUP` as the store context, and adds the `LOOKUP`
onto a stack of held `LOOKUP` objects[3]. When `X509_STORE_free` is
called, all of the held `LOOKUP` objects are freed[4] by calling
`X509_LOOKUP_free`.
Therefore, assigning `X509_STORE_free` as the RAII cleanup function
allows both the `X509_STORE` (and all certificate data inserted into it)
as well as the created `X509_LOOKUP` to be freed.
Tested: ASAN now passes, along with testcases, when ran.
1. https://github.com/openssl/openssl/blob/master/crypto/x509/x509_lu.c#L39
2. https://github.com/openssl/openssl/blob/master/crypto/x509/by_file.c#L30
3. https://github.com/openssl/openssl/blob/master/crypto/x509/x509_lu.c#L285
4. https://github.com/openssl/openssl/blob/master/crypto/x509/x509_lu.c#L238
Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
Change-Id: Ie350c47a2c01c5a47ed538d01e1f71274ece1fc8
diff --git a/certificate.cpp b/certificate.cpp
index 45de407..7872d81 100644
--- a/certificate.cpp
+++ b/certificate.cpp
@@ -23,10 +23,10 @@
{
// RAII support for openSSL functions.
using BIO_MEM_Ptr = std::unique_ptr<BIO, decltype(&::BIO_free)>;
+using X509_STORE_Ptr =
+ std::unique_ptr<X509_STORE, decltype(&::X509_STORE_free)>;
using X509_STORE_CTX_Ptr =
std::unique_ptr<X509_STORE_CTX, decltype(&::X509_STORE_CTX_free)>;
-using X509_LOOKUP_Ptr =
- std::unique_ptr<X509_LOOKUP, decltype(&::X509_LOOKUP_free)>;
using ASN1_TIME_ptr = std::unique_ptr<ASN1_TIME, decltype(&ASN1_STRING_free)>;
using EVP_PKEY_Ptr = std::unique_ptr<EVP_PKEY, decltype(&::EVP_PKEY_free)>;
using BUF_MEM_Ptr = std::unique_ptr<BUF_MEM, decltype(&::BUF_MEM_free)>;
@@ -253,10 +253,8 @@
elog<InternalFailure>();
}
- // Defining store object as RAW to avoid double free.
- // X509_LOOKUP_free free up store object.
// Create an empty X509_STORE structure for certificate validation.
- auto x509Store = X509_STORE_new();
+ X509_STORE_Ptr x509Store(X509_STORE_new(), &X509_STORE_free);
if (!x509Store)
{
log<level::ERR>("Error occured during X509_STORE_new call");
@@ -266,18 +264,16 @@
OpenSSL_add_all_algorithms();
// ADD Certificate Lookup method.
- X509_LOOKUP_Ptr lookup(X509_STORE_add_lookup(x509Store, X509_LOOKUP_file()),
- ::X509_LOOKUP_free);
+ // lookup will be cleaned up automatically when the holding Store goes away.
+ auto lookup = X509_STORE_add_lookup(x509Store.get(), X509_LOOKUP_file());
+
if (!lookup)
{
- // Normally lookup cleanup function interanlly does X509Store cleanup
- // Free up the X509Store.
- X509_STORE_free(x509Store);
log<level::ERR>("Error occured during X509_STORE_add_lookup call");
elog<InternalFailure>();
}
// Load Certificate file.
- errCode = X509_LOOKUP_load_file(lookup.get(), certSrcFilePath.c_str(),
+ errCode = X509_LOOKUP_load_file(lookup, certSrcFilePath.c_str(),
X509_FILETYPE_PEM);
if (errCode != 1)
{
@@ -296,7 +292,8 @@
elog<InternalFailure>();
}
- errCode = X509_STORE_CTX_init(storeCtx.get(), x509Store, cert.get(), NULL);
+ errCode =
+ X509_STORE_CTX_init(storeCtx.get(), x509Store.get(), cert.get(), NULL);
if (errCode != 1)
{
log<level::ERR>("Error occured during X509_STORE_CTX_init call",