From: tb <tb@openbsd.org>
Date: Thu, 14 Dec 2023 12:02:10 +0000 (+0000)
Subject: Fix sk_deep_copy() implementation
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=35600121c61733172b1d6704accfb487b26f381e;p=openbsd

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 4ba697ead4e..662e3179a69 100644
--- 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;
 		}