Cleanup libcrypto memory management. Remove redundant NULL checks

before calling *_free() functions.  Use 'get0' functions where it
makes sense to avoid some frees.

Feedback and ok tb@

diff --git a/sbin/iked/ca.c b/sbin/iked/ca.c
index a2b4190..b062471 100644 (file)
--- a/sbin/iked/ca.c
+++ b/sbin/iked/ca.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: ca.c,v 1.83 2021/12/08 19:17:35 tobhe Exp $   */
+/*     $OpenBSD: ca.c,v 1.84 2021/12/13 17:35:34 tobhe Exp $   */
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -187,10 +187,8 @@ ca_reset(struct privsep *ps)
            store->ca_pubkey.id_type == IKEV2_ID_NONE)
                fatalx("ca_reset: keys not loaded");
 
-       if (store->ca_cas != NULL)
-               X509_STORE_free(store->ca_cas);
-       if (store->ca_certs != NULL)
-               X509_STORE_free(store->ca_certs);
+       X509_STORE_free(store->ca_cas);
+       X509_STORE_free(store->ca_certs);
 
        if ((store->ca_cas = X509_STORE_new()) == NULL)
                fatalx("ca_reset: failed to get ca store");
@@ -483,9 +481,8 @@ ca_getcert(struct iked *env, struct imsg *imsg)
                cert = ca_by_subjectaltname(store->ca_certs, &id);
                if (cert) {
                        log_debug("%s: found local cert", __func__);
-                       if ((certkey = X509_get_pubkey(cert)) != NULL) {
+                       if ((certkey = X509_get0_pubkey(cert)) != NULL) {
                                ret = ca_pubkey_serialize(certkey, &key);
-                               EVP_PKEY_free(certkey);
                                if (ret == 0) {
                                        ptr = ibuf_data(key.id_buf);
                                        len = ibuf_length(key.id_buf);
@@ -1045,7 +1042,7 @@ ca_cert_local(struct iked *env, X509  *cert)
        if ((localpub = ca_id_to_pkey(&store->ca_pubkey)) == NULL)
                goto done;
 
-       if ((certkey = X509_get_pubkey(cert)) == NULL) {
+       if ((certkey = X509_get0_pubkey(cert)) == NULL) {
                log_info("%s: no public key in cert", __func__);
                goto done;
        }
@@ -1057,10 +1054,8 @@ ca_cert_local(struct iked *env, X509  *cert)
 
        ret = 1;
  done:
-       if (certkey != NULL)
-               EVP_PKEY_free(certkey);
-       if (localpub != NULL)
-               EVP_PKEY_free(localpub);
+       EVP_PKEY_free(localpub);
+
        return (ret);
 }
 
@@ -1092,8 +1087,7 @@ ca_cert_info(const char *msg, X509 *cert)
                log_info("%s: subject: %s", msg, buf);
        ca_x509_subjectaltname_log(cert, msg);
 out:
-       if (rawserial)
-               BIO_free(rawserial);
+       BIO_free(rawserial);
 }
 
 int
@@ -1112,10 +1106,9 @@ ca_subjectpubkey_digest(X509 *x509, uint8_t *md, unsigned int *size)
         * that includes the public key type (eg. RSA) and the
         * public key value (see 3.7 of RFC7296).
         */
-       if ((pkey = X509_get_pubkey(x509)) == NULL)
+       if ((pkey = X509_get0_pubkey(x509)) == NULL)
                return (-1);
        buflen = i2d_PUBKEY(pkey, &buf);
-       EVP_PKEY_free(pkey);
        if (buflen == 0)
                return (-1);
        if (!EVP_Digest(buf, buflen, md, size, EVP_sha1(), NULL)) {
@@ -1196,7 +1189,7 @@ ca_pubkey_serialize(EVP_PKEY *key, struct iked_id *id)
                ibuf_release(id->id_buf);
                id->id_buf = NULL;
 
-               if ((rsa = EVP_PKEY_get1_RSA(key)) == NULL)
+               if ((rsa = EVP_PKEY_get0_RSA(key)) == NULL)
                        goto done;
                if ((len = i2d_RSAPublicKey(rsa, NULL)) <= 0)
                        goto done;
@@ -1218,7 +1211,7 @@ ca_pubkey_serialize(EVP_PKEY *key, struct iked_id *id)
                ibuf_release(id->id_buf);
                id->id_buf = NULL;
 
-               if ((ec = EVP_PKEY_get1_EC_KEY(key)) == NULL)
+               if ((ec = EVP_PKEY_get0_EC_KEY(key)) == NULL)
                        goto done;
                if ((len = i2d_EC_PUBKEY(ec, NULL)) <= 0)
                        goto done;
@@ -1245,10 +1238,7 @@ ca_pubkey_serialize(EVP_PKEY *key, struct iked_id *id)
 
        ret = 0;
  done:
-       if (rsa != NULL)
-               RSA_free(rsa);
-       if (ec != NULL)
-               EC_KEY_free(ec);
+
        return (ret);
 }
 
@@ -1268,7 +1258,7 @@ ca_privkey_serialize(EVP_PKEY *key, struct iked_id *id)
                ibuf_release(id->id_buf);
                id->id_buf = NULL;
 
-               if ((rsa = EVP_PKEY_get1_RSA(key)) == NULL)
+               if ((rsa = EVP_PKEY_get0_RSA(key)) == NULL)
                        goto done;
                if ((len = i2d_RSAPrivateKey(rsa, NULL)) <= 0)
                        goto done;
@@ -1290,7 +1280,7 @@ ca_privkey_serialize(EVP_PKEY *key, struct iked_id *id)
                ibuf_release(id->id_buf);
                id->id_buf = NULL;
 
-               if ((ec = EVP_PKEY_get1_EC_KEY(key)) == NULL)
+               if ((ec = EVP_PKEY_get0_EC_KEY(key)) == NULL)
                        goto done;
                if ((len = i2d_ECPrivateKey(ec, NULL)) <= 0)
                        goto done;
@@ -1317,10 +1307,7 @@ ca_privkey_serialize(EVP_PKEY *key, struct iked_id *id)
 
        ret = 0;
  done:
-       if (rsa != NULL)
-               RSA_free(rsa);
-       if (ec != NULL)
-               EC_KEY_free(ec);
+
        return (ret);
 }
 
@@ -1354,14 +1341,11 @@ ca_id_to_pkey(struct iked_id *pubkey)
        out = localkey;
        localkey = NULL;
  done:
-       if (localkey != NULL)
-               EVP_PKEY_free(localkey);
-       if (localrsa != NULL)
-               RSA_free(localrsa);
-       if (localec != NULL)
-               EC_KEY_free(localec);
-       if (rawcert != NULL)
-               BIO_free(rawcert);
+       EVP_PKEY_free(localkey);
+       RSA_free(localrsa);
+       EC_KEY_free(localec);
+       BIO_free(rawcert);
+
        return (out);
 }
 
@@ -1403,11 +1387,8 @@ ca_privkey_to_method(struct iked_id *privkey)
            print_map(method, ikev2_auth_map));
 
  out:
