Make pbf_reader actually provide the strong exception guarantee

CHANGELOG.md

@@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](https://semver.org/).
 ### Fixed
 
 - Fix `-Wsign-conversion` warning in `basic_pbf_writer::commit_submessage()`.
+- `pbf_reader` (and `pbf_message`) now actually provide the documented strong exception guarantee. Previously `next()` on an invalid tag or wire type, `skip()`, `get_view()`, `get_message()`, `get_packed_*()` on a truncated or invalid length-delimited field, and `next(tag)` when skipping over an invalid field would throw after having already advanced the reader.
 
 ## [1.8.2] - 2026-06-30
 

include/protozero/pbf_reader.hpp

@@ -88,11 +88,12 @@ class pbf_reader {
     template <typename T>
     iterator_range<const_fixed_iterator<T>> packed_fixed() {
         protozero_assert(tag() != 0 && "call next() before accessing field value");
-        const auto len = get_len_and_skip();
-        if (len % sizeof(T) != 0) {
+        const auto view = peek_length_delimited();
+        if (view.size() % sizeof(T) != 0) {
             throw invalid_length_exception{};
         }
-        return {const_fixed_iterator<T>(m_data - len),
+        consume_until(view.data() + view.size());
+        return {const_fixed_iterator<T>(view.data()),
                 const_fixed_iterator<T>(m_data)};
     }
 
@@ -108,15 +109,30 @@ class pbf_reader {
         return static_cast<T>(decode_zigzag64(decode_varint(&m_data, m_end)));
     }
 
-    pbf_length_type get_length() {
-        return get_varint<pbf_length_type>();
+    // Decode the length of the current length-delimited field and return a
+    // view of its data. This does not change the reader, so callers can do
+    // further checks before calling consume_until() to keep the strong
+    // exception guarantee.
+    data_view peek_length_delimited() const {
+        const char* data = m_data;
+        const auto len = static_cast<pbf_length_type>(decode_varint(&data, m_end));
+        if (m_end - data < static_cast<ptrdiff_t>(len)) {
+            throw end_of_buffer_exception{};
+        }
+        return {data, len};
     }
 
     void skip_bytes(pbf_length_type len) {
         if (m_end - m_data < static_cast<ptrdiff_t>(len)) {
             throw end_of_buffer_exception{};
         }
-        m_data += len;
+        consume_until(m_data + len);
+    }
+
+    // Mark all data up to (but not including) pos as consumed.
+    void consume_until(const char* pos) {
+        protozero_assert(pos >= m_data && pos <= m_end && "position outside of data");
+        m_data = pos;
 
 #ifndef NDEBUG
         // In debug builds reset the tag to zero so that we can detect (some)
@@ -126,17 +142,19 @@ class pbf_reader {
     }
 
     pbf_length_type get_len_and_skip() {
-        const auto len = get_length();
-        skip_bytes(len);
-        return len;
+        const auto view = peek_length_delimited();
+        consume_until(view.data() + view.size());
+        return static_cast<pbf_length_type>(view.size());
     }
 
     template <typename T>
     iterator_range<T> get_packed() {
         protozero_assert(tag() != 0 && "call next() before accessing field value");
-        const auto len = get_len_and_skip();
-        return {T{m_data - len, m_data},
-                T{m_data, m_data}};
+        const auto view = peek_length_delimited();
+        const char* end = view.data() + view.size();
+        iterator_range<T> range{T{view.data(), end}, T{end, end}};
+        consume_until(end);
+        return range;
     }
 
 public:
@@ -286,17 +304,18 @@ class pbf_reader {
             return false;
         }
 
-        const auto value = get_varint<uint32_t>();
-        m_tag = static_cast<pbf_tag_type>(value >> 3U);
+        const char* data = m_data;
+        const auto value = static_cast<uint32_t>(decode_varint(&data, m_end));
+        const auto new_tag = static_cast<pbf_tag_type>(value >> 3U);
 
         // tags 0 and 19000 to 19999 are not allowed as per
         // https://developers.google.com/protocol-buffers/docs/proto#assigning-tags
-        if (m_tag == 0 || (m_tag >= 19000 && m_tag <= 19999)) {
+        if (new_tag == 0 || (new_tag >= 19000 && new_tag <= 19999)) {
             throw invalid_tag_exception{};
         }
 
-        m_wire_type = pbf_wire_type(value & 0x07U);
-        switch (m_wire_type) {
+        const auto new_wire_type = pbf_wire_type(value & 0x07U);
+        switch (new_wire_type) {
             case pbf_wire_type::varint:
             case pbf_wire_type::fixed64:
             case pbf_wire_type::length_delimited:
@@ -306,6 +325,9 @@ class pbf_reader {
                 throw unknown_pbf_wire_type_exception{};
         }
 
+        m_data = data;
+        m_tag = new_tag;
+        m_wire_type = new_wire_type;
         return true;
     }
 
@@ -341,12 +363,17 @@ class pbf_reader {
      * @post If it returns `true` there is a current field now with the given tag.
      */
     bool next(pbf_tag_type next_tag) {
-        while (next()) {
-            if (m_tag == next_tag) {
+        // Work on a copy, so that the reader is unchanged if skipping one
+        // of the fields throws.
+        pbf_reader reader{*this};
+        while (reader.next()) {
+            if (reader.m_tag == next_tag) {
+                *this = reader;
                 return true;
             }
-            skip();
+            reader.skip();
         }
+        *this = reader;
         return false;
     }
 
@@ -380,12 +407,17 @@ class pbf_reader {
      * @post If it returns `true` there is a current field now with the given tag.
      */
     bool next(pbf_tag_type next_tag, pbf_wire_type type) {
-        while (next()) {
-            if (m_tag == next_tag && m_wire_type == type) {
+        // Work on a copy, so that the reader is unchanged if skipping one
+        // of the fields throws.
+        pbf_reader reader{*this};
+        while (reader.next()) {
+            if (reader.m_tag == next_tag && reader.m_wire_type == type) {
+                *this = reader;
                 return true;
             }
-            skip();
+            reader.skip();
         }
+        *this = reader;
         return false;
     }
 
@@ -478,7 +510,7 @@ class pbf_reader {
                 skip_bytes(8);
                 break;
             case pbf_wire_type::length_delimited:
-                skip_bytes(get_length());
+                get_len_and_skip();
                 break;
             case pbf_wire_type::fixed32:
                 skip_bytes(4);

test/unit/CMakeLists.txt

@@ -10,6 +10,7 @@ set(UNIT_TESTS data_view
                basic
                buffer
                endian
+               exception_guarantee
                exceptions
                iterators
                varint

test/unit/test_exception_guarantee.cpp

@@ -0,0 +1,148 @@
+
+#include <test.hpp>
+
+// The pbf_reader promises the strong exception guarantee: If a method
+// throws, the reader is unchanged.
+
+namespace {
+
+    void require_unchanged(const protozero::pbf_reader& before, const protozero::pbf_reader& after) {
+        REQUIRE(after.length() == before.length());
+        REQUIRE(after.data().data() == before.data().data());
+        REQUIRE(after.tag() == before.tag());
+        REQUIRE(after.wire_type() == before.wire_type());
+    }
+
+} // anonymous namespace
+
+TEST_CASE("next() on invalid tag 0 does not change reader") {
+    // field 1 (varint) = 1, then tag 0 (fixed64)
+    const std::string buffer{"\x08\x01\x01\x00", 4};
+    protozero::pbf_reader item{buffer};
+    REQUIRE(item.next());
+    REQUIRE(item.get_int32() == 1);
+
+    const auto before = item;
+    REQUIRE_THROWS_AS(item.next(), protozero::invalid_tag_exception);
+    require_unchanged(before, item);
+}
+
+TEST_CASE("next() on reserved tag does not change reader") {
+    std::string buffer;
+    protozero::add_varint_to_buffer(&buffer, 19000U << 3U | 1U);
+    protozero::pbf_reader item{buffer};
+
+    const auto before = item;
+    REQUIRE_THROWS_AS(item.next(), protozero::invalid_tag_exception);
+    require_unchanged(before, item);
+}
+
+TEST_CASE("next() on unknown wire type does not change reader") {
+    // field 1 (varint) = 1, then field 1 with wire type 7
+    const std::string buffer{"\x08\x01\x0f", 3};
+    protozero::pbf_reader item{buffer};
+    REQUIRE(item.next());
+    REQUIRE(item.get_int32() == 1);
+
+    const auto before = item;
+    REQUIRE_THROWS_AS(item.next(), protozero::unknown_pbf_wire_type_exception);
+    require_unchanged(before, item);
+}
+
+TEST_CASE("next(tag) does not skip fields if a later field is invalid") {
+    // field 1 (varint) = 1, field 2 (length-delimited) with length 5, but
+    // only one byte of data
+    const std::string buffer{"\x08\x01\x12\x05x", 5};
+
+    SECTION("next(tag)") {
+        protozero::pbf_reader item{buffer};
+        const auto before = item;
+        REQUIRE_THROWS_AS(item.next(3), protozero::end_of_buffer_exception);
+        require_unchanged(before, item);
+    }
+
+    SECTION("next(tag, type)") {
+        protozero::pbf_reader item{buffer};
+        const auto before = item;
+        REQUIRE_THROWS_AS(item.next(3, protozero::pbf_wire_type::varint), protozero::end_of_buffer_exception);
+        require_unchanged(before, item);
+    }
+}
+
+TEST_CASE("length-delimited field with truncated data does not change reader") {
+    // field 1 (length-delimited) with length 5, but only one byte of data
+    const std::string buffer{"\x0a\x05x", 3};
+    protozero::pbf_reader item{buffer};
+    REQUIRE(item.next());
+    const auto before = item;
+
+    SECTION("skip") {
+        REQUIRE_THROWS_AS(item.skip(), protozero::end_of_buffer_exception);
+    }
+
+    SECTION("get_view") {
+        REQUIRE_THROWS_AS(item.get_view(), protozero::end_of_buffer_exception);
+    }
+
+#ifndef PROTOZERO_STRICT_API
+    SECTION("get_data") {
+        REQUIRE_THROWS_AS(item.get_data(), protozero::end_of_buffer_exception);
+    }
+#endif
+
+    SECTION("get_string") {
+        REQUIRE_THROWS_AS(item.get_string(), protozero::end_of_buffer_exception);
+    }
+
+    SECTION("get_message") {
+        REQUIRE_THROWS_AS(item.get_message(), protozero::end_of_buffer_exception);
+    }
+
+    SECTION("get_packed_int32") {
+        REQUIRE_THROWS_AS(item.get_packed_int32(), protozero::end_of_buffer_exception);
+    }
+
+    SECTION("get_packed_fixed32") {
+        REQUIRE_THROWS_AS(item.get_packed_fixed32(), protozero::end_of_buffer_exception);
+    }
+
+    require_unchanged(before, item);
+}
+
+TEST_CASE("packed varint field with truncated last varint does not change reader") {
+    // field 1 (length-delimited) with length 2, last byte has the
+    // continuation bit set
+    const std::string buffer{"\x0a\x02\x01\x80", 4};
+    protozero::pbf_reader item{buffer};
+    REQUIRE(item.next());
+    const auto before = item;
+
+    SECTION("get_packed_int32") {
+        REQUIRE_THROWS_AS(item.get_packed_int32(), protozero::end_of_buffer_exception);
+    }
+
+    SECTION("get_packed_sint64") {
+        REQUIRE_THROWS_AS(item.get_packed_sint64(), protozero::end_of_buffer_exception);
+    }
+
+    require_unchanged(before, item);
+}
+
+TEST_CASE("packed fixed field with invalid length does not change reader") {
+    // field 1 (length-delimited) with length 3, which is not a multiple of 4
+    const std::string buffer{"\x0a\x03\x01\x02\x03", 5};
+    protozero::pbf_reader item{buffer};
+    REQUIRE(item.next());
+    const auto before = item;
+
+    SECTION("get_packed_fixed32") {
+        REQUIRE_THROWS_AS(item.get_packed_fixed32(), protozero::invalid_length_exception);
+    }
+
+    SECTION("get_packed_double") {
+        REQUIRE_THROWS_AS(item.get_packed_double(), protozero::invalid_length_exception);
+    }
+
+    require_unchanged(before, item);
+}
+