From 0dfb9398bb6493d5a56216e0c7039cb3f9fc88c6 Mon Sep 17 00:00:00 2001 From: Rich Salz Date: Tue, 24 Mar 2015 07:52:24 -0400 Subject: [PATCH] free NULL cleanup Start ensuring all OpenSSL "free" routines allow NULL, and remove any if check before calling them. This gets ASN1_OBJECT_free and ASN1_STRING_free. Reviewed-by: Matt Caswell --- CHANGES | 5 +++++ apps/cms.c | 3 +-- crypto/asn1/a_bitstr.c | 2 +- crypto/asn1/a_int.c | 4 ++-- crypto/asn1/a_utctm.c | 8 ++++---- crypto/asn1/asn1_lib.c | 4 +++- crypto/asn1/asn1_par.c | 12 ++++-------- crypto/asn1/evp_asn1.c | 3 +-- crypto/asn1/p5_pbe.c | 3 +-- crypto/asn1/tasn_fre.c | 26 +++++++++++++++++--------- crypto/asn1/x_algor.c | 3 +-- crypto/asn1/x_pkey.c | 3 +-- crypto/asn1/x_x509a.c | 3 +-- crypto/dh/dh_ameth.c | 6 ++---- crypto/dh/dh_pmeth.c | 6 ++---- crypto/dsa/dsa_ameth.c | 6 ++---- crypto/ec/ec_asn1.c | 5 ++--- crypto/ocsp/ocsp_lib.c | 3 +-- crypto/ocsp/v3_ocsp.c | 2 +- crypto/rsa/rsa_ameth.c | 9 +++------ crypto/rsa/rsa_saos.c | 3 +-- crypto/x509v3/pcy_data.c | 3 +-- crypto/x509v3/v3_conf.c | 3 +-- crypto/x509v3/v3_pci.c | 5 +---- doc/crypto/ASN1_OBJECT_new.pod | 1 + doc/crypto/ASN1_STRING_new.pod | 1 + 26 files changed, 61 insertions(+), 71 deletions(-) diff --git a/CHANGES b/CHANGES index 5dd7d8d81b..ab5b48250c 100644 --- a/CHANGES +++ b/CHANGES @@ -71,6 +71,11 @@ Remove all but one '#ifdef undef' which is to be looked at. [Rich Salz] + *) Clean up calling of xxx_free routines. + Just like free(), fix most of the xxx_free routines to accept + NULL. Remove the non-null checks from callers. Save much code. + [Rich Salz] + *) Experimental support for a new, fast, unbiased prime candidate generator, bn_probable_prime_dh_coprime(). Not currently used by any prime generator. [Felix Laurie von Massenbach ] diff --git a/apps/cms.c b/apps/cms.c index d983e28de9..0877426924 100644 --- a/apps/cms.c +++ b/apps/cms.c @@ -1152,8 +1152,7 @@ int MAIN(int argc, char **argv) OPENSSL_free(secret_keyid); if (pwri_tmp) OPENSSL_free(pwri_tmp); - if (econtent_type) - ASN1_OBJECT_free(econtent_type); + ASN1_OBJECT_free(econtent_type); if (rr) CMS_ReceiptRequest_free(rr); if (rr_to) diff --git a/crypto/asn1/a_bitstr.c b/crypto/asn1/a_bitstr.c index 5a5cc23cb1..4078be4c4b 100644 --- a/crypto/asn1/a_bitstr.c +++ b/crypto/asn1/a_bitstr.c @@ -177,7 +177,7 @@ ASN1_BIT_STRING *c2i_ASN1_BIT_STRING(ASN1_BIT_STRING **a, return (ret); err: ASN1err(ASN1_F_C2I_ASN1_BIT_STRING, i); - if ((ret != NULL) && ((a == NULL) || (*a != ret))) + if ((a == NULL) || (*a != ret)) ASN1_BIT_STRING_free(ret); return (NULL); } diff --git a/crypto/asn1/a_int.c b/crypto/asn1/a_int.c index a33e3fd8ef..65fac75698 100644 --- a/crypto/asn1/a_int.c +++ b/crypto/asn1/a_int.c @@ -265,7 +265,7 @@ ASN1_INTEGER *c2i_ASN1_INTEGER(ASN1_INTEGER **a, const unsigned char **pp, return (ret); err: ASN1err(ASN1_F_C2I_ASN1_INTEGER, i); - if ((ret != NULL) && ((a == NULL) || (*a != ret))) + if ((a == NULL) || (*a != ret)) ASN1_INTEGER_free(ret); return (NULL); } @@ -334,7 +334,7 @@ ASN1_INTEGER *d2i_ASN1_UINTEGER(ASN1_INTEGER **a, const unsigned char **pp, return (ret); err: ASN1err(ASN1_F_D2I_ASN1_UINTEGER, i); - if ((ret != NULL) && ((a == NULL) || (*a != ret))) + if ((a == NULL) || (*a != ret)) ASN1_INTEGER_free(ret); return (NULL); } diff --git a/crypto/asn1/a_utctm.c b/crypto/asn1/a_utctm.c index 2dac3b58b4..0e2f1b0c40 100644 --- a/crypto/asn1/a_utctm.c +++ b/crypto/asn1/a_utctm.c @@ -193,11 +193,11 @@ ASN1_UTCTIME *ASN1_UTCTIME_adj(ASN1_UTCTIME *s, time_t t, int free_s = 0; if (s == NULL) { - free_s = 1; s = ASN1_UTCTIME_new(); + if (s == NULL) + goto err; + free_s = 1; } - if (s == NULL) - goto err; ts = OPENSSL_gmtime(&t, &data); if (ts == NULL) @@ -233,7 +233,7 @@ ASN1_UTCTIME *ASN1_UTCTIME_adj(ASN1_UTCTIME *s, time_t t, #endif return (s); err: - if (free_s && s) + if (free_s) ASN1_UTCTIME_free(s); return NULL; } diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c index fe63b6249c..2e36cff055 100644 --- a/crypto/asn1/asn1_lib.c +++ b/crypto/asn1/asn1_lib.c @@ -429,7 +429,9 @@ void ASN1_STRING_free(ASN1_STRING *a) void ASN1_STRING_clear_free(ASN1_STRING *a) { - if (a && a->data && !(a->flags & ASN1_STRING_FLAG_NDEF)) + if (a == NULL) + return; + if (a->data && !(a->flags & ASN1_STRING_FLAG_NDEF)) OPENSSL_cleanse(a->data, a->length); ASN1_STRING_free(a); } diff --git a/crypto/asn1/asn1_par.c b/crypto/asn1/asn1_par.c index 20f3a88b85..574e8de867 100644 --- a/crypto/asn1/asn1_par.c +++ b/crypto/asn1/asn1_par.c @@ -276,10 +276,8 @@ static int asn1_parse2(BIO *bp, const unsigned char **pp, long length, nl = 1; } } - if (os != NULL) { - ASN1_OCTET_STRING_free(os); - os = NULL; - } + ASN1_OCTET_STRING_free(os); + os = NULL; } else if (tag == V_ASN1_INTEGER) { ASN1_INTEGER *bs; int i; @@ -356,10 +354,8 @@ static int asn1_parse2(BIO *bp, const unsigned char **pp, long length, } ret = 1; end: - if (o != NULL) - ASN1_OBJECT_free(o); - if (os != NULL) - ASN1_OCTET_STRING_free(os); + ASN1_OBJECT_free(o); + ASN1_OCTET_STRING_free(os); *pp = p; return (ret); } diff --git a/crypto/asn1/evp_asn1.c b/crypto/asn1/evp_asn1.c index 3664576a81..e6a5b5f28f 100644 --- a/crypto/asn1/evp_asn1.c +++ b/crypto/asn1/evp_asn1.c @@ -187,8 +187,7 @@ int ASN1_TYPE_get_int_octetstring(ASN1_TYPE *a, long *num, err: ASN1err(ASN1_F_ASN1_TYPE_GET_INT_OCTETSTRING, ASN1_R_DATA_IS_WRONG); } - if (os != NULL) - ASN1_OCTET_STRING_free(os); + ASN1_OCTET_STRING_free(os); if (ai != NULL) ASN1_INTEGER_free(ai); return (ret); diff --git a/crypto/asn1/p5_pbe.c b/crypto/asn1/p5_pbe.c index bdbfdcd67c..d54b0943fe 100644 --- a/crypto/asn1/p5_pbe.c +++ b/crypto/asn1/p5_pbe.c @@ -118,8 +118,7 @@ int PKCS5_pbe_set0_algor(X509_ALGOR *algor, int alg, int iter, err: if (pbe != NULL) PBEPARAM_free(pbe); - if (pbe_str != NULL) - ASN1_STRING_free(pbe_str); + ASN1_STRING_free(pbe_str); return 0; } diff --git a/crypto/asn1/tasn_fre.c b/crypto/asn1/tasn_fre.c index 49c5793a49..bdc26f9bb4 100644 --- a/crypto/asn1/tasn_fre.c +++ b/crypto/asn1/tasn_fre.c @@ -85,6 +85,7 @@ static void asn1_item_combine_free(ASN1_VALUE **pval, const ASN1_ITEM *it, const ASN1_AUX *aux = it->funcs; ASN1_aux_cb *asn1_cb; int i; + if (!pval) return; if ((it->itype != ASN1_ITYPE_PRIMITIVE) && !*pval) @@ -116,6 +117,7 @@ static void asn1_item_combine_free(ASN1_VALUE **pval, const ASN1_ITEM *it, i = asn1_get_choice_selector(pval, it); if ((i >= 0) && (i < it->tcount)) { ASN1_VALUE **pchval; + tt = it->templates + i; pchval = asn1_get_field_ptr(pval, tt); ASN1_template_free(pchval, tt); @@ -170,35 +172,41 @@ static void asn1_item_combine_free(ASN1_VALUE **pval, const ASN1_ITEM *it, void ASN1_template_free(ASN1_VALUE **pval, const ASN1_TEMPLATE *tt) { - int i; if (tt->flags & ASN1_TFLG_SK_MASK) { STACK_OF(ASN1_VALUE) *sk = (STACK_OF(ASN1_VALUE) *)*pval; + int i; + for (i = 0; i < sk_ASN1_VALUE_num(sk); i++) { - ASN1_VALUE *vtmp; - vtmp = sk_ASN1_VALUE_value(sk, i); + ASN1_VALUE *vtmp = sk_ASN1_VALUE_value(sk, i); + asn1_item_combine_free(&vtmp, ASN1_ITEM_ptr(tt->item), 0); } sk_ASN1_VALUE_free(sk); *pval = NULL; - } else + } else { asn1_item_combine_free(pval, ASN1_ITEM_ptr(tt->item), tt->flags & ASN1_TFLG_COMBINE); + } } void ASN1_primitive_free(ASN1_VALUE **pval, const ASN1_ITEM *it) { int utype; + + /* Special case: if 'it' is a primitive with a free_func, use that. */ if (it) { - const ASN1_PRIMITIVE_FUNCS *pf; - pf = it->funcs; + const ASN1_PRIMITIVE_FUNCS *pf = it->funcs; + if (pf && pf->prim_free) { pf->prim_free(pval, it); return; } } - /* Special case: if 'it' is NULL free contents of ASN1_TYPE */ + + /* Special case: if 'it' is NULL, free contents of ASN1_TYPE */ if (!it) { ASN1_TYPE *typ = (ASN1_TYPE *)*pval; + utype = typ->type; pval = &typ->value.asn1_value; if (!*pval) @@ -235,8 +243,8 @@ void ASN1_primitive_free(ASN1_VALUE **pval, const ASN1_ITEM *it) default: ASN1_STRING_free((ASN1_STRING *)*pval); - *pval = NULL; break; } - *pval = NULL; + if (*pval) + *pval = NULL; } diff --git a/crypto/asn1/x_algor.c b/crypto/asn1/x_algor.c index 0aa3dedbae..30d648159f 100644 --- a/crypto/asn1/x_algor.c +++ b/crypto/asn1/x_algor.c @@ -86,8 +86,7 @@ int X509_ALGOR_set0(X509_ALGOR *alg, ASN1_OBJECT *aobj, int ptype, void *pval) return 0; } if (alg) { - if (alg->algorithm) - ASN1_OBJECT_free(alg->algorithm); + ASN1_OBJECT_free(alg->algorithm); alg->algorithm = aobj; } if (ptype == 0) diff --git a/crypto/asn1/x_pkey.c b/crypto/asn1/x_pkey.c index cf5fd804a9..f4396e7d17 100644 --- a/crypto/asn1/x_pkey.c +++ b/crypto/asn1/x_pkey.c @@ -143,8 +143,7 @@ void X509_PKEY_free(X509_PKEY *x) if (x->enc_algor != NULL) X509_ALGOR_free(x->enc_algor); - if (x->enc_pkey != NULL) - ASN1_OCTET_STRING_free(x->enc_pkey); + ASN1_OCTET_STRING_free(x->enc_pkey); if (x->dec_pkey != NULL) EVP_PKEY_free(x->dec_pkey); if ((x->key_data != NULL) && (x->key_free)) diff --git a/crypto/asn1/x_x509a.c b/crypto/asn1/x_x509a.c index 2a2ca87178..8be50b55e2 100644 --- a/crypto/asn1/x_x509a.c +++ b/crypto/asn1/x_x509a.c @@ -159,8 +159,7 @@ int X509_add1_trust_object(X509 *x, ASN1_OBJECT *obj) if (!objtmp || sk_ASN1_OBJECT_push(aux->trust, objtmp)) return 1; err: - if (objtmp) - ASN1_OBJECT_free(objtmp); + ASN1_OBJECT_free(objtmp); return 0; } diff --git a/crypto/dh/dh_ameth.c b/crypto/dh/dh_ameth.c index 2c77381a48..e7d56f1a89 100644 --- a/crypto/dh/dh_ameth.c +++ b/crypto/dh/dh_ameth.c @@ -191,8 +191,7 @@ static int dh_pub_encode(X509_PUBKEY *pk, const EVP_PKEY *pkey) err: if (penc) OPENSSL_free(penc); - if (str) - ASN1_STRING_free(str); + ASN1_STRING_free(str); return 0; } @@ -297,8 +296,7 @@ static int dh_priv_encode(PKCS8_PRIV_KEY_INFO *p8, const EVP_PKEY *pkey) err: if (dp != NULL) OPENSSL_free(dp); - if (params != NULL) - ASN1_STRING_free(params); + ASN1_STRING_free(params); if (prkey != NULL) ASN1_STRING_clear_free(prkey); return 0; diff --git a/crypto/dh/dh_pmeth.c b/crypto/dh/dh_pmeth.c index 8975f4492a..668f5f3a87 100644 --- a/crypto/dh/dh_pmeth.c +++ b/crypto/dh/dh_pmeth.c @@ -155,8 +155,7 @@ static void pkey_dh_cleanup(EVP_PKEY_CTX *ctx) if (dctx) { if (dctx->kdf_ukm) OPENSSL_free(dctx->kdf_ukm); - if (dctx->kdf_oid) - ASN1_OBJECT_free(dctx->kdf_oid); + ASN1_OBJECT_free(dctx->kdf_oid); OPENSSL_free(dctx); } } @@ -245,8 +244,7 @@ static int pkey_dh_ctrl(EVP_PKEY_CTX *ctx, int type, int p1, void *p2) return dctx->kdf_ukmlen; case EVP_PKEY_CTRL_DH_KDF_OID: - if (dctx->kdf_oid) - ASN1_OBJECT_free(dctx->kdf_oid); + ASN1_OBJECT_free(dctx->kdf_oid); dctx->kdf_oid = p2; return 1; diff --git a/crypto/dsa/dsa_ameth.c b/crypto/dsa/dsa_ameth.c index d63c417d9f..425144ac1c 100644 --- a/crypto/dsa/dsa_ameth.c +++ b/crypto/dsa/dsa_ameth.c @@ -166,8 +166,7 @@ static int dsa_pub_encode(X509_PUBKEY *pk, const EVP_PKEY *pkey) err: if (penc) OPENSSL_free(penc); - if (str) - ASN1_STRING_free(str); + ASN1_STRING_free(str); return 0; } @@ -328,8 +327,7 @@ static int dsa_priv_encode(PKCS8_PRIV_KEY_INFO *p8, const EVP_PKEY *pkey) err: if (dp != NULL) OPENSSL_free(dp); - if (params != NULL) - ASN1_STRING_free(params); + ASN1_STRING_free(params); if (prkey != NULL) ASN1_STRING_clear_free(prkey); return 0; diff --git a/crypto/ec/ec_asn1.c b/crypto/ec/ec_asn1.c index 87cc334444..90de23b116 100644 --- a/crypto/ec/ec_asn1.c +++ b/crypto/ec/ec_asn1.c @@ -317,8 +317,7 @@ static int ec_asn1_group2fieldid(const EC_GROUP *group, X9_62_FIELDID *field) return 0; /* clear the old values (if necessary) */ - if (field->fieldType != NULL) - ASN1_OBJECT_free(field->fieldType); + ASN1_OBJECT_free(field->fieldType); if (field->p.other != NULL) ASN1_TYPE_free(field->p.other); @@ -654,7 +653,7 @@ ECPKPARAMETERS *ec_asn1_group2pkparameters(const EC_GROUP *group, return NULL; } } else { - if (ret->type == 0 && ret->value.named_curve) + if (ret->type == 0) ASN1_OBJECT_free(ret->value.named_curve); else if (ret->type == 1 && ret->value.parameters) ECPARAMETERS_free(ret->value.parameters); diff --git a/crypto/ocsp/ocsp_lib.c b/crypto/ocsp/ocsp_lib.c index 8e87f49a5d..34df9ac04e 100644 --- a/crypto/ocsp/ocsp_lib.c +++ b/crypto/ocsp/ocsp_lib.c @@ -110,8 +110,7 @@ OCSP_CERTID *OCSP_cert_id_new(const EVP_MD *dgst, goto err; alg = cid->hashAlgorithm; - if (alg->algorithm != NULL) - ASN1_OBJECT_free(alg->algorithm); + ASN1_OBJECT_free(alg->algorithm); if ((nid = EVP_MD_type(dgst)) == NID_undef) { OCSPerr(OCSP_F_OCSP_CERT_ID_NEW, OCSP_R_UNKNOWN_NID); goto err; diff --git a/crypto/ocsp/v3_ocsp.c b/crypto/ocsp/v3_ocsp.c index 655811671a..7e502d7962 100644 --- a/crypto/ocsp/v3_ocsp.c +++ b/crypto/ocsp/v3_ocsp.c @@ -247,7 +247,7 @@ static void *d2i_ocsp_nonce(void *a, const unsigned char **pp, long length) return os; err: - if (os && (!pos || (*pos != os))) + if ((pos == NULL) || (*pos != os)) ASN1_OCTET_STRING_free(os); OCSPerr(OCSP_F_D2I_OCSP_NONCE, ERR_R_MALLOC_FAILURE); return NULL; diff --git a/crypto/rsa/rsa_ameth.c b/crypto/rsa/rsa_ameth.c index 6f4c104858..071dbb8d68 100644 --- a/crypto/rsa/rsa_ameth.c +++ b/crypto/rsa/rsa_ameth.c @@ -484,8 +484,7 @@ static int rsa_md_to_mgf1(X509_ALGOR **palg, const EVP_MD *mgf1md) X509_ALGOR_set0(*palg, OBJ_nid2obj(NID_mgf1), V_ASN1_SEQUENCE, stmp); stmp = NULL; err: - if (stmp) - ASN1_STRING_free(stmp); + ASN1_STRING_free(stmp); if (algtmp) X509_ALGOR_free(algtmp); if (*palg) @@ -576,8 +575,7 @@ static ASN1_STRING *rsa_ctx_to_pss(EVP_PKEY_CTX *pkctx) RSA_PSS_PARAMS_free(pss); if (rv) return os; - if (os) - ASN1_STRING_free(os); + ASN1_STRING_free(os); return NULL; } @@ -921,8 +919,7 @@ static int rsa_cms_encrypt(CMS_RecipientInfo *ri) err: if (oaep) RSA_OAEP_PARAMS_free(oaep); - if (os) - ASN1_STRING_free(os); + ASN1_STRING_free(os); return rv; } diff --git a/crypto/rsa/rsa_saos.c b/crypto/rsa/rsa_saos.c index 6ebab3db55..0f15f00842 100644 --- a/crypto/rsa/rsa_saos.c +++ b/crypto/rsa/rsa_saos.c @@ -138,8 +138,7 @@ int RSA_verify_ASN1_OCTET_STRING(int dtype, } else ret = 1; err: - if (sig != NULL) - ASN1_OCTET_STRING_free(sig); + ASN1_OCTET_STRING_free(sig); if (s != NULL) { OPENSSL_cleanse(s, (unsigned int)siglen); OPENSSL_free(s); diff --git a/crypto/x509v3/pcy_data.c b/crypto/x509v3/pcy_data.c index 90e9970e46..3a8d4328d5 100644 --- a/crypto/x509v3/pcy_data.c +++ b/crypto/x509v3/pcy_data.c @@ -102,8 +102,7 @@ X509_POLICY_DATA *policy_data_new(POLICYINFO *policy, ret->expected_policy_set = sk_ASN1_OBJECT_new_null(); if (!ret->expected_policy_set) { OPENSSL_free(ret); - if (id) - ASN1_OBJECT_free(id); + ASN1_OBJECT_free(id); return NULL; } diff --git a/crypto/x509v3/v3_conf.c b/crypto/x509v3/v3_conf.c index 9631e57b77..eb9cfea66d 100644 --- a/crypto/x509v3/v3_conf.c +++ b/crypto/x509v3/v3_conf.c @@ -212,8 +212,7 @@ static X509_EXTENSION *do_ext_i2d(const X509V3_EXT_METHOD *method, X509V3err(X509V3_F_DO_EXT_I2D, ERR_R_MALLOC_FAILURE); if (ext_der != NULL) OPENSSL_free(ext_der); - if (ext_oct != NULL) - ASN1_OCTET_STRING_free(ext_oct); + ASN1_OCTET_STRING_free(ext_oct); return NULL; } diff --git a/crypto/x509v3/v3_pci.c b/crypto/x509v3/v3_pci.c index 5a937170f8..4139b34cbc 100644 --- a/crypto/x509v3/v3_pci.c +++ b/crypto/x509v3/v3_pci.c @@ -305,10 +305,7 @@ static PROXY_CERT_INFO_EXTENSION *r2i_pci(X509V3_EXT_METHOD *method, pathlen = NULL; goto end; err: - if (language) { - ASN1_OBJECT_free(language); - language = NULL; - } + ASN1_OBJECT_free(language); if (pathlen) { ASN1_INTEGER_free(pathlen); pathlen = NULL; diff --git a/doc/crypto/ASN1_OBJECT_new.pod b/doc/crypto/ASN1_OBJECT_new.pod index 338b72653e..36fc5716ec 100644 --- a/doc/crypto/ASN1_OBJECT_new.pod +++ b/doc/crypto/ASN1_OBJECT_new.pod @@ -19,6 +19,7 @@ ASN1_OBJECT structure, which represents an ASN1 OBJECT IDENTIFIER. ASN1_OBJECT_new() allocates and initializes a ASN1_OBJECT structure. ASN1_OBJECT_free() frees up the B structure B. +If B is NULL, nothing is done. =head1 NOTES diff --git a/doc/crypto/ASN1_STRING_new.pod b/doc/crypto/ASN1_STRING_new.pod index 8ac2a03ae2..6c0b303c27 100644 --- a/doc/crypto/ASN1_STRING_new.pod +++ b/doc/crypto/ASN1_STRING_new.pod @@ -22,6 +22,7 @@ ASN1_STRING_type_new() returns an allocated B structure of type B. ASN1_STRING_free() frees up B. +If B is NULL nothing is done. =head1 NOTES -- 2.25.1