-       if (ec != NULL)
-               EC_KEY_free(ec);
-       if (rawcert != NULL)
-               BIO_free(rawcert);
-
+       EC_KEY_free(ec);
+       BIO_free(rawcert);
        return (method);
 }
 
@@ -1630,19 +1611,13 @@ ca_validate_pubkey(struct iked *env, struct iked_static_id *id,
                ca_sslerror(__func__);
  done:
        ibuf_release(idp.id_buf);
-       if (localkey != NULL)
-               EVP_PKEY_free(localkey);
-       if (peerrsa != NULL)
-               RSA_free(peerrsa);
-       if (peerec != NULL)
-               EC_KEY_free(peerec);
-       if (localrsa != NULL)
-               RSA_free(localrsa);
-       if (rawcert != NULL) {
-               BIO_free(rawcert);
-               if (peerkey != NULL)
-                       EVP_PKEY_free(peerkey);
-       }
+       EVP_PKEY_free(localkey);
+       RSA_free(peerrsa);
+       EC_KEY_free(peerec);
+       RSA_free(localrsa);
+       BIO_free(rawcert);
+       if (len > 0)
+               EVP_PKEY_free(peerkey);
 
        return (ret);
 }
@@ -1682,12 +1657,11 @@ ca_validate_cert(struct iked *env, struct iked_static_id *id,
        }
 
        if (id != NULL) {
-               if ((pkey = X509_get_pubkey(cert)) == NULL) {
+               if ((pkey = X509_get0_pubkey(cert)) == NULL) {
                        errstr = "no public key in cert";
                        goto done;
                }
                ret = ca_validate_pubkey(env, id, pkey, 0, NULL);
-               EVP_PKEY_free(pkey);
                if (ret == 0) {
                        errstr = "in public key file, ok";
                        goto done;
@@ -1759,14 +1733,10 @@ ca_validate_cert(struct iked *env, struct iked_static_id *id,
        }
  err:
 
-       if (rawcert != NULL) {
-               BIO_free(rawcert);
-               if (cert != NULL)
-                       X509_free(cert);
-       }
-
-       if (csc != NULL)
-               X509_STORE_CTX_free(csc);
+       if (len > 0)
+               X509_free(cert);
+       BIO_free(rawcert);
+       X509_STORE_CTX_free(csc);
 
        return (ret);
 }

diff --git a/sbin/iked/crypto.c b/sbin/iked/crypto.c
index 59cac67..87fb765 100644 (file)
--- a/sbin/iked/crypto.c
+++ b/sbin/iked/crypto.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: crypto.c,v 1.38 2021/12/01 16:42:12 deraadt Exp $     */
+/*     $OpenBSD: crypto.c,v 1.39 2021/12/13 17:35:34 tobhe Exp $       */
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -319,8 +319,7 @@ hash_free(struct iked_hash *hash)
 {
        if (hash == NULL)
                return;
-       if (hash->hash_ctx != NULL)
-               HMAC_CTX_free(hash->hash_ctx);
+       HMAC_CTX_free(hash->hash_ctx);
        ibuf_release(hash->hash_key);
        free(hash);
 }
@@ -764,9 +763,8 @@ dsa_free(struct iked_dsa *dsa)
        if (dsa->dsa_hmac) {
                HMAC_CTX_free((HMAC_CTX *)dsa->dsa_ctx);
        } else {
-               EVP_MD_CTX_destroy((EVP_MD_CTX *)dsa->dsa_ctx);
-               if (dsa->dsa_key)
-                       EVP_PKEY_free(dsa->dsa_key);
+               EVP_MD_CTX_free((EVP_MD_CTX *)dsa->dsa_ctx);
+               EVP_PKEY_free(dsa->dsa_key);
        }
 
        ibuf_release(dsa->dsa_keydata);
@@ -842,8 +840,7 @@ dsa_setkey(struct iked_dsa *dsa, void *key, size_t keylen, uint8_t type)
                goto err;
        }
 
-       if (cert != NULL)
-               X509_free(cert);
+       X509_free(cert);
        BIO_free(rawcert);      /* temporary for parsing */
 
        return (dsa->dsa_keydata);
@@ -853,16 +850,11 @@ dsa_setkey(struct iked_dsa *dsa, void *key, size_t keylen, uint8_t type)
  err:
        log_debug("%s: error", __func__);
 
-       if (rsa != NULL)
-               RSA_free(rsa);
-       if (ec != NULL)
-               EC_KEY_free(ec);
-       if (pkey != NULL)
-               EVP_PKEY_free(pkey);
-       if (cert != NULL)
-               X509_free(cert);
-       if (rawcert != NULL)
-               BIO_free(rawcert);
+       RSA_free(rsa);
+       EC_KEY_free(ec);
+       EVP_PKEY_free(pkey);
+       X509_free(cert);
+       BIO_free(rawcert);
        ibuf_release(dsa->dsa_keydata);
        dsa->dsa_keydata = NULL;
        return (NULL);
@@ -1078,8 +1070,8 @@ _dsa_sign_ecdsa(struct iked_dsa *dsa, uint8_t *ptr, size_t len)
        ret = 0;
  done:
        free(tmp);
-       if (obj)
-               ECDSA_SIG_free(obj);
+       ECDSA_SIG_free(obj);
+
        return (ret);
 }
 
