Fix build with MSVC 2022.
MSVC 2022's C4191 warns on most function pointer casts. Fix and/or
silence them:
connect.c is an unavoidable false positive. We're casting the pointer to
the correct type. The problem was that the caller is required to first
cast it to the wrong type in OpenSSL's API, due to the BIO_callback_ctrl
calling convention. Suppress it.
LHASH_OF(T) and STACK_OF(T)'s defintions also have a false positive.
Suppress that warning. Calling the functions through the casted types
would indeed be UB, but we don't do that. We use them as goofy
type-erased types. The problem is there is no function pointer
equivalent of void*. (Using actual void* instead trips a GCC warning.)
The sk_sort instance is a true instance of UB. The problem is qsort
lacks a context parameter. I've made sk_sort call qsort_s on _MSC_VER,
to silence the warning. Ideally we'd fix it on other platforms, but
qsort_r and qsort_s are a disaster. See
https://stackoverflow.com/a/39561369
Fixed: 495
Change-Id: I0dca80670c74afaa03fc5c8fd7059b4cfadfac72
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53005
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
diff --git a/include/openssl/stack.h b/include/openssl/stack.h
index df54713..8740367 100644
--- a/include/openssl/stack.h
+++ b/include/openssl/stack.h
@@ -101,10 +101,20 @@
// extra indirection - the function is given a pointer to a pointer to the
// element. This differs from the usual qsort/bsearch comparison function.
//
-// Note its actual type is int (*)(const T **, const T **). Low-level |sk_*|
+// Note its actual type is |int (*)(const T **a, const T **b)|. Low-level |sk_*|
// functions will be passed a type-specific wrapper to call it correctly.
+//
+// TODO(davidben): This type should be |const T *const *|. It is already fixed
+// in OpenSSL 1.1.1, so hopefully we can fix this compatibly.
typedef int (*stack_cmp_func)(const void **a, const void **b);
+// The following function types call the above type-erased signatures with the
+// true types.
+typedef void (*stack_call_free_func)(stack_free_func, void *);
+typedef void *(*stack_call_copy_func)(stack_copy_func, void *);
+typedef int (*stack_call_cmp_func)(stack_cmp_func, const void *const *,
+ const void *const *);
+
// stack_st contains an array of pointers. It is not designed to be used
// directly, rather the wrapper macros should be used.
typedef struct stack_st {
@@ -161,8 +171,7 @@
// |sk_pop_free_ex| as a workaround for existing code calling an older version
// of |sk_pop_free|.
OPENSSL_EXPORT void sk_pop_free_ex(_STACK *sk,
- void (*call_free_func)(stack_free_func,
- void *),
+ stack_call_free_func call_free_func,
stack_free_func free_func);
// sk_insert inserts |p| into the stack at index |where|, moving existing
@@ -192,8 +201,7 @@
// OpenSSL's sk_find will implicitly sort |sk| if it has a comparison function
// defined.
OPENSSL_EXPORT int sk_find(const _STACK *sk, size_t *out_index, const void *p,
- int (*call_cmp_func)(stack_cmp_func, const void **,
- const void **));
+ stack_call_cmp_func call_cmp_func);
// sk_shift removes and returns the first element in the stack, or returns NULL
// if the stack is empty.
@@ -214,7 +222,7 @@
// sk_sort sorts the elements of |sk| into ascending order based on the
// comparison function. The stack maintains a |sorted| flag and sorting an
// already sorted stack is a no-op.
-OPENSSL_EXPORT void sk_sort(_STACK *sk);
+OPENSSL_EXPORT void sk_sort(_STACK *sk, stack_call_cmp_func call_cmp_func);
// sk_is_sorted returns one if |sk| is known to be sorted and zero
// otherwise.
@@ -227,10 +235,11 @@
// sk_deep_copy performs a copy of |sk| and of each of the non-NULL elements in
// |sk| by using |copy_func|. If an error occurs, |free_func| is used to free
// any copies already made and NULL is returned.
-OPENSSL_EXPORT _STACK *sk_deep_copy(
- const _STACK *sk, void *(*call_copy_func)(stack_copy_func, void *),
- stack_copy_func copy_func, void (*call_free_func)(stack_free_func, void *),
- stack_free_func free_func);
+OPENSSL_EXPORT _STACK *sk_deep_copy(const _STACK *sk,
+ stack_call_copy_func call_copy_func,
+ stack_copy_func copy_func,
+ stack_call_free_func call_free_func,
+ stack_free_func free_func);
// Deprecated functions.
@@ -277,6 +286,16 @@
#endif
#define BORINGSSL_DEFINE_STACK_OF_IMPL(name, ptrtype, constptrtype) \
+ /* We disable MSVC C4191 in this macro, which warns when pointers are cast \
+ * to the wrong type. While the cast itself is valid, it is often a bug \
+ * because calling it through the cast is UB. However, we never actually \
+ * call functions as |stack_cmp_func|. The type is just a type-erased \
+ * function pointer. (C does not guarantee function pointers fit in \
+ * |void*|, and GCC will warn on this.) Thus we just disable the false \
+ * positive warning. */ \
+ OPENSSL_MSVC_PRAGMA(warning(push)) \
+ OPENSSL_MSVC_PRAGMA(warning(disable : 4191)) \
+ \
DECLARE_STACK_OF(name) \
\
typedef void (*stack_##name##_free_func)(ptrtype); \
@@ -294,14 +313,17 @@
} \
\
OPENSSL_INLINE int sk_##name##_call_cmp_func( \
- stack_cmp_func cmp_func, const void **a, const void **b) { \
+ stack_cmp_func cmp_func, const void *const *a, const void *const *b) { \
+ /* The data is actually stored as |void*| pointers, so read the pointer \
+ * as |void*| and then pass the corrected type into the caller-supplied \
+ * function, which expects |constptrtype*|. */ \
constptrtype a_ptr = (constptrtype)*a; \
constptrtype b_ptr = (constptrtype)*b; \
return ((stack_##name##_cmp_func)cmp_func)(&a_ptr, &b_ptr); \
} \
\
- OPENSSL_INLINE STACK_OF(name) * \
- sk_##name##_new(stack_##name##_cmp_func comp) { \
+ OPENSSL_INLINE STACK_OF(name) *sk_##name##_new( \
+ stack_##name##_cmp_func comp) { \
return (STACK_OF(name) *)sk_new((stack_cmp_func)comp); \
} \
\
@@ -327,12 +349,12 @@
return (ptrtype)sk_set((_STACK *)sk, i, (void *)p); \
} \
\
- OPENSSL_INLINE void sk_##name##_free(STACK_OF(name) * sk) { \
+ OPENSSL_INLINE void sk_##name##_free(STACK_OF(name) *sk) { \
sk_free((_STACK *)sk); \
} \
\
OPENSSL_INLINE void sk_##name##_pop_free( \
- STACK_OF(name) * sk, stack_##name##_free_func free_func) { \
+ STACK_OF(name) *sk, stack_##name##_free_func free_func) { \
sk_pop_free_ex((_STACK *)sk, sk_##name##_call_free_func, \
(stack_free_func)free_func); \
} \
@@ -353,7 +375,7 @@
} \
\
OPENSSL_INLINE int sk_##name##_find(const STACK_OF(name) *sk, \
- size_t * out_index, constptrtype p) { \
+ size_t *out_index, constptrtype p) { \
return sk_find((const _STACK *)sk, out_index, (const void *)p, \
sk_##name##_call_cmp_func); \
} \
@@ -370,12 +392,12 @@
return (ptrtype)sk_pop((_STACK *)sk); \
} \
\
- OPENSSL_INLINE STACK_OF(name) * sk_##name##_dup(const STACK_OF(name) *sk) { \
+ OPENSSL_INLINE STACK_OF(name) *sk_##name##_dup(const STACK_OF(name) *sk) { \
return (STACK_OF(name) *)sk_dup((const _STACK *)sk); \
} \
\
OPENSSL_INLINE void sk_##name##_sort(STACK_OF(name) *sk) { \
- sk_sort((_STACK *)sk); \
+ sk_sort((_STACK *)sk, sk_##name##_call_cmp_func); \
} \
\
OPENSSL_INLINE int sk_##name##_is_sorted(const STACK_OF(name) *sk) { \
@@ -388,15 +410,16 @@
(stack_cmp_func)comp); \
} \
\
- OPENSSL_INLINE STACK_OF(name) * \
- sk_##name##_deep_copy(const STACK_OF(name) *sk, \
- ptrtype(*copy_func)(ptrtype), \
- void (*free_func)(ptrtype)) { \
+ OPENSSL_INLINE STACK_OF(name) *sk_##name##_deep_copy( \
+ const STACK_OF(name) *sk, ptrtype (*copy_func)(ptrtype), \
+ void (*free_func)(ptrtype)) { \
return (STACK_OF(name) *)sk_deep_copy( \
(const _STACK *)sk, sk_##name##_call_copy_func, \
(stack_copy_func)copy_func, sk_##name##_call_free_func, \
(stack_free_func)free_func); \
- }
+ } \
+ \
+ OPENSSL_MSVC_PRAGMA(warning(pop))
// DEFINE_NAMED_STACK_OF defines |STACK_OF(name)| to be a stack whose elements
// are |type| *.
