From 0ebd69bd1e0ae834e01935ad0c5cfac63a5aea32 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 13 Jun 2022 17:47:39 -0400 Subject: [PATCH 1/3] Add BN_GENCB_get_arg. bind uses this function. Change-Id: I97ba86d9f75597bff125ae0b56952effc397e6b8 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/53010 Commit-Queue: David Benjamin Reviewed-by: Bob Beck Commit-Queue: Bob Beck --- crypto/fipsmodule/bn/prime.c | 2 ++ include/openssl/bn.h | 3 +++ 2 files changed, 5 insertions(+) diff --git a/crypto/fipsmodule/bn/prime.c b/crypto/fipsmodule/bn/prime.c index 0c5edfeeba..0578558296 100644 --- a/crypto/fipsmodule/bn/prime.c +++ b/crypto/fipsmodule/bn/prime.c @@ -386,6 +386,8 @@ int BN_GENCB_call(BN_GENCB *callback, int event, int n) { return callback->callback(event, n, callback); } +void *BN_GENCB_get_arg(const BN_GENCB *callback) { return callback->arg; } + int BN_generate_prime_ex(BIGNUM *ret, int bits, int safe, const BIGNUM *add, const BIGNUM *rem, BN_GENCB *cb) { BIGNUM *t; diff --git a/include/openssl/bn.h b/include/openssl/bn.h index d9491a936b..9abccaffcd 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h @@ -681,6 +681,9 @@ OPENSSL_EXPORT void BN_GENCB_set(BN_GENCB *callback, // the callback, or 1 if |callback| is NULL. OPENSSL_EXPORT int BN_GENCB_call(BN_GENCB *callback, int event, int n); +// BN_GENCB_get_arg returns |callback->arg|. +OPENSSL_EXPORT void *BN_GENCB_get_arg(const BN_GENCB *callback); + // BN_generate_prime_ex sets |ret| to a prime number of |bits| length. If safe // is non-zero then the prime will be such that (ret-1)/2 is also a prime. // (This is needed for Diffie-Hellman groups to ensure that the only subgroups From f8f97bfcbb97c8bd14c4cb6942dd7b0c492cb3e0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 12 Jun 2022 11:10:08 -0400 Subject: [PATCH 2/3] Don't guard alignof static asserts on GCC/Clang. I'm not sure what the history of this is, but it seems to work just fine in MSVC now. Change-Id: Iebdc365486bb30a61a1001f705aef7dcaa2a9fcd Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/52985 Reviewed-by: Adam Langley --- crypto/cipher_extra/e_aesctrhmac.c | 2 -- crypto/cipher_extra/e_aesgcmsiv.c | 4 ---- crypto/cipher_extra/e_chacha20poly1305.c | 2 -- crypto/cipher_extra/e_tls.c | 2 -- crypto/fipsmodule/cipher/e_aes.c | 8 -------- crypto/fipsmodule/cipher/e_aesccm.c | 2 -- crypto/thread_pthread.c | 2 -- crypto/thread_win.c | 2 -- 8 files changed, 24 deletions(-) diff --git a/crypto/cipher_extra/e_aesctrhmac.c b/crypto/cipher_extra/e_aesctrhmac.c index 8c45c8111a..c75f8a836e 100644 --- a/crypto/cipher_extra/e_aesctrhmac.c +++ b/crypto/cipher_extra/e_aesctrhmac.c @@ -38,11 +38,9 @@ struct aead_aes_ctr_hmac_sha256_ctx { OPENSSL_STATIC_ASSERT(sizeof(((EVP_AEAD_CTX *)NULL)->state) >= sizeof(struct aead_aes_ctr_hmac_sha256_ctx), "AEAD state is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(union evp_aead_ctx_st_state) >= alignof(struct aead_aes_ctr_hmac_sha256_ctx), "AEAD state has insufficient alignment"); -#endif static void hmac_init(SHA256_CTX *out_inner, SHA256_CTX *out_outer, const uint8_t hmac_key[32]) { diff --git a/crypto/cipher_extra/e_aesgcmsiv.c b/crypto/cipher_extra/e_aesgcmsiv.c index 555922dd74..ee20e1bb1d 100644 --- a/crypto/cipher_extra/e_aesgcmsiv.c +++ b/crypto/cipher_extra/e_aesgcmsiv.c @@ -45,10 +45,8 @@ struct aead_aes_gcm_siv_asm_ctx { OPENSSL_STATIC_ASSERT(sizeof(((EVP_AEAD_CTX *)NULL)->state) + 8 >= sizeof(struct aead_aes_gcm_siv_asm_ctx), "AEAD state is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(union evp_aead_ctx_st_state) >= 8, "AEAD state has insufficient alignment"); -#endif // asm_ctx_from_ctx returns a 16-byte aligned context pointer from |ctx|. static struct aead_aes_gcm_siv_asm_ctx *asm_ctx_from_ctx( @@ -555,11 +553,9 @@ struct aead_aes_gcm_siv_ctx { OPENSSL_STATIC_ASSERT(sizeof(((EVP_AEAD_CTX *)NULL)->state) >= sizeof(struct aead_aes_gcm_siv_ctx), "AEAD state is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(union evp_aead_ctx_st_state) >= alignof(struct aead_aes_gcm_siv_ctx), "AEAD state has insufficient alignment"); -#endif static int aead_aes_gcm_siv_init(EVP_AEAD_CTX *ctx, const uint8_t *key, size_t key_len, size_t tag_len) { diff --git a/crypto/cipher_extra/e_chacha20poly1305.c b/crypto/cipher_extra/e_chacha20poly1305.c index 1650188958..a9fce3e88e 100644 --- a/crypto/cipher_extra/e_chacha20poly1305.c +++ b/crypto/cipher_extra/e_chacha20poly1305.c @@ -35,11 +35,9 @@ struct aead_chacha20_poly1305_ctx { OPENSSL_STATIC_ASSERT(sizeof(((EVP_AEAD_CTX *)NULL)->state) >= sizeof(struct aead_chacha20_poly1305_ctx), "AEAD state is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(union evp_aead_ctx_st_state) >= alignof(struct aead_chacha20_poly1305_ctx), "AEAD state has insufficient alignment"); -#endif static int aead_chacha20_poly1305_init(EVP_AEAD_CTX *ctx, const uint8_t *key, size_t key_len, size_t tag_len) { diff --git a/crypto/cipher_extra/e_tls.c b/crypto/cipher_extra/e_tls.c index 6d84f7f02c..9713dc7eaa 100644 --- a/crypto/cipher_extra/e_tls.c +++ b/crypto/cipher_extra/e_tls.c @@ -48,11 +48,9 @@ OPENSSL_STATIC_ASSERT(EVP_MAX_MD_SIZE < 256, OPENSSL_STATIC_ASSERT(sizeof(((EVP_AEAD_CTX *)NULL)->state) >= sizeof(AEAD_TLS_CTX), "AEAD state is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(union evp_aead_ctx_st_state) >= alignof(AEAD_TLS_CTX), "AEAD state has insufficient alignment"); -#endif static void aead_tls_cleanup(EVP_AEAD_CTX *ctx) { AEAD_TLS_CTX *tls_ctx = (AEAD_TLS_CTX *)&ctx->state; diff --git a/crypto/fipsmodule/cipher/e_aes.c b/crypto/fipsmodule/cipher/e_aes.c index 61f4ef355d..ad64a39c3f 100644 --- a/crypto/fipsmodule/cipher/e_aes.c +++ b/crypto/fipsmodule/cipher/e_aes.c @@ -335,11 +335,9 @@ ctr128_f aes_ctr_set_key(AES_KEY *aes_key, GCM128_KEY *gcm_key, #endif static EVP_AES_GCM_CTX *aes_gcm_from_cipher_ctx(EVP_CIPHER_CTX *ctx) { -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT( alignof(EVP_AES_GCM_CTX) <= 16, "EVP_AES_GCM_CTX needs more alignment than this function provides"); -#endif // |malloc| guarantees up to 4-byte alignment on 32-bit and 8-byte alignment // on 64-bit systems, so we need to adjust to reach 16-byte alignment. @@ -926,11 +924,9 @@ static int aead_aes_gcm_init_impl(struct aead_aes_gcm_ctx *gcm_ctx, OPENSSL_STATIC_ASSERT(sizeof(((EVP_AEAD_CTX *)NULL)->state) >= sizeof(struct aead_aes_gcm_ctx), "AEAD state is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(union evp_aead_ctx_st_state) >= alignof(struct aead_aes_gcm_ctx), "AEAD state has insufficient alignment"); -#endif static int aead_aes_gcm_init(EVP_AEAD_CTX *ctx, const uint8_t *key, size_t key_len, size_t requested_tag_len) { @@ -1266,11 +1262,9 @@ struct aead_aes_gcm_tls12_ctx { OPENSSL_STATIC_ASSERT(sizeof(((EVP_AEAD_CTX *)NULL)->state) >= sizeof(struct aead_aes_gcm_tls12_ctx), "AEAD state is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(union evp_aead_ctx_st_state) >= alignof(struct aead_aes_gcm_tls12_ctx), "AEAD state has insufficient alignment"); -#endif static int aead_aes_gcm_tls12_init(EVP_AEAD_CTX *ctx, const uint8_t *key, size_t key_len, size_t requested_tag_len) { @@ -1365,11 +1359,9 @@ struct aead_aes_gcm_tls13_ctx { OPENSSL_STATIC_ASSERT(sizeof(((EVP_AEAD_CTX *)NULL)->state) >= sizeof(struct aead_aes_gcm_tls13_ctx), "AEAD state is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(union evp_aead_ctx_st_state) >= alignof(struct aead_aes_gcm_tls13_ctx), "AEAD state has insufficient alignment"); -#endif static int aead_aes_gcm_tls13_init(EVP_AEAD_CTX *ctx, const uint8_t *key, size_t key_len, size_t requested_tag_len) { diff --git a/crypto/fipsmodule/cipher/e_aesccm.c b/crypto/fipsmodule/cipher/e_aesccm.c index fcbd20b32d..c9d52e0a5b 100644 --- a/crypto/fipsmodule/cipher/e_aesccm.c +++ b/crypto/fipsmodule/cipher/e_aesccm.c @@ -279,11 +279,9 @@ struct aead_aes_ccm_ctx { OPENSSL_STATIC_ASSERT(sizeof(((EVP_AEAD_CTX *)NULL)->state) >= sizeof(struct aead_aes_ccm_ctx), "AEAD state is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(union evp_aead_ctx_st_state) >= alignof(struct aead_aes_ccm_ctx), "AEAD state has insufficient alignment"); -#endif static int aead_aes_ccm_init(EVP_AEAD_CTX *ctx, const uint8_t *key, size_t key_len, size_t tag_len, unsigned M, diff --git a/crypto/thread_pthread.c b/crypto/thread_pthread.c index e873d04909..1b8329429c 100644 --- a/crypto/thread_pthread.c +++ b/crypto/thread_pthread.c @@ -26,10 +26,8 @@ OPENSSL_STATIC_ASSERT(sizeof(CRYPTO_MUTEX) >= sizeof(pthread_rwlock_t), "CRYPTO_MUTEX is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(CRYPTO_MUTEX) >= alignof(pthread_rwlock_t), "CRYPTO_MUTEX has insufficient alignment"); -#endif void CRYPTO_MUTEX_init(CRYPTO_MUTEX *lock) { if (pthread_rwlock_init((pthread_rwlock_t *) lock, NULL) != 0) { diff --git a/crypto/thread_win.c b/crypto/thread_win.c index 49ecc12a11..1065884f11 100644 --- a/crypto/thread_win.c +++ b/crypto/thread_win.c @@ -29,10 +29,8 @@ OPENSSL_MSVC_PRAGMA(warning(pop)) OPENSSL_STATIC_ASSERT(sizeof(CRYPTO_MUTEX) >= sizeof(SRWLOCK), "CRYPTO_MUTEX is too small"); -#if defined(__GNUC__) || defined(__clang__) OPENSSL_STATIC_ASSERT(alignof(CRYPTO_MUTEX) >= alignof(SRWLOCK), "CRYPTO_MUTEX has insufficient alignment"); -#endif static BOOL CALLBACK call_once_init(INIT_ONCE *once, void *arg, void **out) { void (**init)(void) = (void (**)(void))arg; From 167f1760ddfeaea1ee1a671ca88aafcccfe30ee0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 12 Jun 2022 13:47:08 -0400 Subject: [PATCH 3/3] 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 3b65acfca7..9b86e5138a 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 64dca1d364..512f06df44 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 6da6e3b232..6412e07bd1 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 @@ _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; } @@ -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 df54713f18..87403678a3 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| *.