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 #14040)
openssl · Feb 4, 2021 · d53b437 · d53b437
1 parent b91a13f
commit d53b437
Show file tree

Hide file tree

Showing 9 changed files with 73 additions and 68 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -531,24 +531,27 @@ 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
  * each X509 structure.
  */
 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
@@ -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
@@ -182,12 +182,14 @@ B<sk_I<TYPE>_sort>() sorts I<sk> using the supplied comparison function.
 
 B<sk_I<TYPE>_is_sorted>() returns B<1> if I<sk> is sorted and B<0> otherwise.
 
-B<sk_I<TYPE>_dup>() returns a copy of I<sk>. Note the pointers in the copy
-are identical to the original.
+B<sk_I<TYPE>_dup>() returns a shallow copy of I<sk>
+or an empty stack if the passed stack is NULL.
+Note the pointers in the copy are identical to the original.
 
 B<sk_I<TYPE>_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<sk_I<TYPE>_is_sorted>() returns B<1> if the stack is sorted and B<0> if it is
 not.
 
 B<sk_I<TYPE>_dup>() and B<sk_I<TYPE>_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
@@ -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<a> is NULL nothing is done.
 X509_up_ref() increments the reference count of B<a>.
 
 X509_chain_up_ref() increases the reference count of all certificates in
-chain B<x> and returns a copy of the stack.
+chain B<x> and returns a copy of the stack, or an empty stack if B<a> 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<NULL> and sets an error
+If the allocation fails, X509_new() returns NULL and sets an error
 code that can be obtained by L<ERR_get_error(3)>.
 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<NULL> 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
@@ -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;