asn1_template_noexp_d2i call ASN1_item_ex_free(&skfield,...) on error. Reworked error handling in asn1_item_ex_combine_new: - call ASN1_item_ex_free and return the correct error code if ASN1_template_new failed. - dont call ASN1_item_ex_free if ASN1_OP_NEW_PRE failed. Reworked error handing in x509_name_ex_d2i and x509_name_encode. (Imported from upstream's 748cb9a17f4f2b77aad816cf658cd4025dc847ee.) I believe the tasn1_new.c change is a no-op since we have no ASN1_OP_NEW_PRE hooks anymore. I'm not sure what the commit message is referring to with ASN1_template_new. It also seems odd as ASN1_item_ex_free should probably be able to survive *pval being NULL. Whatever. We'd previously tried to fix x509_name_ex_d2i, but I think ours wasn't quite right. (This thing is a mess...) I've aligned that function with upstream. Change-Id: Ie71521cd8a1ec357876caadd13be1ce247110f76 Reviewed-on: https://boringssl-review.googlesource.com/13831 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>kris/onging/CECPQ3_patch15
@@ -667,6 +667,7 @@ static int asn1_template_noexp_d2i(ASN1_VALUE **val, | |||||
} | } | ||||
len -= p - q; | len -= p - q; | ||||
if (!sk_ASN1_VALUE_push((STACK_OF(ASN1_VALUE) *)*val, skfield)) { | if (!sk_ASN1_VALUE_push((STACK_OF(ASN1_VALUE) *)*val, skfield)) { | ||||
ASN1_item_ex_free(&skfield, ASN1_ITEM_ptr(tt->item)); | |||||
OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); | OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); | ||||
goto err; | goto err; | ||||
} | } | ||||
@@ -160,7 +160,7 @@ static int asn1_item_ex_combine_new(ASN1_VALUE **pval, const ASN1_ITEM *it, | |||||
} | } | ||||
asn1_set_choice_selector(pval, -1, it); | asn1_set_choice_selector(pval, -1, it); | ||||
if (asn1_cb && !asn1_cb(ASN1_OP_NEW_POST, pval, it, NULL)) | if (asn1_cb && !asn1_cb(ASN1_OP_NEW_POST, pval, it, NULL)) | ||||
goto auxerr; | |||||
goto auxerr2; | |||||
break; | break; | ||||
case ASN1_ITYPE_NDEF_SEQUENCE: | case ASN1_ITYPE_NDEF_SEQUENCE: | ||||
@@ -188,10 +188,10 @@ static int asn1_item_ex_combine_new(ASN1_VALUE **pval, const ASN1_ITEM *it, | |||||
for (i = 0, tt = it->templates; i < it->tcount; tt++, i++) { | for (i = 0, tt = it->templates; i < it->tcount; tt++, i++) { | ||||
pseqval = asn1_get_field_ptr(pval, tt); | pseqval = asn1_get_field_ptr(pval, tt); | ||||
if (!ASN1_template_new(pseqval, tt)) | if (!ASN1_template_new(pseqval, tt)) | ||||
goto memerr; | |||||
goto memerr2; | |||||
} | } | ||||
if (asn1_cb && !asn1_cb(ASN1_OP_NEW_POST, pval, it, NULL)) | if (asn1_cb && !asn1_cb(ASN1_OP_NEW_POST, pval, it, NULL)) | ||||
goto auxerr; | |||||
goto auxerr2; | |||||
break; | break; | ||||
} | } | ||||
#ifdef CRYPTO_MDEBUG | #ifdef CRYPTO_MDEBUG | ||||
@@ -200,18 +200,20 @@ static int asn1_item_ex_combine_new(ASN1_VALUE **pval, const ASN1_ITEM *it, | |||||
#endif | #endif | ||||
return 1; | return 1; | ||||
memerr2: | |||||
ASN1_item_ex_free(pval, it); | |||||
memerr: | memerr: | ||||
OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); | OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE); | ||||
ASN1_item_ex_free(pval, it); | |||||
#ifdef CRYPTO_MDEBUG | #ifdef CRYPTO_MDEBUG | ||||
if (it->sname) | if (it->sname) | ||||
CRYPTO_pop_info(); | CRYPTO_pop_info(); | ||||
#endif | #endif | ||||
return 0; | return 0; | ||||
auxerr2: | |||||
ASN1_item_ex_free(pval, it); | |||||
auxerr: | auxerr: | ||||
OPENSSL_PUT_ERROR(ASN1, ASN1_R_AUX_ERROR); | OPENSSL_PUT_ERROR(ASN1, ASN1_R_AUX_ERROR); | ||||
ASN1_item_ex_free(pval, it); | |||||
#ifdef CRYPTO_MDEBUG | #ifdef CRYPTO_MDEBUG | ||||
if (it->sname) | if (it->sname) | ||||
CRYPTO_pop_info(); | CRYPTO_pop_info(); | ||||
@@ -229,12 +229,11 @@ static int x509_name_ex_d2i(ASN1_VALUE **val, | |||||
if (*val) | if (*val) | ||||
x509_name_ex_free(val, NULL); | x509_name_ex_free(val, NULL); | ||||
if (!x509_name_ex_new(&nm.a, NULL)) | |||||
goto err; | |||||
/* We've decoded it: now cache encoding */ | /* We've decoded it: now cache encoding */ | ||||
if (!x509_name_ex_new(&nm.a, NULL) || !BUF_MEM_grow(nm.x->bytes, p - q)) { | |||||
sk_STACK_OF_X509_NAME_ENTRY_pop_free(intname.s, | |||||
local_sk_X509_NAME_ENTRY_pop_free); | |||||
if (!BUF_MEM_grow(nm.x->bytes, p - q)) | |||||
goto err; | goto err; | ||||
} | |||||
OPENSSL_memcpy(nm.x->bytes->data, q, p - q); | OPENSSL_memcpy(nm.x->bytes->data, q, p - q); | ||||
/* Convert internal representation to X509_NAME structure */ | /* Convert internal representation to X509_NAME structure */ | ||||
@@ -245,13 +244,14 @@ static int x509_name_ex_d2i(ASN1_VALUE **val, | |||||
entry->set = i; | entry->set = i; | ||||
if (!sk_X509_NAME_ENTRY_push(nm.x->entries, entry)) | if (!sk_X509_NAME_ENTRY_push(nm.x->entries, entry)) | ||||
goto err; | goto err; | ||||
sk_X509_NAME_ENTRY_set(entries, j, NULL); | |||||
} | } | ||||
sk_X509_NAME_ENTRY_free(entries); | |||||
} | } | ||||
sk_STACK_OF_X509_NAME_ENTRY_free(intname.s); | |||||
ret = x509_name_canon(nm.x); | ret = x509_name_canon(nm.x); | ||||
if (!ret) | if (!ret) | ||||
goto err; | goto err; | ||||
sk_STACK_OF_X509_NAME_ENTRY_pop_free(intname.s, | |||||
local_sk_X509_NAME_ENTRY_free); | |||||
nm.x->modified = 0; | nm.x->modified = 0; | ||||
*val = nm.a; | *val = nm.a; | ||||
*in = p; | *in = p; | ||||
@@ -259,6 +259,8 @@ static int x509_name_ex_d2i(ASN1_VALUE **val, | |||||
err: | err: | ||||
if (nm.x != NULL) | if (nm.x != NULL) | ||||
X509_NAME_free(nm.x); | X509_NAME_free(nm.x); | ||||
sk_STACK_OF_X509_NAME_ENTRY_pop_free(intname.s, | |||||
local_sk_X509_NAME_ENTRY_pop_free); | |||||
OPENSSL_PUT_ERROR(X509, ERR_R_ASN1_LIB); | OPENSSL_PUT_ERROR(X509, ERR_R_ASN1_LIB); | ||||
return 0; | return 0; | ||||
} | } | ||||
@@ -307,8 +309,10 @@ static int x509_name_encode(X509_NAME *a) | |||||
entries = sk_X509_NAME_ENTRY_new_null(); | entries = sk_X509_NAME_ENTRY_new_null(); | ||||
if (!entries) | if (!entries) | ||||
goto memerr; | goto memerr; | ||||
if (!sk_STACK_OF_X509_NAME_ENTRY_push(intname.s, entries)) | |||||
if (!sk_STACK_OF_X509_NAME_ENTRY_push(intname.s, entries)) { | |||||
sk_X509_NAME_ENTRY_free(entries); | |||||
goto memerr; | goto memerr; | ||||
} | |||||
set = entry->set; | set = entry->set; | ||||
} | } | ||||
if (!sk_X509_NAME_ENTRY_push(entries, entry)) | if (!sk_X509_NAME_ENTRY_push(entries, entry)) | ||||