From 167f1760ddfeaea1ee1a671ca88aafcccfe30ee0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 12 Jun 2022 13:47:08 -0400 Subject: [PATCH] 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 Commit-Queue: Adam Langley --- crypto/bio/connect.c | 12 +++++-- crypto/lhash/internal.h | 14 ++++++++- crypto/stack/stack.c | 49 +++++++++++++++++++---------- include/openssl/stack.h | 69 +++++++++++++++++++++++++++-------------- 4 files changed, 101 insertions(+), 43 deletions(-) diff --git a/crypto/bio/connect.c b/crypto/bio/connect.c index 3b65acfca..9b86e5138 100644 --- a/crypto/bio/connect.c +++ b/crypto/bio/connect.c @@ -117,7 +117,8 @@ static int closesocket(int sock) { // split_host_and_port sets |*out_host| and |*out_port| to the host and port // parsed from |name|. It returns one on success or zero on error. Even when // successful, |*out_port| may be NULL on return if no port was specified. -static int split_host_and_port(char **out_host, char **out_port, const char *name) { +static int split_host_and_port(char **out_host, char **out_port, + const char *name) { const char *host, *port = NULL; size_t host_len = 0; @@ -466,8 +467,7 @@ static long conn_ctrl(BIO *bio, int cmd, long num, void *ptr) { case BIO_CTRL_FLUSH: break; case BIO_CTRL_GET_CALLBACK: { - int (**fptr)(const BIO *bio, int state, int xret); - fptr = (int (**)(const BIO *bio, int state, int xret))ptr; + int (**fptr)(const BIO *bio, int state, int xret) = ptr; *fptr = data->info_callback; } break; default: @@ -485,7 +485,13 @@ static long conn_callback_ctrl(BIO *bio, int cmd, bio_info_cb fp) { switch (cmd) { case BIO_CTRL_SET_CALLBACK: + // This is the actual type signature of |fp|. The caller is expected to + // cast it to |bio_info_cb| due to the |BIO_callback_ctrl| calling + // convention. + OPENSSL_MSVC_PRAGMA(warning(push)) + OPENSSL_MSVC_PRAGMA(warning(disable : 4191)) data->info_callback = (int (*)(const struct bio_st *, int, int))fp; + OPENSSL_MSVC_PRAGMA(warning(pop)) break; default: ret = 0; diff --git a/crypto/lhash/internal.h b/crypto/lhash/internal.h index 64dca1d36..512f06df4 100644 --- a/crypto/lhash/internal.h +++ b/crypto/lhash/internal.h @@ -157,6 +157,16 @@ OPENSSL_EXPORT void OPENSSL_lh_doall_arg(_LHASH *lh, void *arg); #define DEFINE_LHASH_OF(type) \ + /* 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 |lhash_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_LHASH_OF(type) \ \ typedef int (*lhash_##type##_cmp_func)(const type *, const type *); \ @@ -243,7 +253,9 @@ OPENSSL_EXPORT void OPENSSL_lh_doall_arg(_LHASH *lh, LHASH_OF(type) *lh, void (*func)(type *, void *), void *arg) { \ LHASH_DOALL_##type cb = {func, arg}; \ OPENSSL_lh_doall_arg((_LHASH *)lh, lh_##type##_call_doall_arg, &cb); \ - } + } \ + \ + OPENSSL_MSVC_PRAGMA(warning(pop)) #if defined(__cplusplus) diff --git a/crypto/stack/stack.c b/crypto/stack/stack.c index 6da6e3b23..6412e07bd 100644 --- a/crypto/stack/stack.c +++ b/crypto/stack/stack.c @@ -131,7 +131,7 @@ void sk_free(_STACK *sk) { OPENSSL_free(sk); } -void sk_pop_free_ex(_STACK *sk, void (*call_free_func)(stack_free_func, void *), +void sk_pop_free_ex(_STACK *sk, stack_call_free_func call_free_func, stack_free_func free_func) { if (sk == NULL) { return; @@ -234,8 +234,7 @@ void *sk_delete_ptr(_STACK *sk, const void *p) { } 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) { if (sk == NULL) { return 0; } @@ -355,24 +354,43 @@ err: return NULL; } -void sk_sort(_STACK *sk) { +#if defined(_MSC_VER) +struct sort_compare_ctx { + stack_call_cmp_func call_cmp_func; + stack_cmp_func cmp_func; +}; + +static int sort_compare(void *ctx_v, const void *a, const void *b) { + struct sort_compare_ctx *ctx = ctx_v; + return ctx->call_cmp_func(ctx->cmp_func, a, b); +} +#endif + +void sk_sort(_STACK *sk, stack_call_cmp_func call_cmp_func) { if (sk == NULL || sk->comp == NULL || sk->sorted) { return; } - // sk->comp is a function that takes pointers to pointers to elements, but - // qsort take a comparison function that just takes pointers to elements. - // However, since we're passing an array of pointers to qsort, we can just - // cast the comparison function and everything works. - // - // TODO(davidben): This is undefined behavior, but the call is in libc so, - // e.g., CFI does not notice. Unfortunately, |qsort| is missing a void* - // parameter in its callback and |qsort_s| / |qsort_r| are a mess of - // incompatibility. if (sk->num >= 2) { +#if defined(_MSC_VER) + // MSVC's |qsort_s| is different from the C11 one. + // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/qsort-s?view=msvc-170 + struct sort_compare_ctx ctx = {call_cmp_func, sk->comp}; + qsort_s(sk->data, sk->num, sizeof(void *), sort_compare, &ctx); +#else + // sk->comp is a function that takes pointers to pointers to elements, but + // qsort take a comparison function that just takes pointers to elements. + // However, since we're passing an array of pointers to qsort, we can just + // cast the comparison function and everything works. + // + // TODO(davidben): This is undefined behavior, but the call is in libc so, + // e.g., CFI does not notice. |qsort| is missing a void* parameter in its + // callback, while no one defines |qsort_r| or |qsort_s| consistently. See + // https://stackoverflow.com/a/39561369 int (*comp_func)(const void *, const void *) = (int (*)(const void *, const void *))(sk->comp); qsort(sk->data, sk->num, sizeof(void *), comp_func); +#endif } sk->sorted = 1; } @@ -395,10 +413,9 @@ stack_cmp_func sk_set_cmp_func(_STACK *sk, stack_cmp_func comp) { return old; } -_STACK *sk_deep_copy(const _STACK *sk, - void *(*call_copy_func)(stack_copy_func, void *), +_STACK *sk_deep_copy(const _STACK *sk, stack_call_copy_func call_copy_func, stack_copy_func copy_func, - void (*call_free_func)(stack_free_func, void *), + stack_call_free_func call_free_func, stack_free_func free_func) { _STACK *ret = sk_dup(sk); if (ret == NULL) { diff --git a/include/openssl/stack.h b/include/openssl/stack.h index df54713f1..87403678a 100644 --- a/include/openssl/stack.h +++ b/include/openssl/stack.h @@ -101,10 +101,20 @@ typedef void *(*stack_copy_func)(void *ptr); // 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 @@ OPENSSL_EXPORT void sk_free(_STACK *sk); // |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_EXPORT void *sk_delete_ptr(_STACK *sk, const void *p); // 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 @@ OPENSSL_EXPORT _STACK *sk_dup(const _STACK *sk); // 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 @@ OPENSSL_EXPORT stack_cmp_func sk_set_cmp_func(_STACK *sk, stack_cmp_func comp); // 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 @@ BSSL_NAMESPACE_END #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 @@ BSSL_NAMESPACE_END } \ \ 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 @@ BSSL_NAMESPACE_END 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 @@ BSSL_NAMESPACE_END } \ \ 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 @@ BSSL_NAMESPACE_END 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 @@ BSSL_NAMESPACE_END (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| *.