cleanup: fix possible overflow errors in binary search
authorDerrick Stolee <dstolee@microsoft.com>
Sun, 8 Oct 2017 18:29:37 +0000 (14:29 -0400)
committerJunio C Hamano <gitster@pobox.com>
Mon, 9 Oct 2017 23:57:24 +0000 (08:57 +0900)
A common mistake when writing binary search is to allow possible
integer overflow by using the simple average:

mid = (min + max) / 2;

Instead, use the overflow-safe version:

mid = min + (max - min) / 2;

This translation is safe since the operation occurs inside a loop
conditioned on "min < max". The included changes were found using
the following git grep:

git grep '/ *2;' '*.c'

Making this cleanup will prevent future review friction when a new
binary search is contructed based on existing code.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
12 files changed:
builtin/index-pack.c
builtin/pack-objects.c
builtin/unpack-objects.c
cache-tree.c
compat/regex/regex_internal.c
compat/regex/regexec.c
packfile.c
sha1-lookup.c
sha1_name.c
string-list.c
utf8.c
xdiff/xpatience.c
index f2be145e128d819da3adeaed0d0a05218fbe1411..8ec459f522522897c9ed4def4e72b4263d4c93e8 100644 (file)
@@ -633,7 +633,7 @@ static int find_ofs_delta(const off_t offset, enum object_type type)
        int first = 0, last = nr_ofs_deltas;
 
        while (first < last) {
-               int next = (first + last) / 2;
+               int next = first + (last - first) / 2;
                struct ofs_delta_entry *delta = &ofs_deltas[next];
                int cmp;
 
@@ -687,7 +687,7 @@ static int find_ref_delta(const unsigned char *sha1, enum object_type type)
        int first = 0, last = nr_ref_deltas;
 
        while (first < last) {
-               int next = (first + last) / 2;
+               int next = first + (last - first) / 2;
                struct ref_delta_entry *delta = &ref_deltas[next];
                int cmp;
 
index 5ee2c48ffb82e4adde3fe25b4661f3624314f238..6e77dfd44439f4c5f928a73dce3233ff6e63c3b4 100644 (file)
@@ -1277,7 +1277,7 @@ static int done_pbase_path_pos(unsigned hash)
        int lo = 0;
        int hi = done_pbase_paths_num;
        while (lo < hi) {
-               int mi = (hi + lo) / 2;
+               int mi = lo + (hi - lo) / 2;
                if (done_pbase_paths[mi] == hash)
                        return mi;
                if (done_pbase_paths[mi] < hash)
index 689a29fac1a50df6e5b8fc3cda617ef1c6d060b3..62ea264c46783374d0f1968c19ea7581498a1f87 100644 (file)
@@ -394,7 +394,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
                lo = 0;
                hi = nr;
                while (lo < hi) {
-                       mid = (lo + hi)/2;
+                       mid = lo + (hi - lo) / 2;
                        if (base_offset < obj_list[mid].offset) {
                                hi = mid;
                        } else if (base_offset > obj_list[mid].offset) {
index 71d092ed514549f03407aef4fd17ab1467191c68..d3f7401278bd5130558dde18da1235c188bb7303 100644 (file)
@@ -49,7 +49,7 @@ static int subtree_pos(struct cache_tree *it, const char *path, int pathlen)
        lo = 0;
        hi = it->subtree_nr;
        while (lo < hi) {
-               int mi = (lo + hi) / 2;
+               int mi = lo + (hi - lo) / 2;
                struct cache_tree_sub *mdl = down[mi];
                int cmp = subtree_name_cmp(path, pathlen,
                                           mdl->name, mdl->namelen);
index d4121f2f4f1cfa9bf90c22b6e3c944f451225fdf..98342b83163f2b4f1221891995c6484246a45977 100644 (file)
@@ -613,7 +613,7 @@ re_string_reconstruct (re_string_t *pstr, int idx, int eflags)
              int low = 0, high = pstr->valid_len, mid;
              do
                {
-                 mid = (high + low) / 2;
+                 mid = low + (high - low) / 2;
                  if (pstr->offsets[mid] > offset)
                    high = mid;
                  else if (pstr->offsets[mid] < offset)
@@ -1394,7 +1394,7 @@ re_node_set_contains (const re_node_set *set, int elem)
   right = set->nelem - 1;
   while (idx < right)
     {
-      mid = (idx + right) / 2;
+      mid = idx + (right - idx) / 2;
       if (set->elems[mid] < elem)
        idx = mid + 1;
       else
index 0a745d9c3b375fbfdefa3afcd021b952cef0e685..6f2b48a78bc3d8578a3a1a1e8fc28757ef2ea78b 100644 (file)
@@ -4284,7 +4284,7 @@ search_cur_bkref_entry (const re_match_context_t *mctx, int str_idx)
   last = right = mctx->nbkref_ents;
   for (left = 0; left < right;)
     {
-      mid = (left + right) / 2;
+      mid = left + (right - left) / 2;
       if (mctx->bkref_ents[mid].str_idx < str_idx)
        left = mid + 1;
       else
index eab7542487a194ecd25708f906e66c40e1bed336..4a5fe7ab1883843a389ce74bf1c7bd89890d8e51 100644 (file)
@@ -1743,7 +1743,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
                       sha1[0], sha1[1], sha1[2], lo, hi, p->num_objects);
 
        while (lo < hi) {
-               unsigned mi = (lo + hi) / 2;
+               unsigned mi = lo + (hi - lo) / 2;
                int cmp = hashcmp(index + mi * stride, sha1);
 
                if (debug_lookup)
index 2552b7902c70154282eb278ffed7e21b2d618514..4cf3ebd9212f6d5c9b9829373e58c34b83f0a548 100644 (file)
@@ -10,7 +10,7 @@ static uint32_t take2(const unsigned char *sha1)
  * Conventional binary search loop looks like this:
  *
  *      do {
- *              int mi = (lo + hi) / 2;
+ *              int mi = lo + (hi - lo) / 2;
  *              int cmp = "entry pointed at by mi" minus "target";
  *              if (!cmp)
  *                      return (mi is the wanted one)
@@ -95,7 +95,7 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
                        hi = mi;
                else
                        lo = mi + 1;
-               mi = (hi + lo) / 2;
+               mi = lo + (hi - lo) / 2;
        } while (lo < hi);
        return -lo-1;
 }
index 134ac9742f9eed3dc6e587875bdb9aa77b7a8307..c7c5ab376ccb56b3a4f4533f61293bd543ea8097 100644 (file)
@@ -157,7 +157,7 @@ static void unique_in_pack(struct packed_git *p,
        num = p->num_objects;
        last = num;
        while (first < last) {
-               uint32_t mid = (first + last) / 2;
+               uint32_t mid = first + (last - first) / 2;
                const unsigned char *current;
                int cmp;
 
index 806b4c87232fb2ed307f6ce64817e14feefc216f..a0cf0cfe88ee38f35ec33662778ec1bd7036a127 100644 (file)
@@ -16,7 +16,7 @@ static int get_entry_index(const struct string_list *list, const char *string,
        compare_strings_fn cmp = list->cmp ? list->cmp : strcmp;
 
        while (left + 1 < right) {
-               int middle = (left + right) / 2;
+               int middle = left + (right - left) / 2;
                int compare = cmp(string, list->items[middle].string);
                if (compare < 0)
                        right = middle;
diff --git a/utf8.c b/utf8.c
index 47a42047c814fb5c1a933d75fe1b5ecf338dfda1..2c27ce0137f8a60ca2fadf855f2c67738931e2f8 100644 (file)
--- a/utf8.c
+++ b/utf8.c
@@ -32,7 +32,7 @@ static int bisearch(ucs_char_t ucs, const struct interval *table, int max)
        if (ucs < table[0].first || ucs > table[max].last)
                return 0;
        while (max >= min) {
-               mid = (min + max) / 2;
+               mid = min + (max - min) / 2;
                if (ucs > table[mid].last)
                        min = mid + 1;
                else if (ucs < table[mid].first)
index a613efc7034bc0dc45bf13428c819701f044323d..9f91702de740c3a2ca922059e71a32b0245edf1b 100644 (file)
@@ -166,7 +166,7 @@ static int binary_search(struct entry **sequence, int longest,
        int left = -1, right = longest;
 
        while (left + 1 < right) {
-               int middle = (left + right) / 2;
+               int middle = left + (right - left) / 2;
                /* by construction, no two entries can be equal */
                if (sequence[middle]->line2 > entry->line2)
                        right = middle;