reftable/block: fix error handling when searching restart points

When doing the binary search over restart points in a block we need to
decode the record keys. This decoding step can result in an error when
the block is corrupted, which we indicate to the caller of the binary
search by setting `args.error = 1`. But the only caller that exists
mishandles this because it in fact performs the error check before
calling `binsearch()`.

Fix this bug by checking for errors at the right point in time.
Furthermore, refactor `binsearch()` so that it aborts the search in case
the callback function returns a negative value so that we don't
needlessly continue to search the block.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/reftable/basics.c b/reftable/basics.c
index 2c5f34b39e61179b7c3e9512c106b2e675e97ed8..fea711db7e23e680fbf1937df43d625bb9c055fb 100644 (file)
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -39,8 +39,11 @@ size_t binsearch(size_t sz, int (*f)(size_t k, void *args), void *args)
         */
        while (hi - lo > 1) {
                size_t mid = lo + (hi - lo) / 2;
+               int ret = f(mid, args);
+               if (ret < 0)
+                       return sz;
 
-               if (f(mid, args))
+               if (ret > 0)
                        hi = mid;
                else
                        lo = mid;

diff --git a/reftable/basics.h b/reftable/basics.h
index 2672520e765aaedac0b7dd9c427199978763465a..523ecd530762f6e4cde48edfa3761b3139b21b92 100644 (file)
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -22,8 +22,9 @@ uint32_t get_be24(uint8_t *in);
 void put_be16(uint8_t *out, uint16_t i);
 
 /*
- * find smallest index i in [0, sz) at which f(i) is true, assuming
- * that f is ascending. Return sz if f(i) is false for all indices.
+ * find smallest index i in [0, sz) at which `f(i) > 0`, assuming that f is
+ * ascending. Return sz if `f(i) == 0` for all indices. The search is aborted
+ * and `sz` is returned in case `f(i) < 0`.
  *
  * Contrary to bsearch(3), this returns something useful if the argument is not
  * found.

diff --git a/reftable/block.c b/reftable/block.c
index 57e3dbf658c5c2439d72a0cd695ba36f42161ac7..ca217636aec12feb22e0f87b4c25821780970c9a 100644 (file)
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -387,11 +387,6 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,
        int err = 0;
        size_t i;
 
-       if (args.error) {
-               err = REFTABLE_FORMAT_ERROR;
-               goto done;
-       }
-
        /*
         * Perform a binary search over the block's restart points, which
         * avoids doing a linear scan over the whole block. Like this, we
@@ -405,6 +400,10 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,
         * too many record.
         */
        i = binsearch(br->restart_count, &restart_needle_less, &args);
+       if (args.error) {
+               err = REFTABLE_FORMAT_ERROR;
+               goto done;
+       }
 
        /*
         * Now there are multiple cases: