diff --git a/crypto/cmp/cmp_ctx.c b/crypto/cmp/cmp_ctx.c index e1b4e50ea9..ccca282721 100644 --- a/crypto/cmp/cmp_ctx.c +++ b/crypto/cmp/cmp_ctx.c @@ -462,8 +462,6 @@ STACK_OF(X509) *OSSL_CMP_CTX_get1_newChain(const OSSL_CMP_CTX *ctx) ERR_raise(ERR_LIB_CMP, CMP_R_NULL_ARGUMENT); return NULL; } - if (ctx->newChain == NULL) - return sk_X509_new_null(); return X509_chain_up_ref(ctx->newChain); } @@ -477,10 +475,9 @@ int ossl_cmp_ctx_set1_newChain(OSSL_CMP_CTX *ctx, STACK_OF(X509) *newChain) return 0; sk_X509_pop_free(ctx->newChain, X509_free); - ctx->newChain= NULL; - if (newChain == NULL) - return 1; - return (ctx->newChain = X509_chain_up_ref(newChain)) != NULL; + ctx->newChain = NULL; + return newChain == NULL || + (ctx->newChain = X509_chain_up_ref(newChain)) != NULL; } /* Returns the stack of extraCerts received in CertRepMessage, NULL on error */ @@ -490,8 +487,6 @@ STACK_OF(X509) *OSSL_CMP_CTX_get1_extraCertsIn(const OSSL_CMP_CTX *ctx) ERR_raise(ERR_LIB_CMP, CMP_R_NULL_ARGUMENT); return NULL; } - if (ctx->extraCertsIn == NULL) - return sk_X509_new_null(); return X509_chain_up_ref(ctx->extraCertsIn); } @@ -507,9 +502,8 @@ int ossl_cmp_ctx_set1_extraCertsIn(OSSL_CMP_CTX *ctx, sk_X509_pop_free(ctx->extraCertsIn, X509_free); ctx->extraCertsIn = NULL; - if (extraCertsIn == NULL) - return 1; - return (ctx->extraCertsIn = X509_chain_up_ref(extraCertsIn)) != NULL; + return extraCertsIn == NULL + || (ctx->extraCertsIn = X509_chain_up_ref(extraCertsIn)) != NULL; } /* @@ -526,9 +520,8 @@ int OSSL_CMP_CTX_set1_extraCertsOut(OSSL_CMP_CTX *ctx, sk_X509_pop_free(ctx->extraCertsOut, X509_free); ctx->extraCertsOut = NULL; - if (extraCertsOut == NULL) - return 1; - return (ctx->extraCertsOut = X509_chain_up_ref(extraCertsOut)) != NULL; + return extraCertsOut == NULL + || (ctx->extraCertsOut = X509_chain_up_ref(extraCertsOut)) != NULL; } /* @@ -580,8 +573,6 @@ STACK_OF(X509) *OSSL_CMP_CTX_get1_caPubs(const OSSL_CMP_CTX *ctx) ERR_raise(ERR_LIB_CMP, CMP_R_NULL_ARGUMENT); return NULL; } - if (ctx->caPubs == NULL) - return sk_X509_new_null(); return X509_chain_up_ref(ctx->caPubs); } @@ -596,9 +587,7 @@ int ossl_cmp_ctx_set1_caPubs(OSSL_CMP_CTX *ctx, STACK_OF(X509) *caPubs) sk_X509_pop_free(ctx->caPubs, X509_free); ctx->caPubs = NULL; - if (caPubs == NULL) - return 1; - return (ctx->caPubs = X509_chain_up_ref(caPubs)) != NULL; + return caPubs == NULL || (ctx->caPubs = X509_chain_up_ref(caPubs)) != NULL; } #define char_dup OPENSSL_strdup diff --git a/crypto/ocsp/ocsp_vfy.c b/crypto/ocsp/ocsp_vfy.c index f49f651007..56b9261640 100644 --- a/crypto/ocsp/ocsp_vfy.c +++ b/crypto/ocsp/ocsp_vfy.c @@ -113,10 +113,9 @@ int OCSP_basic_verify(OCSP_BASICRESP *bs, STACK_OF(X509) *certs, goto end; if ((flags & OCSP_NOVERIFY) == 0) { ret = -1; - if ((flags & OCSP_NOCHAIN) != 0) { - untrusted = NULL; - } else if (bs->certs != NULL && certs != NULL) { - untrusted = sk_X509_dup(bs->certs); + if ((flags & OCSP_NOCHAIN) == 0) { + if ((untrusted = sk_X509_dup(bs->certs)) == NULL) + goto end; if (!X509_add_certs(untrusted, certs, X509_ADD_FLAG_DEFAULT)) goto end; } else if (certs != NULL) { @@ -159,8 +158,7 @@ int OCSP_basic_verify(OCSP_BASICRESP *bs, STACK_OF(X509) *certs, end: sk_X509_pop_free(chain, X509_free); - if (bs->certs && certs) - sk_X509_free(untrusted); + sk_X509_free(untrusted); return ret; } diff --git a/crypto/stack/stack.c b/crypto/stack/stack.c index e38efad022..c50a55da14 100644 --- a/crypto/stack/stack.c +++ b/crypto/stack/stack.c @@ -45,26 +45,33 @@ OPENSSL_STACK *OPENSSL_sk_dup(const OPENSSL_STACK *sk) { OPENSSL_STACK *ret; - if ((ret = OPENSSL_malloc(sizeof(*ret))) == NULL) { - ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE); - return NULL; + if ((ret = OPENSSL_malloc(sizeof(*ret))) == NULL) + goto err; + + if (sk == NULL) { + ret->num = 0; + ret->sorted = 0; + ret->comp = NULL; + } else { + /* direct structure assignment */ + *ret = *sk; } - /* direct structure assignment */ - *ret = *sk; - - if (sk->num == 0) { + if (sk == NULL || sk->num == 0) { /* postpone |ret->data| allocation */ ret->data = NULL; ret->num_alloc = 0; return ret; } + /* duplicate |sk->data| content */ if ((ret->data = OPENSSL_malloc(sizeof(*ret->data) * sk->num_alloc)) == NULL) goto err; memcpy(ret->data, sk->data, sizeof(void *) * sk->num); return ret; + err: + ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE); OPENSSL_sk_free(ret); return NULL; } @@ -76,15 +83,19 @@ OPENSSL_STACK *OPENSSL_sk_deep_copy(const OPENSSL_STACK *sk, OPENSSL_STACK *ret; int i; - if ((ret = OPENSSL_malloc(sizeof(*ret))) == NULL) { - ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE); - return NULL; + if ((ret = OPENSSL_malloc(sizeof(*ret))) == NULL) + goto err; + + if (sk == NULL) { + ret->num = 0; + ret->sorted = 0; + ret->comp = NULL; + } else { + /* direct structure assignment */ + *ret = *sk; } - /* direct structure assignment */ - *ret = *sk; - - if (sk->num == 0) { + if (sk == NULL || sk->num == 0) { /* postpone |ret| data allocation */ ret->data = NULL; ret->num_alloc = 0; @@ -93,10 +104,8 @@ OPENSSL_STACK *OPENSSL_sk_deep_copy(const OPENSSL_STACK *sk, ret->num_alloc = sk->num > min_nodes ? sk->num : min_nodes; ret->data = OPENSSL_zalloc(sizeof(*ret->data) * ret->num_alloc); - if (ret->data == NULL) { - OPENSSL_free(ret); - return NULL; - } + if (ret->data == NULL) + goto err; for (i = 0; i < ret->num; ++i) { if (sk->data[i] == NULL) @@ -105,11 +114,15 @@ OPENSSL_STACK *OPENSSL_sk_deep_copy(const OPENSSL_STACK *sk, while (--i >= 0) if (ret->data[i] != NULL) free_func((void *)ret->data[i]); - OPENSSL_sk_free(ret); - return NULL; + goto err; } } return ret; + + err: + ERR_raise(ERR_LIB_CRYPTO, ERR_R_MALLOC_FAILURE); + OPENSSL_sk_free(ret); + return NULL; } OPENSSL_STACK *OPENSSL_sk_new_null(void) diff --git a/crypto/ts/ts_rsp_sign.c b/crypto/ts/ts_rsp_sign.c index 9ae584ff12..17024ea7bb 100644 --- a/crypto/ts/ts_rsp_sign.c +++ b/crypto/ts/ts_rsp_sign.c @@ -183,17 +183,10 @@ int TS_RESP_CTX_set_def_policy(TS_RESP_CTX *ctx, const ASN1_OBJECT *def_policy) int TS_RESP_CTX_set_certs(TS_RESP_CTX *ctx, STACK_OF(X509) *certs) { - sk_X509_pop_free(ctx->certs, X509_free); ctx->certs = NULL; - if (!certs) - return 1; - if ((ctx->certs = X509_chain_up_ref(certs)) == NULL) { - ERR_raise(ERR_LIB_TS, ERR_R_MALLOC_FAILURE); - return 0; - } - return 1; + return certs == NULL || (ctx->certs = X509_chain_up_ref(certs)) != NULL; } int TS_RESP_CTX_add_policy(TS_RESP_CTX *ctx, const ASN1_OBJECT *policy) diff --git a/crypto/x509/x509_cmp.c b/crypto/x509/x509_cmp.c index 1192527125..8e525a3815 100644 --- a/crypto/x509/x509_cmp.c +++ b/crypto/x509/x509_cmp.c @@ -531,6 +531,7 @@ int X509_CRL_check_suiteb(X509_CRL *crl, EVP_PKEY *pk, unsigned long flags) } #endif + /* * Not strictly speaking an "up_ref" as a STACK doesn't have a reference * count but it has the same effect by duping the STACK and upping the ref of @@ -538,17 +539,19 @@ int X509_CRL_check_suiteb(X509_CRL *crl, EVP_PKEY *pk, unsigned long flags) */ STACK_OF(X509) *X509_chain_up_ref(STACK_OF(X509) *chain) { - STACK_OF(X509) *ret; + STACK_OF(X509) *ret = sk_X509_dup(chain); int i; - ret = sk_X509_dup(chain); + if (ret == NULL) return NULL; for (i = 0; i < sk_X509_num(ret); i++) { X509 *x = sk_X509_value(ret, i); + if (!X509_up_ref(x)) goto err; } return ret; + err: while (i-- > 0) X509_free(sk_X509_value(ret, i)); diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 29ccc0ecb1..8e78c13b8e 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -3004,7 +3004,7 @@ static int build_chain(X509_STORE_CTX *ctx) * typically the content of the peer's certificate message) so can make * multiple passes over it, while free to remove elements as we go. */ - if (ctx->untrusted && (sktmp = sk_X509_dup(ctx->untrusted)) == NULL) { + if ((sktmp = sk_X509_dup(ctx->untrusted)) == NULL) { ERR_raise(ERR_LIB_X509, ERR_R_MALLOC_FAILURE); ctx->error = X509_V_ERR_OUT_OF_MEM; return 0; diff --git a/doc/man3/DEFINE_STACK_OF.pod b/doc/man3/DEFINE_STACK_OF.pod index 9088dc040b..b5908fead5 100644 --- a/doc/man3/DEFINE_STACK_OF.pod +++ b/doc/man3/DEFINE_STACK_OF.pod @@ -182,12 +182,14 @@ B_sort>() sorts I using the supplied comparison function. B_is_sorted>() returns B<1> if I is sorted and B<0> otherwise. -B_dup>() returns a copy of I. Note the pointers in the copy -are identical to the original. +B_dup>() returns a shallow copy of I +or an empty stack if the passed stack is NULL. +Note the pointers in the copy are identical to the original. B_deep_copy>() returns a new stack where each element has been -copied. Copying is performed by the supplied copyfunc() and freeing by -freefunc(). The function freefunc() is only called if an error occurs. +copied or an empty stack if the passed stack is NULL. +Copying is performed by the supplied copyfunc() and freeing by freefunc(). +The function freefunc() is only called if an error occurs. =head1 NOTES @@ -258,7 +260,7 @@ B_is_sorted>() returns B<1> if the stack is sorted and B<0> if it is not. B_dup>() and B_deep_copy>() return a pointer to the copy -of the stack. +of the stack or NULL on error. =head1 HISTORY diff --git a/doc/man3/X509_new.pod b/doc/man3/X509_new.pod index b40715bddf..ab310bff57 100644 --- a/doc/man3/X509_new.pod +++ b/doc/man3/X509_new.pod @@ -2,9 +2,9 @@ =head1 NAME -X509_chain_up_ref, X509_new, X509_new_ex, -X509_free, X509_up_ref - X509 certificate ASN1 allocation functions +X509_free, X509_up_ref, +X509_chain_up_ref - X509 certificate ASN1 allocation functions =head1 SYNOPSIS @@ -37,7 +37,7 @@ frees it up if the reference count is zero. If B is NULL nothing is done. X509_up_ref() increments the reference count of B. X509_chain_up_ref() increases the reference count of all certificates in -chain B and returns a copy of the stack. +chain B and returns a copy of the stack, or an empty stack if B is NULL. =head1 NOTES @@ -46,20 +46,19 @@ used by several different operations each of which will free it up after use: this avoids the need to duplicate the entire certificate structure. The function X509_chain_up_ref() doesn't just up the reference count of -each certificate it also returns a copy of the stack, using sk_X509_dup(), +each certificate. It also returns a copy of the stack, using sk_X509_dup(), but it serves a similar purpose: the returned chain persists after the original has been freed. =head1 RETURN VALUES -If the allocation fails, X509_new() returns B and sets an error +If the allocation fails, X509_new() returns NULL and sets an error code that can be obtained by L. Otherwise it returns a pointer to the newly allocated structure. X509_up_ref() returns 1 for success and 0 for failure. -X509_chain_up_ref() returns a copy of the stack or B if an error -occurred. +X509_chain_up_ref() returns a copy of the stack or NULL if an error occurred. =head1 SEE ALSO diff --git a/test/stack_test.c b/test/stack_test.c index 0c1648da77..e59acd353b 100644 --- a/test/stack_test.c +++ b/test/stack_test.c @@ -195,6 +195,10 @@ static int test_uchar_stack(int reserve) goto end; /* dup */ + r = sk_uchar_dup(NULL); + if (sk_uchar_num(r) != 0) + goto end; + sk_uchar_free(r); r = sk_uchar_dup(s); if (!TEST_int_eq(sk_uchar_num(r), n)) goto end; @@ -291,6 +295,10 @@ static int test_SS_stack(void) goto end; /* deepcopy */ + r = sk_SS_deep_copy(NULL, &SS_copy, &SS_free); + if (sk_SS_num(r) != 0) + goto end; + sk_SS_free(r); r = sk_SS_deep_copy(s, &SS_copy, &SS_free); if (!TEST_ptr(r)) goto end;