Allow NULL arg to OPENSSL_sk_{dup,deep_copy} returning empty stack

This simplifies many usages

Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14040)

6 files changed, 52 insertions, 56 deletions

diff --git a/crypto/cmp/cmp_ctx.c b/crypto/cmp/cmp_ctx.c
index e1b4e50..ccca282 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 f49f651..56b9261 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 e38efad..c50a55d 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;
 
-    /* direct structure assignment */
-    *ret = *sk;
+    if (sk == NULL) {
+        ret->num = 0;
+        ret->sorted = 0;
+        ret->comp = NULL;
+    } else {
+        /* 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;
 
-    /* direct structure assignment */
-    *ret = *sk;
+    if (sk == NULL) {
+        ret->num = 0;
+        ret->sorted = 0;
+        ret->comp = NULL;
+    } else {
+        /* 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 9ae584f..17024ea 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 1192527..8e525a3 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 29ccc0e..8e78c13 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;