Merge pull request #29377 from vespa-engine/toregge/avoid-reading-bey…

…ond-end-of-stack-dump-buffer Avoid reading beyond end of stack dump buffer.
vespa-engine · Nov 19, 2023 · f08f64e · f08f64e
2 parents a1ca91a + 15e5129
commit f08f64e
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 7 deletions.
diff --git a/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.cpp b/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.cpp
@@ -4,6 +4,7 @@
 #include <vespa/searchlib/query/tree/predicate_query_term.h>
 #include <vespa/vespalib/util/compress.h>
 #include <vespa/vespalib/objects/nbo.h>
+#include <cassert>
 
 using search::query::PredicateQueryTerm;
 
@@ -58,8 +59,7 @@ SimpleQueryStackDumpIterator::~SimpleQueryStackDumpIterator() = default;
 vespalib::stringref
 SimpleQueryStackDumpIterator::read_stringref(const char *&p)
 {
-    uint64_t len;
-    p += vespalib::compress::Integer::decompressPositive(len, p);
+    uint64_t len = readCompressedPositiveInt(p);
     if ((p + len) > _bufEnd) throw false;
     vespalib::stringref result(p, len);
     p += len;
@@ -69,9 +69,24 @@ SimpleQueryStackDumpIterator::read_stringref(const char *&p)
 uint64_t
 SimpleQueryStackDumpIterator::readCompressedPositiveInt(const char *&p)
 {
+    if (p > _bufEnd || !vespalib::compress::Integer::check_decompress_space(p, _bufEnd - p)) {
+        throw false;
+    }
     uint64_t tmp;
     p += vespalib::compress::Integer::decompressPositive(tmp, p);
-    if (p > _bufEnd) throw false;
+    assert(p <= _bufEnd);
+    return tmp;
+}
+
+int64_t
+SimpleQueryStackDumpIterator::readCompressedInt(const char *&p)
+{
+    if (p > _bufEnd || !vespalib::compress::Integer::check_decompress_positive_space(p, _bufEnd - p)) {
+        throw false;
+    }
+    int64_t tmp;
+    p += vespalib::compress::Integer::decompress(tmp, p);
+    assert(p <= _bufEnd);
     return tmp;
 }
 
@@ -98,11 +113,8 @@ bool SimpleQueryStackDumpIterator::readNext() {
     _currType = ParseItem::GetType(typefield);
 
     if (ParseItem::GetFeature_Weight(typefield)) {
-        int64_t tmpLong;
-        if (p >= _bufEnd) return false;
-        p += vespalib::compress::Integer::decompress(tmpLong, p);
+        int64_t tmpLong = readCompressedInt(p);
         _currWeight.setPercent(tmpLong);
-        if (p > _bufEnd) return false;
     } else {
         _currWeight.setPercent(100);
     }

diff --git a/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.h b/searchlib/src/vespa/searchlib/parsequery/stackdumpiterator.h
@@ -51,6 +51,7 @@ class SimpleQueryStackDumpIterator
 
     VESPA_DLL_LOCAL vespalib::stringref read_stringref(const char *&p);
     VESPA_DLL_LOCAL uint64_t readCompressedPositiveInt(const char *&p);
+    VESPA_DLL_LOCAL int64_t readCompressedInt(const char *&p);
     VESPA_DLL_LOCAL bool readPredicate(const char *&p);
     VESPA_DLL_LOCAL bool readNN(const char *&p);
     VESPA_DLL_LOCAL bool readComplexTerm(const char *& p);

diff --git a/vespalib/src/tests/compress/compress_test.cpp b/vespalib/src/tests/compress/compress_test.cpp
@@ -26,6 +26,9 @@ CompressTest::verifyPositiveNumber(uint64_t n, const uint8_t * expected, size_t
     for (size_t i(0); i < sz; i++) {
         EXPECT_EQUAL(expected[i], buf[i]);
     }
+    EXPECT_FALSE(Integer::check_decompress_positive_space(expected, 0u));
+    EXPECT_FALSE(Integer::check_decompress_positive_space(expected, sz - 1));
+    EXPECT_TRUE(Integer::check_decompress_positive_space(expected, sz));
     uint64_t v(0);
     EXPECT_EQUAL(sz, Integer::decompressPositive(v, expected));
     EXPECT_EQUAL(n, v);
@@ -39,6 +42,9 @@ CompressTest::verifyNumber(int64_t n, const uint8_t * expected, size_t sz) {
     for (size_t i(0); i < sz; i++) {
         EXPECT_EQUAL(expected[i], buf[i]);
     }
+    EXPECT_FALSE(Integer::check_decompress_space(expected, 0u));
+    EXPECT_FALSE(Integer::check_decompress_space(expected, sz - 1));
+    EXPECT_TRUE(Integer::check_decompress_space(expected, sz));
     int64_t v(0);
     EXPECT_EQUAL(sz, Integer::decompress(v, expected));
     EXPECT_EQUAL(n, v);

diff --git a/vespalib/src/vespa/vespalib/util/compress.h b/vespalib/src/vespa/vespalib/util/compress.h
@@ -105,6 +105,32 @@ class Integer {
         }
         return numbytes;
     }
+
+    static bool check_decompress_space(const void* srcv, size_t len) {
+        if (len == 0u) {
+            return false;
+        }
+        const uint8_t * src = static_cast<const uint8_t *>(srcv);
+        const uint8_t c = src[0];
+        if ((c & 0x40) != 0) {
+            return (((c & 0x20) != 0) ? (len >= 4u) : (len >= 2u));
+        } else {
+            return true;
+        }
+    }
+
+    static bool check_decompress_positive_space(const void* srcv, size_t len) {
+        if (len == 0u) {
+            return false;
+        }
+        const uint8_t * src = static_cast<const uint8_t *>(srcv);
+        const uint8_t c = src[0];
+        if ((c & 0x80) != 0) {
+            return (((c & 0x40) != 0) ? (len >= 4u) : (len >= 2u));
+        } else {
+            return true;
+        }
+    }
 private:
     [[ noreturn ]] static void throw_too_big(int64_t n);
     [[ noreturn ]] static void throw_too_big(uint64_t n);