From a303198982779e6ce7c21ccbf10f1132bcc0dce3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Tue, 13 Oct 2026 13:36:01 +0200 Subject: [PATCH] Make pbf_reader actually provide the strong exception guarantee Several methods advanced the reader before all checks had passed, so a throwing call left the reader in a changed state, contrary to the class documentation: - next() on an invalid tag or wire type - get_view()/get_message()/skip() on a truncated length-delimited field - get_packed_*() on a truncated last varint or an invalid fixed length - next(tag) and next(tag, type) when skipping over an invalid field Length-delimited fields are now decoded and validated without changing the reader, which is only advanced once every check has passed. --- CHANGELOG.md | 1 + include/protozero/pbf_reader.hpp | 80 +++++++++---- test/unit/CMakeLists.txt | 1 + test/unit/test_exception_guarantee.cpp | 148 +++++++++++++++++++++++++ 4 files changed, 206 insertions(+), 24 deletions(-) create mode 100644 test/unit/test_exception_guarantee.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index f3757659b..227120426 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/include/protozero/pbf_reader.hpp b/include/protozero/pbf_reader.hpp index 42156daac..cce0d8f29 100644 --- a/include/protozero/pbf_reader.hpp +++ b/include/protozero/pbf_reader.hpp @@ -88,11 +88,12 @@ class pbf_reader { template iterator_range> 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(m_data - len), + consume_until(view.data() + view.size()); + return {const_fixed_iterator(view.data()), const_fixed_iterator(m_data)}; } @@ -108,15 +109,30 @@ class pbf_reader { return static_cast(decode_zigzag64(decode_varint(&m_data, m_end))); } - pbf_length_type get_length() { - return get_varint(); + // 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(decode_varint(&data, m_end)); + if (m_end - data < static_cast(len)) { + throw end_of_buffer_exception{}; + } + return {data, len}; } void skip_bytes(pbf_length_type len) { if (m_end - m_data < static_cast(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(view.size()); } template iterator_range 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 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(); - m_tag = static_cast(value >> 3U); + const char* data = m_data; + const auto value = static_cast(decode_varint(&data, m_end)); + const auto new_tag = static_cast(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); diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt index 0d1d75a3c..6a588267a 100644 --- a/test/unit/CMakeLists.txt +++ b/test/unit/CMakeLists.txt @@ -10,6 +10,7 @@ set(UNIT_TESTS data_view basic buffer endian + exception_guarantee exceptions iterators varint diff --git a/test/unit/test_exception_guarantee.cpp b/test/unit/test_exception_guarantee.cpp new file mode 100644 index 000000000..9d9919dfe --- /dev/null +++ b/test/unit/test_exception_guarantee.cpp @@ -0,0 +1,148 @@ + +#include + +// 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); +} +