From: Nikola Pajkovsky Date: Tue, 12 May 2026 06:49:31 +0000 (+0200) Subject: stack: use a copy thunk for typed stack deep copies X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b1389437f56ace99f2a59a8a78fbf3f69b487d34;p=thirdparty%2Fopenssl.git stack: use a copy thunk for typed stack deep copies typed safestack wrappers pass type-specific copy callbacks such as TYPE *(*)(const TYPE *) to OPENSSL_sk_deep_copy(). The generic stack code then called those callbacks through OPENSSL_sk_copyfunc, void *(*)(const void *), which is an incompatible function pointer type and triggers UBSan. Add an OPENSSL_sk_copyfunc_thunk and store it on typed stacks, mirroring the existing compare/free thunk pattern. Generated safestack helpers now install a per-type copy thunk when constructing a stack, and internal_copy() uses that thunk when deep-copying typed stacks. This preserves the generic stack API while ensuring typed copy callbacks are invoked through their real signature. Fixes: https://github.com/openssl/project/issues/1951 Signed-off-by: Nikola Pajkovsky Reviewed-by: Eugene Syromiatnikov Reviewed-by: Neil Horman MergeDate: Wed May 20 15:53:45 2026 (Merged from https://github.com/openssl/openssl/pull/31151) --- diff --git a/crypto/stack/stack.c b/crypto/stack/stack.c index bd3c6f54ef1..5799bf9d7ca 100644 --- a/crypto/stack/stack.c +++ b/crypto/stack/stack.c @@ -33,6 +33,7 @@ struct stack_st { OPENSSL_sk_compfunc comp; int (*cmp_thunk)(OPENSSL_sk_compfunc, const void *, const void *); OPENSSL_sk_freefunc_thunk free_thunk; + OPENSSL_sk_copyfunc_thunk copy_thunk; }; OPENSSL_sk_compfunc OPENSSL_sk_set_cmp_func(OPENSSL_STACK *sk, @@ -88,7 +89,12 @@ static OPENSSL_STACK *internal_copy(const OPENSSL_STACK *sk, for (i = 0; i < ret->num; ++i) { if (sk->data[i] == NULL) continue; - if ((ret->data[i] = copy_func(sk->data[i])) == NULL) { + if (ret->copy_thunk != NULL) + ret->data[i] = ret->copy_thunk(copy_func, sk->data[i]); + else + ret->data[i] = copy_func(sk->data[i]); + + if (ret->data[i] == NULL) { while (--i >= 0) free_with_thunk(ret, free_func, ret->data[i]); goto err; @@ -266,6 +272,14 @@ OPENSSL_STACK *OPENSSL_sk_set_cmp_thunks(OPENSSL_STACK *st, int (*c_thunk)(int ( return st; } +OPENSSL_STACK *OPENSSL_sk_set_copy_thunks(OPENSSL_STACK *st, OPENSSL_sk_copyfunc_thunk cp_thunk) +{ + if (st != NULL) + st->copy_thunk = cp_thunk; + + return st; +} + int OPENSSL_sk_insert(OPENSSL_STACK *st, const void *data, int loc) { int cmp_ret; diff --git a/doc/man3/DEFINE_STACK_OF.pod b/doc/man3/DEFINE_STACK_OF.pod index a9d678baccc..5f4fcaa487d 100644 --- a/doc/man3/DEFINE_STACK_OF.pod +++ b/doc/man3/DEFINE_STACK_OF.pod @@ -16,7 +16,8 @@ OPENSSL_sk_dup, OPENSSL_sk_find, OPENSSL_sk_find_ex, OPENSSL_sk_find_all, OPENSSL_sk_free, OPENSSL_sk_insert, OPENSSL_sk_is_sorted, OPENSSL_sk_new, OPENSSL_sk_new_null, OPENSSL_sk_new_reserve, OPENSSL_sk_num, OPENSSL_sk_pop, OPENSSL_sk_pop_free, OPENSSL_sk_push, OPENSSL_sk_reserve, OPENSSL_sk_set, -OPENSSL_sk_set_thunks, OPENSSL_sk_set_cmp_thunks, OPENSSL_sk_set_cmp_func, OPENSSL_sk_shift, +OPENSSL_sk_set_thunks, OPENSSL_sk_set_cmp_thunks, OPENSSL_sk_set_copy_thunks, +OPENSSL_sk_set_cmp_func, OPENSSL_sk_shift, OPENSSL_sk_sort, OPENSSL_sk_unshift, OPENSSL_sk_value, OPENSSL_sk_zero - stack container @@ -240,11 +241,13 @@ OPENSSL_sk_free(), OPENSSL_sk_insert(), OPENSSL_sk_is_sorted(), OPENSSL_sk_new(), OPENSSL_sk_new_null(), OPENSSL_sk_new_reserve(), OPENSSL_sk_num(), OPENSSL_sk_pop(), OPENSSL_sk_pop_free(), OPENSSL_sk_push(), OPENSSL_sk_reserve(), OPENSSL_sk_set(), OPENSSL_sk_set_cmp_func(), -OPENSSL_sk_set_thunks(), OPENSSL_sk_set_cmp_thunks(), OPENSSL_sk_shift(), OPENSSL_sk_sort(), +OPENSSL_sk_set_thunks(), OPENSSL_sk_set_cmp_thunks(), +OPENSSL_sk_set_copy_thunks(), OPENSSL_sk_shift(), OPENSSL_sk_sort(), OPENSSL_sk_unshift(), OPENSSL_sk_value(), OPENSSL_sk_zero(). -OPENSSL_sk_set_thunks() and OPENSSL_sk_set_cmp_thunks(), while public by necessity, are actually -internal and should not be used. +OPENSSL_sk_set_thunks(), OPENSSL_sk_set_cmp_thunks(), and +OPENSSL_sk_set_copy_thunks(), while public by necessity, are actually internal +and should not be used. =head1 RETURN VALUES @@ -303,7 +306,9 @@ was changed to return 0 in this condition as for other errors. OPENSSL_sk_set_thunks() was added in OpenSSL 3.6.0. -OPENSSL_sk_set_cmp_thunks() was added in OpenSSL 4.0.0 +OPENSSL_sk_set_cmp_thunks() was added in OpenSSL 4.0.0. + +OPENSSL_sk_set_copy_thunks() was added in OpenSSL 4.1.0. =head1 COPYRIGHT diff --git a/include/openssl/safestack.h.in b/include/openssl/safestack.h.in index 67e0448252a..a4c178d1897 100644 --- a/include/openssl/safestack.h.in +++ b/include/openssl/safestack.h.in @@ -46,6 +46,11 @@ extern "C" { sk_##t1##_freefunc freefunc = (sk_##t1##_freefunc)freefunc_arg; \ freefunc((t3 *)ptr); \ } \ + static ossl_inline void *sk_##t1##_copyfunc_thunk(OPENSSL_sk_copyfunc copyfunc_arg, const void *ptr) \ + { \ + sk_##t1##_copyfunc copyfunc = (sk_##t1##_copyfunc)copyfunc_arg; \ + return (void *)copyfunc((const t3 *)ptr); \ + } \ static ossl_inline int sk_##t1##_cmpfunc_thunk(int (*cmp)(const void *, const void *), const void *a, const void *b) \ { \ int (*realcmp)(const t3 *const *a, const t3 *const *b) = (int (*)(const t3 *const *a, const t3 *const *b))(cmp); \ @@ -89,6 +94,11 @@ extern "C" { sk_##t1##_freefunc freefunc = (sk_##t1##_freefunc)freefunc_arg; \ freefunc((t3 *)ptr); \ } \ + static ossl_inline void *sk_##t1##_copyfunc_thunk(OPENSSL_sk_copyfunc copyfunc_arg, const void *ptr) \ + { \ + sk_##t1##_copyfunc copyfunc = (sk_##t1##_copyfunc)copyfunc_arg; \ + return (void *)copyfunc((const t3 *)ptr); \ + } \ static ossl_inline int sk_##t1##_cmpfunc_thunk(int (*cmp)(const void *, const void *), const void *a, const void *b) \ { \ int (*realcmp)(const t3 *const *a, const t3 *const *b) = (int (*)(const t3 *const *a, const t3 *const *b))(cmp); \ @@ -112,6 +122,7 @@ extern "C" { \ f_thunk = (OPENSSL_sk_freefunc_thunk)sk_##t1##_freefunc_thunk; \ OPENSSL_sk_set_cmp_thunks(ret, sk_##t1##_cmpfunc_thunk); \ + OPENSSL_sk_set_copy_thunks(ret, sk_##t1##_copyfunc_thunk); \ return (STACK_OF(t1) *)OPENSSL_sk_set_thunks(ret, f_thunk); \ } \ static ossl_unused ossl_inline STACK_OF(t1) *sk_##t1##_new_null(void) \ @@ -121,6 +132,7 @@ extern "C" { \ f_thunk = (OPENSSL_sk_freefunc_thunk)sk_##t1##_freefunc_thunk; \ OPENSSL_sk_set_cmp_thunks(ret, sk_##t1##_cmpfunc_thunk); \ + OPENSSL_sk_set_copy_thunks(ret, sk_##t1##_copyfunc_thunk); \ return (STACK_OF(t1) *)OPENSSL_sk_set_thunks(ret, f_thunk); \ } \ static ossl_unused ossl_inline STACK_OF(t1) *sk_##t1##_new_reserve(sk_##t1##_compfunc compare, int n) \ @@ -130,6 +142,7 @@ extern "C" { \ f_thunk = (OPENSSL_sk_freefunc_thunk)sk_##t1##_freefunc_thunk; \ OPENSSL_sk_set_cmp_thunks(ret, sk_##t1##_cmpfunc_thunk); \ + OPENSSL_sk_set_copy_thunks(ret, sk_##t1##_copyfunc_thunk); \ return (STACK_OF(t1) *)OPENSSL_sk_set_thunks(ret, f_thunk); \ } \ static ossl_unused ossl_inline int sk_##t1##_reserve(STACK_OF(t1) *sk, int n) \ diff --git a/include/openssl/stack.h b/include/openssl/stack.h index 67e4d258972..7cab36c4f64 100644 --- a/include/openssl/stack.h +++ b/include/openssl/stack.h @@ -26,6 +26,7 @@ typedef int (*OPENSSL_sk_compfunc)(const void *, const void *); typedef void (*OPENSSL_sk_freefunc)(void *); typedef void (*OPENSSL_sk_freefunc_thunk)(OPENSSL_sk_freefunc, void *); typedef void *(*OPENSSL_sk_copyfunc)(const void *); +typedef void *(*OPENSSL_sk_copyfunc_thunk)(OPENSSL_sk_copyfunc, const void *); int OPENSSL_sk_num(const OPENSSL_STACK *); void *OPENSSL_sk_value(const OPENSSL_STACK *, int); @@ -37,6 +38,7 @@ OPENSSL_STACK *OPENSSL_sk_new_null(void); OPENSSL_STACK *OPENSSL_sk_new_reserve(OPENSSL_sk_compfunc c, int n); OPENSSL_STACK *OPENSSL_sk_set_thunks(OPENSSL_STACK *st, OPENSSL_sk_freefunc_thunk f_thunk); OPENSSL_STACK *OPENSSL_sk_set_cmp_thunks(OPENSSL_STACK *st, int (*c_thunk)(int (*)(const void *, const void *), const void *, const void *)); +OPENSSL_STACK *OPENSSL_sk_set_copy_thunks(OPENSSL_STACK *st, OPENSSL_sk_copyfunc_thunk cp_thunk); int OPENSSL_sk_reserve(OPENSSL_STACK *st, int n); void OPENSSL_sk_free(OPENSSL_STACK *); void OPENSSL_sk_pop_free(OPENSSL_STACK *st, OPENSSL_sk_freefunc func); diff --git a/test/stack_test.c b/test/stack_test.c index aa29e6c822f..c74cca21677 100644 --- a/test/stack_test.c +++ b/test/stack_test.c @@ -1,5 +1,5 @@ /* - * Copyright 2017-2025 The OpenSSL Project Authors. All Rights Reserved. + * Copyright 2017-2026 The OpenSSL Project Authors. All Rights Reserved. * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved. * * Licensed under the Apache License 2.0 (the "License"). You may not use @@ -339,6 +339,16 @@ static void SS_free(SS *p) OPENSSL_free(p); } +static char *string_copy(const char *p) +{ + return OPENSSL_strdup(p); +} + +static void string_free(char *p) +{ + OPENSSL_free(p); +} + static int test_SS_stack(void) { STACK_OF(SS) *s = sk_SS_new_null(); @@ -413,6 +423,55 @@ end: return testresult; } +static int test_OPENSSL_STRING_deep_copy_mfail(void) +{ + STACK_OF(OPENSSL_STRING) *s = sk_OPENSSL_STRING_new_null(); + STACK_OF(OPENSSL_STRING) *r = NULL; + static const char *strings[] = { + "alpha", "beta", "gamma" + }; + char *p; + int i; + int testresult = 0; + + if (!TEST_ptr(s)) + goto end; + + for (i = 0; i < (int)OSSL_NELEM(strings); i++) { + p = OPENSSL_strdup(strings[i]); + if (!TEST_ptr(p) + || !TEST_int_eq(sk_OPENSSL_STRING_push(s, p), i + 1)) { + OPENSSL_free(p); + goto end; + } + } + + MFAIL_start(); + r = sk_OPENSSL_STRING_deep_copy(s, string_copy, string_free); + MFAIL_end(); + + if (r == NULL) + goto end; + + if (!TEST_int_eq(sk_OPENSSL_STRING_num(r), sk_OPENSSL_STRING_num(s))) + goto end; + + for (i = 0; i < sk_OPENSSL_STRING_num(s); i++) { + char *src = sk_OPENSSL_STRING_value(s, i); + char *dst = sk_OPENSSL_STRING_value(r, i); + + if (!TEST_ptr_ne(dst, src) + || !TEST_str_eq(dst, src)) + goto end; + } + + testresult = 1; +end: + sk_OPENSSL_STRING_pop_free(r, string_free); + sk_OPENSSL_STRING_pop_free(s, string_free); + return testresult; +} + static int test_SU_stack(void) { STACK_OF(SU) *s = sk_SU_new_null(); @@ -455,5 +514,6 @@ int setup_tests(void) ADD_ALL_TESTS(test_uchar_stack, 4); ADD_TEST(test_SS_stack); ADD_TEST(test_SU_stack); + ADD_MFAIL_TEST(test_OPENSSL_STRING_deep_copy_mfail); return 1; } diff --git a/util/libcrypto.num b/util/libcrypto.num index dbaac8b37fa..659eb5cbf45 100644 --- a/util/libcrypto.num +++ b/util/libcrypto.num @@ -5720,3 +5720,4 @@ CRYPTO_atomic_load_ptr ? 4_1_0 EXIST::FUNCTION: CRYPTO_atomic_store_ptr ? 4_1_0 EXIST::FUNCTION: CRYPTO_atomic_cmp_exch_ptr ? 4_1_0 EXIST::FUNCTION: EVP_EC_affine2oct ? 4_1_0 EXIST::FUNCTION: +OPENSSL_sk_set_copy_thunks ? 4_1_0 EXIST::FUNCTION: diff --git a/util/perl/OpenSSL/stackhash.pm b/util/perl/OpenSSL/stackhash.pm index cab5738d187..39f71e1ce71 100644 --- a/util/perl/OpenSSL/stackhash.pm +++ b/util/perl/OpenSSL/stackhash.pm @@ -28,9 +28,9 @@ sub generate_stack_macros_int { SKM_DEFINE_STACK_OF_INTERNAL(${nametype}, ${realtype}, ${plaintype}) #define sk_${nametype}_num(sk) OPENSSL_sk_num(ossl_check_const_${nametype}_sk_type(sk)) #define sk_${nametype}_value(sk, idx) ((${realtype} *)OPENSSL_sk_value(ossl_check_const_${nametype}_sk_type(sk), (idx))) -#define sk_${nametype}_new(cmp) ((STACK_OF(${nametype}) *)OPENSSL_sk_set_cmp_thunks(OPENSSL_sk_new(ossl_check_${nametype}_compfunc_type(cmp)), sk_${nametype}_cmpfunc_thunk)) -#define sk_${nametype}_new_null() ((STACK_OF(${nametype}) *)OPENSSL_sk_set_thunks(OPENSSL_sk_new_null(), sk_${nametype}_freefunc_thunk)) -#define sk_${nametype}_new_reserve(cmp, n) ((STACK_OF(${nametype}) *)OPENSSL_sk_set_cmp_thunks(OPENSSL_sk_new_reserve(ossl_check_${nametype}_compfunc_type(cmp), (n)), sk_${nametype}_cmpfunc_thunk)) +#define sk_${nametype}_new(cmp) ((STACK_OF(${nametype}) *)OPENSSL_sk_set_copy_thunks(OPENSSL_sk_set_cmp_thunks(OPENSSL_sk_new(ossl_check_${nametype}_compfunc_type(cmp)), sk_${nametype}_cmpfunc_thunk), sk_${nametype}_copyfunc_thunk)) +#define sk_${nametype}_new_null() ((STACK_OF(${nametype}) *)OPENSSL_sk_set_thunks(OPENSSL_sk_set_copy_thunks(OPENSSL_sk_new_null(), sk_${nametype}_copyfunc_thunk), sk_${nametype}_freefunc_thunk)) +#define sk_${nametype}_new_reserve(cmp, n) ((STACK_OF(${nametype}) *)OPENSSL_sk_set_copy_thunks(OPENSSL_sk_set_cmp_thunks(OPENSSL_sk_new_reserve(ossl_check_${nametype}_compfunc_type(cmp), (n)), sk_${nametype}_cmpfunc_thunk), sk_${nametype}_copyfunc_thunk)) #define sk_${nametype}_reserve(sk, n) OPENSSL_sk_reserve(ossl_check_${nametype}_sk_type(sk), (n)) #define sk_${nametype}_free(sk) OPENSSL_sk_free(ossl_check_${nametype}_sk_type(sk)) #define sk_${nametype}_zero(sk) OPENSSL_sk_zero(ossl_check_${nametype}_sk_type(sk))