Mostly fix undefined casts around STACK_OF's comparator.
The calls to qsort and bsearch are still invalid, but not avoidable
without reimplementing them. Fortunately, they cross libraries, so CFI
does not object.
With that, all that's left is LHASH!
Bug: chromium:785442
Change-Id: I6d29f60fac5cde1f7870d7cc515346e55b98315b
Reviewed-on: https://boringssl-review.googlesource.com/32114
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/stack/stack.c b/crypto/stack/stack.c
index fe50bb9..fe456fc 100644
--- a/crypto/stack/stack.c
+++ b/crypto/stack/stack.c
@@ -235,7 +235,9 @@
return NULL;
}
-int sk_find(const _STACK *sk, size_t *out_index, 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 **)) {
if (sk == NULL) {
return 0;
}
@@ -259,7 +261,8 @@
if (!sk_is_sorted(sk)) {
for (size_t i = 0; i < sk->num; i++) {
- if (sk->comp(&p, (const void **)&sk->data[i]) == 0) {
+ const void *elem = sk->data[i];
+ if (call_cmp_func(sk->comp, &p, &elem) == 0) {
if (out_index) {
*out_index = i;
}
@@ -274,6 +277,10 @@
// elements. However, since we're passing an array of pointers to
// qsort/bsearch, 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, |bsearch| is missing a void*
+ // parameter in its callback and |bsearch_s| is a mess of incompatibility.
const void *const *r = bsearch(&p, sk->data, sk->num, sizeof(void *),
(int (*)(const void *, const void *))sk->comp);
if (r == NULL) {
@@ -281,8 +288,11 @@
}
size_t idx = ((void **)r) - sk->data;
// This function always returns the first result.
- while (idx > 0 &&
- sk->comp(&p, (const void **)&sk->data[idx - 1]) == 0) {
+ while (idx > 0) {
+ const void *elem = sk->data[idx - 1];
+ if (call_cmp_func(sk->comp, &p, &elem) != 0) {
+ break;
+ }
idx--;
}
if (out_index) {
@@ -352,6 +362,11 @@
}
// See the comment in sk_find about this cast.
+ //
+ // 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.
comp_func = (int (*)(const void *, const void *))(sk->comp);
qsort(sk->data, sk->num, sizeof(void *), comp_func);
sk->sorted = 1;
diff --git a/include/openssl/stack.h b/include/openssl/stack.h
index 3a7155f..82869cf 100644
--- a/include/openssl/stack.h
+++ b/include/openssl/stack.h
@@ -100,6 +100,9 @@
// if |*a| is less than, equal to or greater than |*b|, respectively. Note the
// 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_*|
+// functions will be passed a type-specific wrapper to call it correctly.
typedef int (*stack_cmp_func)(const void **a, const void **b);
// stack_st contains an array of pointers. It is not designed to be used
@@ -188,7 +191,9 @@
// Note this differs from OpenSSL. The type signature is slightly different, and
// 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);
+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 **));
// sk_shift removes and returns the first element in the stack, or returns NULL
// if the stack is empty.
@@ -292,6 +297,13 @@
return (void *)((stack_##name##_copy_func)copy_func)((ptrtype)ptr); \
} \
\
+ static inline OPENSSL_UNUSED int sk_##name##_call_cmp_func( \
+ stack_cmp_func cmp_func, const void **a, const void **b) { \
+ constptrtype a_ptr = (constptrtype)*a; \
+ constptrtype b_ptr = (constptrtype)*b; \
+ return ((stack_##name##_cmp_func)cmp_func)(&a_ptr, &b_ptr); \
+ } \
+ \
static inline OPENSSL_UNUSED STACK_OF(name) * \
sk_##name##_new(stack_##name##_cmp_func comp) { \
return (STACK_OF(name) *)sk_new((stack_cmp_func)comp); \
@@ -347,7 +359,8 @@
\
static inline OPENSSL_UNUSED int sk_##name##_find( \
const STACK_OF(name) *sk, size_t *out_index, constptrtype p) { \
- return sk_find((const _STACK *)sk, out_index, (const void *)p); \
+ return sk_find((const _STACK *)sk, out_index, (const void *)p, \
+ sk_##name##_call_cmp_func); \
} \
\
static inline OPENSSL_UNUSED ptrtype sk_##name##_shift(STACK_OF(name) *sk) { \
