Add malloc failure tests.
This commit fixes a number of crashes caused by malloc failures. They
were found using the -malloc-test=0 option to runner.go which runs tests
many times, causing a different allocation call to fail in each case.
(This test only works on Linux and only looks for crashes caused by
allocation failures, not memory leaks or other errors.)
This is not the complete set of crashes! More can be found by collecting
core dumps from running with -malloc-test=0.
Change-Id: Ia61d19f51e373bccb7bc604642c51e043a74bd83
Reviewed-on: https://boringssl-review.googlesource.com/2320
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/asn1/x_bignum.c b/crypto/asn1/x_bignum.c
index 2ffa093..7985235 100644
--- a/crypto/asn1/x_bignum.c
+++ b/crypto/asn1/x_bignum.c
@@ -126,7 +126,13 @@
int utype, char *free_cont, const ASN1_ITEM *it)
{
BIGNUM *bn;
- if(!*pval) bn_new(pval, it);
+ if(!*pval)
+ {
+ if (!bn_new(pval, it))
+ {
+ return 0;
+ }
+ }
bn = (BIGNUM *)*pval;
if(!BN_bin2bn(cont, len, bn)) {
bn_free(pval, it);
diff --git a/crypto/err/err.c b/crypto/err/err.c
index 6390b9a..6349a74 100644
--- a/crypto/err/err.c
+++ b/crypto/err/err.c
@@ -175,8 +175,7 @@
uint32_t ret;
state = err_get_state();
-
- if (state->bottom == state->top) {
+ if (state == NULL || state->bottom == state->top) {
return 0;
}
@@ -282,6 +281,10 @@
ERR_STATE *const state = err_get_state();
unsigned i;
+ if (state == NULL) {
+ return;
+ }
+
for (i = 0; i < ERR_NUM_ERRORS; i++) {
err_clear(&state->errors[i]);
}
@@ -481,7 +484,7 @@
ERR_STATE *const state = err_get_state();
struct err_error_st *error;
- if (state->top == state->bottom) {
+ if (state == NULL || state->top == state->bottom) {
if (flags & ERR_FLAG_MALLOCED) {
OPENSSL_free(data);
}
@@ -500,6 +503,10 @@
ERR_STATE *const state = err_get_state();
struct err_error_st *error;
+ if (state == NULL) {
+ return;
+ }
+
if (library == ERR_LIB_SYS && reason == 0) {
#if defined(WIN32)
reason = GetLastError();
@@ -600,7 +607,7 @@
int ERR_set_mark(void) {
ERR_STATE *const state = err_get_state();
- if (state->bottom == state->top) {
+ if (state == NULL || state->bottom == state->top) {
return 0;
}
state->errors[state->top].flags |= ERR_FLAG_MARK;
@@ -611,6 +618,10 @@
ERR_STATE *const state = err_get_state();
struct err_error_st *error;
+ if (state == NULL) {
+ return 0;
+ }
+
while (state->bottom != state->top) {
error = &state->errors[state->top];
diff --git a/crypto/err/err_impl.c b/crypto/err/err_impl.c
index 7554714..7428fb7 100644
--- a/crypto/err/err_impl.c
+++ b/crypto/err/err_impl.c
@@ -231,6 +231,11 @@
CRYPTO_r_lock(CRYPTO_LOCK_ERR);
}
+ if (state_hash == NULL) {
+ CRYPTO_r_unlock(CRYPTO_LOCK_ERR);
+ return NULL;
+ }
+
state = lh_ERR_STATE_retrieve(state_hash, &pattern);
CRYPTO_r_unlock(CRYPTO_LOCK_ERR);
if (state != NULL) {
diff --git a/crypto/evp/evp_ctx.c b/crypto/evp/evp_ctx.c
index d1ed67d..6616038 100644
--- a/crypto/evp/evp_ctx.c
+++ b/crypto/evp/evp_ctx.c
@@ -124,7 +124,10 @@
if (pmeth->init) {
if (pmeth->init(ret) <= 0) {
- EVP_PKEY_CTX_free(ret);
+ if (pkey) {
+ EVP_PKEY_free(ret->pkey);
+ }
+ OPENSSL_free(ret);
return NULL;
}
}
@@ -176,17 +179,25 @@
if (pctx->pkey) {
rctx->pkey = EVP_PKEY_dup(pctx->pkey);
+ if (rctx->pkey == NULL) {
+ goto err;
+ }
}
if (pctx->peerkey) {
rctx->peerkey = EVP_PKEY_dup(pctx->peerkey);
+ if (rctx->peerkey == NULL) {
+ goto err;
+ }
}
if (pctx->pmeth->copy(rctx, pctx) > 0) {
return rctx;
}
+err:
EVP_PKEY_CTX_free(rctx);
+ OPENSSL_PUT_ERROR(EVP, EVP_PKEY_CTX_dup, ERR_LIB_EVP);
return NULL;
}
@@ -485,6 +496,10 @@
if (!*ppkey) {
*ppkey = EVP_PKEY_new();
+ if (!*ppkey) {
+ OPENSSL_PUT_ERROR(EVP, EVP_PKEY_keygen, ERR_LIB_EVP);
+ return 0;
+ }
}
if (!ctx->pmeth->keygen(ctx, *ppkey)) {
diff --git a/crypto/evp/evp_error.c b/crypto/evp/evp_error.c
index d2d8aba..b0d311e 100644
--- a/crypto/evp/evp_error.c
+++ b/crypto/evp/evp_error.c
@@ -20,6 +20,7 @@
{ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_DigestSignAlgorithm, 0), "EVP_DigestSignAlgorithm"},
{ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_DigestVerifyInitFromAlgorithm, 0), "EVP_DigestVerifyInitFromAlgorithm"},
{ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_PKEY_CTX_ctrl, 0), "EVP_PKEY_CTX_ctrl"},
+ {ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_PKEY_CTX_dup, 0), "EVP_PKEY_CTX_dup"},
{ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_PKEY_copy_parameters, 0), "EVP_PKEY_copy_parameters"},
{ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_PKEY_decrypt, 0), "EVP_PKEY_decrypt"},
{ERR_PACK(ERR_LIB_EVP, EVP_F_EVP_PKEY_decrypt_init, 0), "EVP_PKEY_decrypt_init"},
diff --git a/crypto/evp/p_hmac.c b/crypto/evp/p_hmac.c
index f068f20..d781920 100644
--- a/crypto/evp/p_hmac.c
+++ b/crypto/evp/p_hmac.c
@@ -109,6 +109,10 @@
static void pkey_hmac_cleanup(EVP_PKEY_CTX *ctx) {
HMAC_PKEY_CTX *hctx = ctx->data;
+ if (hctx == NULL) {
+ return;
+ }
+
HMAC_CTX_cleanup(&hctx->ctx);
if (hctx->ktmp.data) {
if (hctx->ktmp.length) {
diff --git a/crypto/ex_data_impl.c b/crypto/ex_data_impl.c
index 972e85a..db811d4 100644
--- a/crypto/ex_data_impl.c
+++ b/crypto/ex_data_impl.c
@@ -193,8 +193,10 @@
static void cleanup(void) {
LHASH_OF(EX_CLASS_ITEM) *classes = get_classes();
- lh_EX_CLASS_ITEM_doall(classes, class_free);
- lh_EX_CLASS_ITEM_free(classes);
+ if (classes != NULL) {
+ lh_EX_CLASS_ITEM_doall(classes, class_free);
+ lh_EX_CLASS_ITEM_free(classes);
+ }
global_classes = NULL;
}
@@ -204,6 +206,10 @@
EX_CLASS_ITEM template, *class_item;
int ok = 0;
+ if (classes == NULL) {
+ return NULL;
+ }
+
CRYPTO_w_lock(CRYPTO_LOCK_EX_DATA);
template.class_value = class_value;
class_item = lh_EX_CLASS_ITEM_retrieve(classes, &template);
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 8094c78..030d3ce 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -2247,38 +2247,26 @@
STACK_OF(X509) *chain)
{
int ret = 1;
+ int ex_data_allocated = 0;
+
+ memset(ctx, 0, sizeof(X509_STORE_CTX));
ctx->ctx=store;
- ctx->current_method=0;
ctx->cert=x509;
ctx->untrusted=chain;
- ctx->crls = NULL;
- ctx->last_untrusted=0;
- ctx->other_ctx=NULL;
- ctx->valid=0;
- ctx->chain=NULL;
- ctx->error=0;
- ctx->explicit_policy=0;
- ctx->error_depth=0;
- ctx->current_cert=NULL;
- ctx->current_issuer=NULL;
- ctx->current_crl=NULL;
- ctx->current_crl_score=0;
- ctx->current_reasons=0;
- ctx->tree = NULL;
- ctx->parent = NULL;
+
+ if(!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_X509_STORE_CTX, ctx,
+ &ctx->ex_data))
+ {
+ goto err;
+ }
+ ex_data_allocated = 1;
ctx->param = X509_VERIFY_PARAM_new();
-
if (!ctx->param)
- {
- OPENSSL_PUT_ERROR(X509, X509_STORE_CTX_init, ERR_R_MALLOC_FAILURE);
- return 0;
- }
+ goto err;
/* Inherit callbacks and flags from X509_STORE if not set
- * use defaults.
- */
-
+ * use defaults. */
if (store)
ret = X509_VERIFY_PARAM_inherit(ctx->param, store->param);
@@ -2298,10 +2286,7 @@
X509_VERIFY_PARAM_lookup("default"));
if (ret == 0)
- {
- OPENSSL_PUT_ERROR(X509, X509_STORE_CTX_init, ERR_R_MALLOC_FAILURE);
- return 0;
- }
+ goto err;
if (store && store->check_issued)
ctx->check_issued = store->check_issued;
@@ -2355,19 +2340,21 @@
ctx->check_policy = check_policy;
-
- /* This memset() can't make any sense anyway, so it's removed. As
- * X509_STORE_CTX_cleanup does a proper "free" on the ex_data, we put a
- * corresponding "new" here and remove this bogus initialisation. */
- /* memset(&(ctx->ex_data),0,sizeof(CRYPTO_EX_DATA)); */
- if(!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_X509_STORE_CTX, ctx,
- &(ctx->ex_data)))
- {
- OPENSSL_free(ctx);
- OPENSSL_PUT_ERROR(X509, X509_STORE_CTX_init, ERR_R_MALLOC_FAILURE);
- return 0;
- }
return 1;
+
+err:
+ if (ex_data_allocated)
+ {
+ CRYPTO_free_ex_data(CRYPTO_EX_INDEX_X509_STORE_CTX, ctx, &ctx->ex_data);
+ }
+ if (ctx->param != NULL)
+ {
+ X509_VERIFY_PARAM_free(ctx->param);
+ }
+
+ memset(ctx, 0, sizeof(X509_STORE_CTX));
+ OPENSSL_PUT_ERROR(X509, X509_STORE_CTX_init, ERR_R_MALLOC_FAILURE);
+ return 0;
}
/* Set alternative lookup method: just a STACK of trusted certificates.
diff --git a/crypto/x509/x_name.c b/crypto/x509/x_name.c
index 56eeaf4..6e108f5 100644
--- a/crypto/x509/x_name.c
+++ b/crypto/x509/x_name.c
@@ -354,10 +354,15 @@
if(!entries)
goto err;
if(!sk_STACK_OF_X509_NAME_ENTRY_push(intname, entries))
+ {
+ sk_X509_NAME_ENTRY_free(entries);
goto err;
+ }
set = entry->set;
}
tmpentry = X509_NAME_ENTRY_new();
+ if (tmpentry == NULL)
+ goto err;
tmpentry->object = OBJ_dup(entry->object);
if (!asn1_string_canon(tmpentry->value, entry->value))
goto err;
