OpenSSL Stack API - Freeing objects after pushing to the stack

Question

I'm trying to understand if i need to free X509 objects after pushing them into a STACK_OF(X509) structure, or does the sk_X509_free() call frees everything for me including the content. I found no documentation for this in OpenSSL.

std::vector<std::string> caPems;    

// Fill the vector from input
// ...


BIO *bufio = NULL;
X509 *x509 = NULL, *x509_ca = NULL;
bool success = false;
STACK_OF(X509)* x509_ca_stack;

x509_ca_stack = sk_X509_new_null();
if (x509_ca_stack) {
    success = true;
    for (const std::string& caPem : caPems) {
        BIO_new_mem_buf(caPem.c_str(), caPem.size());
        PEM_read_bio_X509(bufio, &x509_ca, NULL, NULL);
        BIO_free_all(bufio);
        if (x509_ca != nullptr) {
            sk_X509_push(x509_ca_stack, x509_ca);
            x509_ca = NULL; // should I free after push???
        } else
            success = false;
    }
    if (success)
        foo(x509_ca_stack);
    sk_X509_free(x509_ca_stack); // or is this free enough for the entire stack?
} else {
    printf("ERROR: failed loading cert\n");
}

Edit: valgrind was no help, it showed nothing both when I freed and when I did not.

Answer 1

Since openssl 1.1 X509 (and many others) are reference counted. The sk_XXX_push() API does NOT automatically increment reference so you are handing over your X509 reference when doing the push. Therefore your code is NOT valid and does leak, as-is, since sk_X509_free() will NOT decrease reference count of the internal X509.

edit: I corrected the above, I originally thought sk_X509_free() would automatically free up the ref. It does not. From openssl documentation https://www.openssl.org/docs/man1.1.0/man3/DEFINE_STACK_OF.html

sk_TYPE_free() frees up the sk structure. It does not free up any elements of sk. After this call sk is no longer valid.

If one really wants for some reason to create a deep copy of the X509, one can use X509_dup() to duplicate the X509 object and push that into the stack:

sk_X509_push(stack, X509_dup(cert));

In this case, both X509 objects need to be freed explicitly.

One can also use sk_X509_pop_free(stack, X509_free) to automatically unref all stack elements and delete the stack itself.

In conclusion your code can be fixed in 2 ways:

• add X509_free() at the end, not right after sk_X509_push()
• replace sk_X509_free() with sk_X509_pop_free()