Merge pull request #25053 from vespa-engine/toregge/add-range-checks-for-bitvector-setbit-and-clearbit

Add range checks for BitVector setBits and clearBits member functions,

5 files changed, 61 insertions, 11 deletions

diff --git a/searchlib/src/vespa/searchlib/common/allocatedbitvector.cpp b/searchlib/src/vespa/searchlib/common/allocatedbitvector.cpp
index 33f044845aa..2dd402a66f7 100644
--- a/searchlib/src/vespa/searchlib/common/allocatedbitvector.cpp
+++ b/searchlib/src/vespa/searchlib/common/allocatedbitvector.cpp
@@ -69,7 +69,7 @@ AllocatedBitVector::AllocatedBitVector(Index numberOfElements, Index capacityBit
         if (minCount/8 == numberOfElements/8) {
             static_cast<Word *>(getStart())[numWords()-1] &= ~endBits(minCount);
         }
-        setBit(size()); // Guard bit
+        set_bit_no_range_check(size()); // Guard bit
     }
     updateCount();
 }
@@ -90,7 +90,7 @@ AllocatedBitVector::AllocatedBitVector(const BitVector & rhs, std::pair<Index, I
     _capacityBits = computeCapacity(_capacityBits, _alloc.size());
     memcpy(_alloc.get(),  rhs.getStart(), numBytes(size_capacity.first - rhs.getStartIndex()));
     init(_alloc.get(), 0, size_capacity.first);
-    setBit(size());
+    set_bit_no_range_check(size());
     updateCount();
 }
 
diff --git a/searchlib/src/vespa/searchlib/common/bitvector.cpp b/searchlib/src/vespa/searchlib/common/bitvector.cpp
index 03fe97cabbb..47ca590c9bc 100644
--- a/searchlib/src/vespa/searchlib/common/bitvector.cpp
+++ b/searchlib/src/vespa/searchlib/common/bitvector.cpp
@@ -10,6 +10,7 @@
 #include <vespa/vespalib/objects/nbostream.h>
 #include <vespa/fastos/file.h>
 #include <cassert>
+#include <cstdlib>
 
 #include <vespa/log/log.h>
 LOG_SETUP(".searchlib.common.bitvector");
@@ -42,6 +43,8 @@ namespace search {
 
 using vespalib::nbostream;
 
+bool BitVector::_enable_range_check = false;
+
 Alloc
 BitVector::allocatePaddedAndAligned(Index start, Index end, Index capacity, const Alloc* init_alloc)
 {
@@ -79,7 +82,7 @@ void
 BitVector::clear()
 {
     memset(getActiveStart(), '\0', getActiveBytes());
-    setBit(size()); // Guard bit
+    set_bit_no_range_check(size()); // Guard bit
     setTrueBits(0);
 }
 
@@ -369,6 +372,15 @@ BitVector::create(const BitVector & rhs)
     return std::make_unique<AllocatedBitVector>(rhs);
 }
 
+void
+BitVector::consider_enable_range_check()
+{
+    const char *env = getenv("VESPA_BITVECTOR_RANGE_CHECK");
+    if (env != nullptr && strcmp(env, "true") == 0) {
+        _enable_range_check = true;
+    }
+}
+
 MMappedBitVector::MMappedBitVector(Index numberOfElements, FastOS_FileInterface &file,
                                    int64_t offset, Index doccount) :
     BitVector()
@@ -422,5 +434,17 @@ operator>>(nbostream &in, AllocatedBitVector &bv)
     return in;
 }
 
+class ConsiderEnableRangeCheckCaller
+{
+public:
+    ConsiderEnableRangeCheckCaller();
+};
+
+ConsiderEnableRangeCheckCaller::ConsiderEnableRangeCheckCaller()
+{
+    BitVector::consider_enable_range_check();
+}
+
+ConsiderEnableRangeCheckCaller consider_enable_range_check_caller;
 
 } // namespace search
diff --git a/searchlib/src/vespa/searchlib/common/bitvector.h b/searchlib/src/vespa/searchlib/common/bitvector.h
index d3671d61ccf..1d289435f6d 100644
--- a/searchlib/src/vespa/searchlib/common/bitvector.h
+++ b/searchlib/src/vespa/searchlib/common/bitvector.h
@@ -8,6 +8,7 @@
 #include <vespa/vespalib/util/atomic.h>
 #include <vespa/fastos/types.h>
 #include <algorithm>
+#include <cassert>
 
 namespace vespalib {
     class nbostream;
@@ -134,23 +135,38 @@ public:
     }
 
     void setSize(Index sz) {
-        setBit(sz);  // Need to place the new stop sign first
+        set_bit_no_range_check(sz);  // Need to place the new stop sign first
         std::atomic_thread_fence(std::memory_order_release);
         if (sz > _sz) {
             // Can only remove the old stopsign if it is ahead of the new.
-            clearBit(_sz);
+            clear_bit_no_range_check(_sz);
         }
         vespalib::atomic::store_ref_release(_sz, sz);
     }
