From aadcce380fe9e5e17ff38f8471e956463fc4df21 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Wed, 13 Mar 2019 13:44:31 -0500
Subject: [PATCH] Implement sk_find manually.

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>
---
 crypto/stack/stack.c | 63 ++++++++++++++++++++++++--------------------
 1 file changed, 35 insertions(+), 28 deletions(-)

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