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| *.