kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Implement sk_find manually.

Browse Source 
glibc inlines bsearch, so CFI does observe the function pointer mishap.
Binary search is easy enough, aside from thinking through the edge case
at the end, so just implement it by hand. As a bonus, it actually gives
O(lg N) behavior.

sk_*_find needs to return the *first* match, while bsearch does not
promise a particular one. sk_find thus performs a fixup step to find the
first one, but this is linear in the number of matching elements.
Instead, the binary search should take this into account.

This still leaves qsort, but it's not inlined, so hopefully we can leave
it alone.

Bug: chromium:941463
Change-Id: I5c94d6b15423beea3bdb389639466f8b3ff0dc5d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/35304
Reviewed-by: Adam Langley <agl@google.com>
This commit is contained in:
David Benjamin 2019-03-13 13:44:31 -05:00 committed by Adam Langley
parent 35941f2923
commit aadcce380f
1 changed files with 35 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
crypto/stack/stack.c
View File

@ -56,6 +56,7 @@
#include <openssl/stack.h> #include <openssl/stack.h>
#include <assert.h>
#include <string.h> #include <string.h>
#include <openssl/mem.h> #include <openssl/mem.h>
@ -272,36 +273,39 @@ int sk_find(const _STACK *sk, size_t *out_index, const void *p,
return 0; return 0;
} }
// sk->comp is a function that takes pointers to pointers to elements, but // The stack is sorted, so binary search to find the element.
// qsort and bsearch take a comparison function that just takes pointers to
// 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, // |lo| and |hi| maintain a half-open interval of where the answer may be. All
// e.g., CFI does not notice. Unfortunately, |bsearch| is missing a void* // indices such that |lo <= idx < hi| are candidates.
// parameter in its callback and |bsearch_s| is a mess of incompatibility. size_t lo = 0, hi = sk->num;
const void *const *r = bsearch(&p, sk->data, sk->num, sizeof(void *), while (lo < hi) {
(int (*)(const void *, const void *))sk->comp); // Bias |mid| towards |lo|. See the |r == 0| case below.
if (r == NULL) { size_t mid = lo + (hi - lo - 1) / 2;
return 0; assert(lo <= mid && mid < hi);
} const void *elem = sk->data[mid];
size_t idx = ((void **)r) - sk->data; int r = call_cmp_func(sk->comp, &p, &elem);
// This function always returns the first result. Note this logic is, in the if (r > 0) {
// worst case, O(N) rather than O(log(N)). If this ever becomes a problem, lo = mid + 1; // |mid| is too low.
// restore https://boringssl-review.googlesource.com/c/boringssl/+/32115/ } else if (r < 0) {
// which integrates the preference into the binary search. hi = mid; // |mid| is too high.
while (idx > 0) { } else {
const void *elem = sk->data[idx - 1]; // |mid| matches. However, this function returns the earliest match, so we
if (call_cmp_func(sk->comp, &p, &elem) != 0) { // can only return if the range has size one.
break; if (hi - lo == 1) {
if (out_index != NULL) {
*out_index = mid;
}
return 1;
}
// The sample is biased towards |lo|. |mid| can only be |hi - 1| if
// |hi - lo| was one, so this makes forward progress.
assert(mid + 1 < hi);
hi = mid + 1;
} }
idx--;
} }
if (out_index) {
*out_index = idx; assert(lo == hi);
} return 0; // Not found.
return 1;
} }
void *sk_shift(_STACK *sk) { void *sk_shift(_STACK *sk) {
@ -362,7 +366,10 @@ void sk_sort(_STACK *sk) {
return; return;
} }
// See the comment in sk_find about this cast. // 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, // 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* // e.g., CFI does not notice. Unfortunately, |qsort| is missing a void*