Rewrite smartlist_bsearch_idx() to not be broken for lists of length zero or one (fixes bug 7191)

2 files changed, 98 insertions, 17 deletions

diff --git a/changes/bug7191 b/changes/bug7191
new file mode 100644
index 000000000..f50b16e3a
--- /dev/null
+++ b/changes/bug7191
@@ -0,0 +1,3 @@
+ o Bugfixes
+   - The smartlist_bsearch_idx() function was broken for lists of length zero
+     or one; fix it.  This fixes bug7191.
diff --git a/src/common/container.c b/src/common/container.c
index d941048b0..d4a2f89c9 100644
--- a/src/common/container.c
+++ b/src/common/container.c
@@ -571,31 +571,109 @@ smartlist_bsearch_idx(const smartlist_t *sl, const void *key,
                       int (*compare)(const void *key, const void **member),
                       int *found_out)
 {
-  int hi = smartlist_len(sl) - 1, lo = 0, cmp, mid;
+  int hi, lo, cmp, mid, len, diff;
+
+  tor_assert(sl);
+  tor_assert(compare);
+  tor_assert(found_out);
+
+  len = smartlist_len(sl);
+
+  /* Check for the trivial case of a zero-length list */
+  if (len == 0) {
+    *found_out = 0;
+    /* We already know smartlist_len(sl) is 0 in this case */
+    return 0;
+  }
+
+  /* Okay, we have a real search to do */
+  tor_assert(len > 0);
+  lo = 0;
+  hi = len - 1;
+
+  /*
+   * These invariants are always true:
+   *
+   * For all i such that 0 <= i < lo, sl[i] < key
+   * For all i such that hi < i <= len, sl[i] > key
+   */
 
   while (lo <= hi) {
-    mid = (lo + hi) / 2;
+    diff = hi - lo;
+    /*
+     * We want mid = (lo + hi) / 2, but that could lead to overflow, so
+     * instead diff = hi - lo (non-negative because of loop condition), and
+     * then hi = lo + diff, mid = (lo + lo + diff) / 2 = lo + (diff / 2).
+     */
+    mid = lo + (diff / 2);
     cmp = compare(key, (const void**) &(sl->list[mid]));
-    if (cmp>0) { /* key > sl[mid] */
-      lo = mid+1;
-    } else if (cmp<0) { /* key < sl[mid] */
-      hi = mid-1;
-    } else { /* key == sl[mid] */
+    if (cmp == 0) {
+      /* sl[mid] == key; we found it */
       *found_out = 1;
       return mid;
-    }
-  }
-  /* lo > hi. */
-  {
-    tor_assert(lo >= 0);
-    if (lo < smartlist_len(sl)) {
-      cmp = compare(key, (const void**) &(sl->list[lo]));
+    } else if (cmp > 0) {
+      /*
+       * key > sl[mid] and an index i such that sl[i] == key must
+       * have i > mid if it exists.
+       */
+
+      /*
+       * Since lo <= mid <= hi, hi can only decrease on each iteration (by
+       * being set to mid - 1) and hi is initially len - 1, mid < len should
+       * always hold, and this is not symmetric with the left end of list
+       * mid > 0 test below.  A key greater than the right end of the list
+       * should eventually lead to lo == hi == mid == len - 1, and then
+       * we set lo to len below and fall out to the same exit we hit for
+       * a key in the middle of the list but not matching.  Thus, we just
+       * assert for consistency here rather than handle a mid == len case.
+       */
+      tor_assert(mid < len);
+      /* Move lo to the element immediately after sl[mid] */
+      lo = mid + 1;
+    } else {
+      /* This should always be true in this case */
       tor_assert(cmp < 0);
-    } else if (smartlist_len(sl)) {
-      cmp = compare(key, (const void**) &(sl->list[smartlist_len(sl)-1]));
-      tor_assert(cmp > 0);
+
+      /*
+       * key < sl[mid] and an index i such that sl[i] == key must
+       * have i < mid if it exists.
+       */
+
+      if (mid > 0) {
+        /* Normal case, move hi to the element immediately before sl[mid] */
+        hi = mid - 1;
+      } else {
+        /* These should always be true in this case */
+        tor_assert(mid == lo);
+        tor_assert(mid == 0);
+        /*
+         * We were at the beginning of the list and concluded that every
+         * element e compares e > key.
+         */
+        *found_out = 0;
+        return 0;
+      }
     }
   }
+
+  /*
+   * lo > hi; we have no element matching key but we have elements falling
+   * on both sides of it.  The lo index points to the first element > key.
+   */
+  tor_assert(lo == hi + 1); /* All other cases should have been handled */
+  tor_assert(lo >= 0);
+  tor_assert(lo <= len);
+  tor_assert(hi >= 0);
+  tor_assert(hi <= len);
+
+  if (lo < len) {
+    cmp = compare(key, (const void **) &(sl->list[lo]));
+    tor_assert(cmp < 0);
+  } else {
+    cmp = compare(key, (const void **) &(sl->list[len-1]));
+    tor_assert(cmp > 0);
+  }
+
   *found_out = 0;
   return lo;
 }