-    void setBit(Index idx) {
+    void set_bit_no_range_check(Index idx) {
         store(_words[wordNum(idx)], _words[wordNum(idx)] | mask(idx));
     }
-    void clearBit(Index idx) {
+    void clear_bit_no_range_check(Index idx) {
         store(_words[wordNum(idx)], _words[wordNum(idx)] & ~ mask(idx));
     }
-    void flipBit(Index idx) {
+    void flip_bit_no_range_check(Index idx) {
         store(_words[wordNum(idx)], _words[wordNum(idx)] ^ mask(idx));
     }
+    void range_check(Index idx) {
+        assert(!_enable_range_check || (idx >= _startOffset && idx < _sz));
+    }
+    void setBit(Index idx) {
+        range_check(idx);
+        set_bit_no_range_check(idx);
+    }
+    void clearBit(Index idx) {
+        range_check(idx);
+        clear_bit_no_range_check(idx);
+    }
+    void flipBit(Index idx) {
+        range_check(idx);
+        flip_bit_no_range_check(idx);
+    }
 
     void andWith(const BitVector &right);
     void orWith(const BitVector &right);
@@ -187,6 +203,14 @@ public:
             incNumBits();
         }
     }
+
+    void clear_bit_and_maintain_count_no_range_check(Index idx) {
+        if (testBit(idx)) {
+            clear_bit_no_range_check(idx);
+            decNumBits();
+        }
+    }
+
     /**
      * Clears a bit and maintains count of number of bits set.
      * @param idx
@@ -251,6 +275,7 @@ public:
     static UP create(const BitVector & org, Index start, Index end);
     static UP create(Index numberOfElements);
     static UP create(const BitVector & rhs);
+    static void consider_enable_range_check();
 protected:
     using Alloc = vespalib::alloc::Alloc;
     VESPA_DLL_LOCAL BitVector(void * buf, Index start, Index end);
@@ -291,7 +316,7 @@ private:
         return (end >= start) ? (numWords(end) - wordNum(start)) : 0;
     }
     static Index invalidCount() { return std::numeric_limits<Index>::max(); }
-    void setGuardBit() { setBit(size()); }
+    void setGuardBit() { set_bit_no_range_check(size()); }
     void incNumBits() {
         if ( isValidCount() ) {
             _numTrueBits.store(_numTrueBits.load(std::memory_order_relaxed) + 1, std::memory_order_relaxed);
@@ -360,6 +385,7 @@ private:
     Index          _startOffset;  // This is the official start
     Index          _sz;           // This is the official end.
     mutable std::atomic<Index>  _numTrueBits;
+    static bool _enable_range_check;
 
 protected:
     friend vespalib::nbostream &
diff --git a/searchlib/src/vespa/searchlib/common/growablebitvector.cpp b/searchlib/src/vespa/searchlib/common/growablebitvector.cpp
index 5f971e21cd3..b0f8ff365ca 100644
--- a/searchlib/src/vespa/searchlib/common/growablebitvector.cpp
+++ b/searchlib/src/vespa/searchlib/common/growablebitvector.cpp
@@ -28,7 +28,7 @@ GrowableBitVector::grow(BitWord::Index newSize, BitWord::Index newCapacity)
     if (newCapacity != self.capacity()) {
         auto tbv = std::make_unique<AllocatedBitVector>(newSize, newCapacity, self._alloc.get(), self.size(), &self._alloc);
         if (newSize > self.size()) {
-            tbv->clearBitAndMaintainCount(self.size());  // Clear old guard bit.
+            tbv->clear_bit_and_maintain_count_no_range_check(self.size());  // Clear old guard bit.
         }
         auto to_hold = std::make_unique<GenerationHeldAllocatedBitVector>(std::move(_stored));
         _self.store(tbv.get(), std::memory_order_release);
diff --git a/searchlib/src/vespa/searchlib/common/partialbitvector.cpp b/searchlib/src/vespa/searchlib/common/partialbitvector.cpp
index 128a9ab8452..af4e2efd315 100644
--- a/searchlib/src/vespa/searchlib/common/partialbitvector.cpp
+++ b/searchlib/src/vespa/searchlib/common/partialbitvector.cpp
@@ -26,7 +26,7 @@ PartialBitVector::PartialBitVector(const BitVector & org, Index start, Index end
     if (org.size() < end) {
         clearInterval(org.size(), end);
     }
-    setBit(size());
+    set_bit_no_range_check(size());
 }
 
 PartialBitVector::~PartialBitVector() = default;