Fix sk_deep_copy() implementation

sk_deep_copy() is bad code. It is less bad than the upstream code, but
still bad: it passes strdup() through a void pointer and assigns it to
a function pointer of different type before calling the latter. That's
not kosher in more than one way.

There is no need for such gymnastics. If we need a deep copy for a type,
we should implement it as appropriate for that type.

Also, we should not expect and even less so allow holes in a STACK_OF().
The only way the vpm->hosts can be populated is by way of this deep_copy
function or x509_param_set_hosts_internal(), which pushes only after a
non-NULL check. Invariants: they're useful.

ok jsing

diff --git a/lib/libcrypto/x509/x509_vpm.c b/lib/libcrypto/x509/x509_vpm.c
index 4ba697e..662e317 100644 (file)
--- a/lib/libcrypto/x509/x509_vpm.c
+++ b/lib/libcrypto/x509/x509_vpm.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: x509_vpm.c,v 1.40 2023/05/28 05:25:24 tb Exp $ */
+/* $OpenBSD: x509_vpm.c,v 1.41 2023/12/14 12:02:10 tb Exp $ */
 /* Written by Dr Stephen N Henson (steve@openssl.org) for the OpenSSL
  * project 2004.
  */
@@ -84,41 +84,31 @@ str_free(char *s)
     free(s);
 }
 
-/*
- * Post 1.0.1 sk function "deep_copy".  For the moment we simply make
- * these take void * and use them directly without a glorious blob of
- * obfuscating macros of dubious value in front of them. All this in
- * preparation for a rototilling of safestack.h (likely inspired by
- * this).
- */
-static void *
-sk_deep_copy(void *sk_void, void *copy_func_void, void *free_func_void)
+static STACK_OF(OPENSSL_STRING) *
+sk_OPENSSL_STRING_deep_copy(const STACK_OF(OPENSSL_STRING) *sk)
 {
-       _STACK *sk = sk_void;
-       void *(*copy_func)(void *) = copy_func_void;
-       void (*free_func)(void *) = free_func_void;
-       _STACK *ret = sk_dup(sk);
-       size_t i;
+       STACK_OF(OPENSSL_STRING) *new;
+       char *copy = NULL;
+       int i;
 
-       if (ret == NULL)
-               return NULL;
+       if ((new = sk_OPENSSL_STRING_new_null()) == NULL)
+               goto err;
 
-       for (i = 0; i < ret->num; i++) {
-               if (ret->data[i] == NULL)
-                       continue;
-               ret->data[i] = copy_func(ret->data[i]);
-               if (ret->data[i] == NULL) {
-                       size_t j;
-                       for (j = 0; j < i; j++) {
-                               if (ret->data[j] != NULL)
-                                       free_func(ret->data[j]);
-                       }
-                       sk_free(ret);
-                       return NULL;
-               }
+       for (i = 0; i < sk_OPENSSL_STRING_num(sk); i++) {
+               if ((copy = strdup(sk_OPENSSL_STRING_value(sk, i))) == NULL)
+                       goto err;
+               if (sk_OPENSSL_STRING_push(new, copy) <= 0)
+                       goto err;
+               copy = NULL;
        }
 
-       return ret;
+       return new;
+
+ err:
+       sk_OPENSSL_STRING_pop_free(new, str_free);
+       free(copy);
+
+       return NULL;
 }
 
 static int
@@ -313,7 +303,7 @@ X509_VERIFY_PARAM_inherit(X509_VERIFY_PARAM *dest, const X509_VERIFY_PARAM *src)
                        dest->hosts = NULL;
                }
                if (src->hosts) {
-                       dest->hosts = sk_deep_copy(src->hosts, strdup, str_free);
+                       dest->hosts = sk_OPENSSL_STRING_deep_copy(src->hosts);
                        if (dest->hosts == NULL)
                                return 0;
                }