Skip to content

Commit

Permalink
Fix build with MSVC 2022.
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@davidben
davidben authored and Boringssl LUCI CQ committed Jun 14, 2022
1 parent f8f97bf commit 167f176
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 43 deletions.
12 changes: 9 additions & 3 deletions crypto/bio/connect.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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:
Expand All @@ -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;
Expand Down
14 changes: 13 additions & 1 deletion crypto/lhash/internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 *); \
Expand Down Expand Up @@ -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)
Expand Down
49 changes: 33 additions & 16 deletions crypto/stack/stack.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -355,24 +354,43 @@ _STACK *sk_dup(const _STACK *sk) {
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;
}
Expand All @@ -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) {
Expand Down
69 changes: 46 additions & 23 deletions include/openssl/stack.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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); \
Expand All @@ -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); \
} \
\
Expand All @@ -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); \
} \
Expand All @@ -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); \
} \
Expand All @@ -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) { \
Expand All @@ -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| *.
Expand Down

0 comments on commit 167f176

Please sign in to comment.