Fix various malloc failure paths.
Caught by running malloc failure tests on unit tests.
Bug: 563
Change-Id: Ic0167ef346a282dc8b5a26a1cedafced7fef9ed0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56927
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/crypto/asn1/a_mbstr.c b/crypto/asn1/a_mbstr.c
index ef74d0d..81916c2 100644
--- a/crypto/asn1/a_mbstr.c
+++ b/crypto/asn1/a_mbstr.c
@@ -85,11 +85,6 @@
int ASN1_mbstring_ncopy(ASN1_STRING **out, const unsigned char *in, int len,
int inform, unsigned long mask, long minsize,
long maxsize) {
- int str_type;
- char free_out;
- ASN1_STRING *dest;
- size_t nchar = 0;
- char strbuf[32];
if (len == -1) {
len = strlen((const char *)in);
}
@@ -128,7 +123,7 @@
// Check |minsize| and |maxsize| and work out the minimal type, if any.
CBS cbs;
CBS_init(&cbs, in, len);
- size_t utf8_len = 0;
+ size_t utf8_len = 0, nchar = 0;
while (CBS_len(&cbs) != 0) {
uint32_t c;
if (!decode_func(&cbs, &c)) {
@@ -169,6 +164,7 @@
utf8_len += cbb_get_utf8_len(c);
}
+ char strbuf[32];
if (minsize > 0 && nchar < (size_t)minsize) {
OPENSSL_PUT_ERROR(ASN1, ASN1_R_STRING_TOO_SHORT);
BIO_snprintf(strbuf, sizeof strbuf, "%ld", minsize);
@@ -184,6 +180,7 @@
}
// Now work out output format and string type
+ int str_type;
int (*encode_func)(CBB *, uint32_t) = cbb_add_latin1;
size_t size_estimate = nchar;
int outform = MBSTRING_ASC;
@@ -216,31 +213,28 @@
if (!out) {
return str_type;
}
+
+ int free_dest = 0;
+ ASN1_STRING *dest;
if (*out) {
- free_out = 0;
dest = *out;
- if (dest->data) {
- dest->length = 0;
- OPENSSL_free(dest->data);
- dest->data = NULL;
- }
- dest->type = str_type;
} else {
- free_out = 1;
+ free_dest = 1;
dest = ASN1_STRING_type_new(str_type);
if (!dest) {
OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE);
return -1;
}
- *out = dest;
}
// If both the same type just copy across
if (inform == outform) {
if (!ASN1_STRING_set(dest, in, len)) {
OPENSSL_PUT_ERROR(ASN1, ERR_R_MALLOC_FAILURE);
- return -1;
+ goto err;
}
+ dest->type = str_type;
+ *out = dest;
return str_type;
}
@@ -267,12 +261,13 @@
OPENSSL_free(data);
goto err;
}
- dest->length = (int)(data_len - 1);
- dest->data = data;
+ dest->type = str_type;
+ ASN1_STRING_set0(dest, data, (int)data_len - 1);
+ *out = dest;
return str_type;
err:
- if (free_out) {
+ if (free_dest) {
ASN1_STRING_free(dest);
}
CBB_cleanup(&cbb);
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 59e80d2..3bb7b34 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -1339,6 +1339,7 @@
SCOPED_TRACE(t.flags);
bssl::UniquePtr<ASN1_STRING> str(ASN1_STRING_type_new(t.type));
+ ASSERT_TRUE(str);
ASSERT_TRUE(ASN1_STRING_set(str.get(), t.data.data(), t.data.size()));
str->flags = t.str_flags;
@@ -1393,6 +1394,7 @@
SCOPED_TRACE(t.flags);
bssl::UniquePtr<ASN1_STRING> str(ASN1_STRING_type_new(t.type));
+ ASSERT_TRUE(str);
ASSERT_TRUE(ASN1_STRING_set(str.get(), t.data.data(), t.data.size()));
str->flags = t.str_flags;
diff --git a/crypto/ecdh_extra/ecdh_test.cc b/crypto/ecdh_extra/ecdh_test.cc
index 4b88754..3948525 100644
--- a/crypto/ecdh_extra/ecdh_test.cc
+++ b/crypto/ecdh_extra/ecdh_test.cc
@@ -274,6 +274,7 @@
}
bssl::UniquePtr<EC_KEY> key(EC_KEY_new());
+ ASSERT_TRUE(key);
ASSERT_TRUE(EC_KEY_set_group(key.get(), a.get()));
ASSERT_TRUE(EC_KEY_generate_key(key.get()));
diff --git a/crypto/evp/p_hkdf.c b/crypto/evp/p_hkdf.c
index 932372d..05158e2 100644
--- a/crypto/evp/p_hkdf.c
+++ b/crypto/evp/p_hkdf.c
@@ -64,7 +64,7 @@
if (hctx_src->key_len != 0) {
hctx_dst->key = OPENSSL_memdup(hctx_src->key, hctx_src->key_len);
- if (hctx_src->key == NULL) {
+ if (hctx_dst->key == NULL) {
OPENSSL_PUT_ERROR(EVP, ERR_R_MALLOC_FAILURE);
return 0;
}
@@ -73,7 +73,7 @@
if (hctx_src->salt_len != 0) {
hctx_dst->salt = OPENSSL_memdup(hctx_src->salt, hctx_src->salt_len);
- if (hctx_src->salt == NULL) {
+ if (hctx_dst->salt == NULL) {
OPENSSL_PUT_ERROR(EVP, ERR_R_MALLOC_FAILURE);
return 0;
}
diff --git a/crypto/fipsmodule/bn/exponentiation.c b/crypto/fipsmodule/bn/exponentiation.c
index 4ec9171..41c7233 100644
--- a/crypto/fipsmodule/bn/exponentiation.c
+++ b/crypto/fipsmodule/bn/exponentiation.c
@@ -444,6 +444,7 @@
return BN_one(r);
}
+ BN_RECP_CTX_init(&recp);
BN_CTX_start(ctx);
aa = BN_CTX_get(ctx);
val[0] = BN_CTX_get(ctx);
@@ -451,7 +452,6 @@
goto err;
}
- BN_RECP_CTX_init(&recp);
if (m->neg) {
// ignore sign of 'm'
if (!BN_copy(aa, m)) {
diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc
index 8e144ec..88665b2 100644
--- a/crypto/fipsmodule/ec/ec_test.cc
+++ b/crypto/fipsmodule/ec/ec_test.cc
@@ -656,6 +656,7 @@
bssl::UniquePtr<EC_POINT> inf1(EC_POINT_new(group())),
inf2(EC_POINT_new(group()));
ASSERT_TRUE(inf1);
+ ASSERT_TRUE(inf2);
ASSERT_TRUE(EC_POINT_set_to_infinity(group(), inf1.get()));
// |q| is currently -|pub2|.
ASSERT_TRUE(EC_POINT_add(group(), inf2.get(), pub2, q.get(), nullptr));
@@ -843,8 +844,8 @@
bssl::UniquePtr<EC_KEY> key(EC_KEY_new_by_curve_name(GetParam()));
ASSERT_TRUE(key);
- bssl::UniquePtr<BIGNUM> bn(BN_new());
- ASSERT_TRUE(BN_one(bn.get()));
+ bssl::UniquePtr<BIGNUM> bn(BN_dup(BN_value_one()));
+ ASSERT_TRUE(bn);
BN_set_negative(bn.get(), 1);
EXPECT_FALSE(EC_KEY_set_private_key(key.get(), bn.get()))
<< "Unexpectedly set a key of -1";
@@ -937,11 +938,13 @@
TEST_P(ECCurveTest, GPlusMinusG) {
const EC_POINT *g = EC_GROUP_get0_generator(group());
+
bssl::UniquePtr<EC_POINT> p(EC_POINT_dup(g, group()));
ASSERT_TRUE(p);
ASSERT_TRUE(EC_POINT_invert(group(), p.get(), nullptr));
- bssl::UniquePtr<EC_POINT> sum(EC_POINT_new(group()));
+ bssl::UniquePtr<EC_POINT> sum(EC_POINT_new(group()));
+ ASSERT_TRUE(sum);
ASSERT_TRUE(EC_POINT_add(group(), sum.get(), g, p.get(), nullptr));
EXPECT_TRUE(EC_POINT_is_at_infinity(group(), sum.get()));
}
diff --git a/crypto/obj/obj.c b/crypto/obj/obj.c
index 958625d..c4e1aee 100644
--- a/crypto/obj/obj.c
+++ b/crypto/obj/obj.c
@@ -506,25 +506,37 @@
// obj_add_object inserts |obj| into the various global hashes for run-time
// added objects. It returns one on success or zero otherwise.
static int obj_add_object(ASN1_OBJECT *obj) {
- int ok;
- ASN1_OBJECT *old_object;
-
obj->flags &= ~(ASN1_OBJECT_FLAG_DYNAMIC | ASN1_OBJECT_FLAG_DYNAMIC_STRINGS |
ASN1_OBJECT_FLAG_DYNAMIC_DATA);
CRYPTO_STATIC_MUTEX_lock_write(&global_added_lock);
if (global_added_by_nid == NULL) {
global_added_by_nid = lh_ASN1_OBJECT_new(hash_nid, cmp_nid);
+ }
+ if (global_added_by_data == NULL) {
global_added_by_data = lh_ASN1_OBJECT_new(hash_data, cmp_data);
- global_added_by_short_name = lh_ASN1_OBJECT_new(hash_short_name, cmp_short_name);
+ }
+ if (global_added_by_short_name == NULL) {
+ global_added_by_short_name =
+ lh_ASN1_OBJECT_new(hash_short_name, cmp_short_name);
+ }
+ if (global_added_by_long_name == NULL) {
global_added_by_long_name = lh_ASN1_OBJECT_new(hash_long_name, cmp_long_name);
}
+ int ok = 0;
+ if (global_added_by_nid == NULL ||
+ global_added_by_data == NULL ||
+ global_added_by_short_name == NULL ||
+ global_added_by_long_name == NULL) {
+ goto err;
+ }
+
// We don't pay attention to |old_object| (which contains any previous object
// that was evicted from the hashes) because we don't have a reference count
// on ASN1_OBJECT values. Also, we should never have duplicates nids and so
// should always have objects in |global_added_by_nid|.
-
+ ASN1_OBJECT *old_object;
ok = lh_ASN1_OBJECT_insert(global_added_by_nid, &old_object, obj);
if (obj->length != 0 && obj->data != NULL) {
ok &= lh_ASN1_OBJECT_insert(global_added_by_data, &old_object, obj);
@@ -535,8 +547,9 @@
if (obj->ln != NULL) {
ok &= lh_ASN1_OBJECT_insert(global_added_by_long_name, &old_object, obj);
}
- CRYPTO_STATIC_MUTEX_unlock_write(&global_added_lock);
+err:
+ CRYPTO_STATIC_MUTEX_unlock_write(&global_added_lock);
return ok;
}
diff --git a/crypto/pkcs7/pkcs7_test.cc b/crypto/pkcs7/pkcs7_test.cc
index bf85379..3c042ec 100644
--- a/crypto/pkcs7/pkcs7_test.cc
+++ b/crypto/pkcs7/pkcs7_test.cc
@@ -639,6 +639,7 @@
bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(pem, strlen(pem)));
ASSERT_TRUE(bio);
bssl::UniquePtr<STACK_OF(X509_CRL)> crls(sk_X509_CRL_new_null());
+ ASSERT_TRUE(crls);
ASSERT_TRUE(PKCS7_get_PEM_CRLs(crls.get(), bio.get()));
ASSERT_EQ(1u, sk_X509_CRL_num(crls.get()));
diff --git a/crypto/rsa_extra/rsa_test.cc b/crypto/rsa_extra/rsa_test.cc
index 883eb97..d3ee69b 100644
--- a/crypto/rsa_extra/rsa_test.cc
+++ b/crypto/rsa_extra/rsa_test.cc
@@ -513,6 +513,7 @@
SCOPED_TRACE(bits);
rsa.reset(RSA_new());
+ ASSERT_TRUE(rsa);
ASSERT_TRUE(RSA_generate_key_fips(rsa.get(), bits, nullptr));
EXPECT_EQ(bits, BN_num_bits(rsa->n));
}
diff --git a/crypto/stack/stack_test.cc b/crypto/stack/stack_test.cc
index 98e5448..1ff44b9 100644
--- a/crypto/stack/stack_test.cc
+++ b/crypto/stack/stack_test.cc
@@ -317,6 +317,7 @@
// sk_*_find should return the first matching element in all cases.
TEST(StackTest, FindFirst) {
bssl::UniquePtr<STACK_OF(TEST_INT)> sk(sk_TEST_INT_new(compare));
+ ASSERT_TRUE(sk);
auto value = TEST_INT_new(1);
ASSERT_TRUE(value);
ASSERT_TRUE(bssl::PushToStack(sk.get(), std::move(value)));
@@ -397,6 +398,7 @@
TEST(StackTest, DeleteIf) {
bssl::UniquePtr<STACK_OF(TEST_INT)> sk(sk_TEST_INT_new(compare));
+ ASSERT_TRUE(sk);
for (int v : {1, 9, 2, 8, 3, 7, 4, 6, 5}) {
auto obj = TEST_INT_new(v);
ASSERT_TRUE(obj);
diff --git a/crypto/x509/x509_cmp.c b/crypto/x509/x509_cmp.c
index 7e3c395..b640413 100644
--- a/crypto/x509/x509_cmp.c
+++ b/crypto/x509/x509_cmp.c
@@ -284,10 +284,12 @@
// count but it has the same effect by duping the STACK and upping the ref of
// each X509 structure.
STACK_OF(X509) *X509_chain_up_ref(STACK_OF(X509) *chain) {
- STACK_OF(X509) *ret;
- size_t i;
- ret = sk_X509_dup(chain);
- for (i = 0; i < sk_X509_num(ret); i++) {
+ STACK_OF(X509) *ret = sk_X509_dup(chain);
+ if (ret == NULL) {
+ OPENSSL_PUT_ERROR(X509, ERR_R_MALLOC_FAILURE);
+ return NULL;
+ }
+ for (size_t i = 0; i < sk_X509_num(ret); i++) {
X509_up_ref(sk_X509_value(ret, i));
}
return ret;
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index e152f3d..4f931b3 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -1391,6 +1391,7 @@
TEST(X509Test, ZeroLengthsWithCheckFunctions) {
bssl::UniquePtr<X509> leaf(CertFromPEM(kSANTypesLeaf));
+ ASSERT_TRUE(leaf);
EXPECT_EQ(
1, X509_check_host(leaf.get(), kHostname, strlen(kHostname), 0, nullptr));
@@ -2467,7 +2468,9 @@
for (auto t : asn1_utctime_tests) {
SCOPED_TRACE(t.val);
bssl::UniquePtr<ASN1_UTCTIME> tm(ASN1_UTCTIME_new());
+ ASSERT_TRUE(tm);
bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem()));
+ ASSERT_TRUE(bio);
// Use this instead of ASN1_UTCTIME_set() because some callers get
// type-confused and pass ASN1_GENERALIZEDTIME to ASN1_UTCTIME_print().
@@ -2525,6 +2528,7 @@
TEST(X509Test, X509NameSet) {
bssl::UniquePtr<X509_NAME> name(X509_NAME_new());
+ ASSERT_TRUE(name);
EXPECT_TRUE(X509_NAME_add_entry_by_txt(
name.get(), "C", MBSTRING_ASC, reinterpret_cast<const uint8_t *>("US"),
-1, -1, 0));