@@ -1180,8 +1172,8 @@ _dsa_verify_prepare(struct iked_dsa *dsa, uint8_t **sigp, size_t *lenp,
        BN_clear_free(r);
        BN_clear_free(s);
        free(ptr);
-       if (obj)
-               ECDSA_SIG_free(obj);
+       ECDSA_SIG_free(obj);
+
        return (ret);
 }
 

diff --git a/sbin/iked/ocsp.c b/sbin/iked/ocsp.c
index 06d77af..0b05a62 100644 (file)
--- a/sbin/iked/ocsp.c
+++ b/sbin/iked/ocsp.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: ocsp.c,v 1.22 2021/11/19 21:16:25 tobhe Exp $ */
+/*     $OpenBSD: ocsp.c,v 1.23 2021/12/13 17:35:34 tobhe Exp $ */
 
 /*
  * Copyright (c) 2014 Markus Friedl
@@ -322,20 +322,16 @@ ocsp_validate_cert(struct iked *env, void *data, size_t len,
        ret = proc_composev(&env->sc_ps, PROC_PARENT, IMSG_OCSP_FD,
            iov, iovcnt);
 
-       if (aia)
-               X509_email_free(aia);   /* free stack of openssl strings */
+       X509_email_free(aia);   /* free stack of openssl strings */
 
        return (ret);
 
  err:
        ca_sslerror(__func__);
        free(ioe);
