Merge branch 'origin/topic/bbannier/issue-1628'

CHANGES

@@ -1,3 +1,23 @@
+1.10.0-dev.91 | 2024-01-04 12:44:32 +0100
+
+  * Add unit tests for extracting from expanding Views. (Benjamin Bannier, Corelight)
+
+  * Add unit tests for extracting from Views with gaps. (Benjamin Bannier, Corelight)
+
+  * Add fast pass to noop unsafe stream iterator increment/decrement. (Benjamin Bannier, Corelight)
+
+    We already had this optimization for safe, but not for unsafe iterators.
+
+  * GH-1628: Avoid potentially inefficient data access in `View::extract`. (Benjamin Bannier, Corelight)
+
+    While we already had a fast path to extract data out of `View`s
+    consisting of a single chunk we still would use a potentially
+    inefficient approach when extracting from `View`s over multiple chunks.
+
+    This patch uses the same optimized handling for both cases.
+
+    Closes #1628.
+
 1.10.0-dev.86 | 2024-01-02 16:38:08 +0100
 
   * Suppress clang-tidy `misc-include-cleaner` lint. (Benjamin Bannier, Corelight)

VERSION

@@ -1 +1 @@
-1.10.0-dev.86
+1.10.0-dev.91

hilti/runtime/include/types/stream.h

@@ -837,6 +837,9 @@ class UnsafeConstIterator {
     }
 
     void _increment(const integer::safe<uint64_t>& n) {
+        if ( n == 0 )
+            return;
+
         _offset += n;
 
         if ( _chunk && _offset < _chunk->endOffset() )
@@ -846,6 +849,9 @@ class UnsafeConstIterator {
     }
 
     void _decrement(const integer::safe<uint64_t>& n) {
+        if ( n == 0 )
+            return;
+
         _offset -= n;
 
         if ( _chunk && _offset > _chunk->offset() )
@@ -1303,21 +1309,31 @@ class View final {
     View extract(Byte* dst, uint64_t n) const {
         _ensureValid();
 
-        auto p = unsafeBegin();
-
-        // Fast-path for when we're staying inside the initial chunk.
-        if ( auto chunk = p.chunk(); chunk && chunk->inRange(p.offset() + n) ) {
-            memcpy(dst, chunk->data(p.offset()), n);
-            return View(SafeConstIterator(p + n), _end);
-        }
-
         if ( n > size() )
             throw WouldBlock("end of stream view");
 
-        for ( uint64_t i = 0; i < n; ++i )
-            dst[i] = *p++;
+        const auto p = begin();
+
+        const auto* chain = p.chain();
+        assert(chain);
+        assert(chain->isValid());
+        assert(chain->inRange(p.offset()));
+
+        auto offset = p.offset().Ref();
+
+        for ( auto c = chain->findChunk(p.offset()); offset - p.offset().Ref() < n; c = c->next() ) {
+            assert(c);
+
+            const auto into_chunk = offset - c->offset().Ref();
+            const auto remaining = n + p.offset().Ref() - offset;
+            const auto m = std::min(remaining, c->size().Ref() - into_chunk);
+
+            ::memcpy(dst, c->data(offset), m);
+            offset += m;
+            dst += m;
+        }
 
-        return View(SafeConstIterator(p), _end);
+        return View(p + n, _end);
     }
 
     /**

hilti/runtime/src/tests/stream.cc

@@ -1002,6 +1002,51 @@ TEST_CASE("View") {
             Byte dst[1] = {'0'};
             CHECK_THROWS_WITH_AS(Stream().view().extract(dst, sizeof(dst)), "end of stream view", const WouldBlock&);
         }
+
+        SUBCASE("gaps") {
+            auto s = Stream();
+            SUBCASE("just gap") {
+                s.append(nullptr, 3); // Gap.
+                Byte dst[3] = {};
+                REQUIRE_EQ(sizeof(dst), s.size());
+                CHECK_THROWS_WITH_AS(s.view().extract(dst, sizeof(dst)), "data is missing", const MissingData&);
+            }
+
+            SUBCASE("begin in gap") {
+                s.append(nullptr, 2); // Gap.
+                s.append("A");
+                Byte dst[3] = {};
+                REQUIRE_EQ(sizeof(dst), s.size());
+                CHECK_THROWS_WITH_AS(s.view().extract(dst, sizeof(dst)), "data is missing", const MissingData&);
+            }
+
+            SUBCASE("end in gap") {
+                s.append("A");
+                s.append(nullptr, 2); // Gap.
+                Byte dst[3] = {};
+                REQUIRE_EQ(sizeof(dst), s.size());
+                CHECK_THROWS_WITH_AS(s.view().extract(dst, sizeof(dst)), "data is missing", const MissingData&);
+            }
+        }
+
+        SUBCASE("from expanding View") {
+            auto s = Stream();
+            auto v = s.view();
+
+            Byte dst[3] = {};
+
+            CHECK_THROWS_WITH_AS(v.extract(dst, sizeof(dst)), "end of stream view", const WouldBlock&);
+
+            s.append("A");
+            CHECK_THROWS_WITH_AS(v.extract(dst, sizeof(dst)), "end of stream view", const WouldBlock&);
+
+            s.append("B");
+            CHECK_THROWS_WITH_AS(v.extract(dst, sizeof(dst)), "end of stream view", const WouldBlock&);
+
+            s.append("C");
+            CHECK_EQ(v.extract(dst, sizeof(dst)), ""_b);
+            CHECK_EQ(vec(dst), std::vector<Byte>{'A', 'B', 'C'});
+        }
     }
 
     SUBCASE("sub") {