-       if (rawcert != NULL)
-               BIO_free(rawcert);
-       if (cert != NULL)
-               X509_free(cert);
-       if (id != NULL)
-               OCSP_CERTID_free(id);
+       BIO_free(rawcert);
+       X509_free(cert);
+       OCSP_CERTID_free(id);
        ocsp_validate_finish(ocsp, 0);  /* failed */
        return (-1);
 }
@@ -349,15 +345,9 @@ ocsp_free(struct iked_ocsp *ocsp)
                        close(ocsp->ocsp_sock->sock_fd);
                        free(ocsp->ocsp_sock);
                }
-               if (ocsp->ocsp_cbio != NULL)
-                       BIO_free_all(ocsp->ocsp_cbio);
-
-               if (ocsp->ocsp_req_ctx != NULL)
-                       OCSP_REQ_CTX_free(ocsp->ocsp_req_ctx);
-
-               if (ocsp->ocsp_req != NULL)
-                       OCSP_REQUEST_free(ocsp->ocsp_req);
-
+               BIO_free_all(ocsp->ocsp_cbio);
+               OCSP_REQ_CTX_free(ocsp->ocsp_req_ctx);
+               OCSP_REQUEST_free(ocsp->ocsp_req);
                free(ocsp);
        }
 }
@@ -471,12 +461,10 @@ ocsp_load_certs(const char *file)
        }
 
  done:
-       if (bio)
-               BIO_free(bio);
-       if (xis)
-               sk_X509_INFO_pop_free(xis, X509_INFO_free);
-       if (certs && sk_X509_num(certs) <= 0) {
-               sk_X509_pop_free(certs, X509_free);
+       BIO_free(bio);
+       sk_X509_INFO_pop_free(xis, X509_INFO_free);
+       if (sk_X509_num(certs) <= 0) {
+               sk_X509_free(certs);
                certs = NULL;
        }
        return (certs);
@@ -599,14 +587,10 @@ ocsp_parse_response(struct iked_ocsp *ocsp, OCSP_RESPONSE *resp)
        if (!valid) {
                log_debug("%s: status: %s", __func__, errstr);
        }
-       if (store)
-               X509_STORE_free(store);
-       if (verify_other)
-               sk_X509_pop_free(verify_other, X509_free);
-       if (resp)
-               OCSP_RESPONSE_free(resp);
-       if (bs)
-               OCSP_BASICRESP_free(bs);
+       X509_STORE_free(store);
+       sk_X509_pop_free(verify_other, X509_free);
+       OCSP_RESPONSE_free(resp);
+       OCSP_BASICRESP_free(bs);
 
        ocsp_validate_finish(ocsp, valid);
 }