Skip to content

Fix closing submessages with known size #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kkaefer wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix closing submessages with known size #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions include/protozero/basic_pbf_writer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ class basic_pbf_writer {

// This is usually 0. If there is an open submessage, this is set in the
// parent to the position where the data of the submessage is written to.
// If the size of the submessage was known when it was opened, this is
// set to the position where the data of the submessage must end instead.
std::size_t m_pos = 0;

void add_varint(uint64_t value) {
Expand Down Expand Up @@ -179,12 +181,13 @@ class basic_pbf_writer {
m_rollback_pos = buffer_customization<TBuffer>::size(m_data);
add_field(tag, pbf_wire_type::length_delimited);
buffer_customization<TBuffer>::append_zeros(m_data, std::size_t(reserve_bytes));
m_pos = buffer_customization<TBuffer>::size(m_data);
} else {
m_rollback_pos = size_is_known;
add_length_varint(tag, static_cast<pbf_length_type>(size));
reserve(size);
m_pos = buffer_customization<TBuffer>::size(m_data) + size;
}
m_pos = buffer_customization<TBuffer>::size(m_data);
}

void rollback_submessage() {
Expand All @@ -210,7 +213,14 @@ class basic_pbf_writer {

void close_submessage() {
protozero_assert(m_data);
if (m_pos == 0 || m_rollback_pos == size_is_known) {
if (m_pos == 0) {
return;
}
if (m_rollback_pos == size_is_known) {
const auto end_pos = m_pos;
m_pos = 0;
protozero_assert(buffer_customization<TBuffer>::size(m_data) == end_pos && "submessage with known size must have exactly that many bytes written");

@kkaefer kkaefer Oct 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we throw here instead?

@kkaefer kkaefer Oct 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess overall in protozero, we use asserts for API misuse rather than throwing exceptions.

(void)end_pos;
return;
}
if (buffer_customization<TBuffer>::size(m_data) - m_pos == 0) {
Expand Down
53 changes: 53 additions & 0 deletions test/t/repeated_packed_fixed32/reader_test_cases.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,56 @@ using cpp_type = uint32_t;

#include <packed_access.hpp>


TEST_CASE("write repeated packed fixed32 field using packed field with known size") {
std::string buffer;
protozero::pbf_writer pw{buffer};

SECTION("correct number of elements") {
{
protozero::packed_field_fixed32 field{pw, 1, 2};
field.add_element(17);
field.add_element(1);
}
pw.add_uint32(2, 5);

REQUIRE(buffer == std::string{"\x0a\x08\x11\x00\x00\x00\x01\x00\x00\x00\x10\x05", 12});
}

SECTION("correct number of elements with commit") {
protozero::packed_field_fixed32 field{pw, 1, 2};
field.add_element(17);
field.add_element(1);
field.commit();
pw.add_uint32(2, 5);

REQUIRE(buffer == std::string{"\x0a\x08\x11\x00\x00\x00\x01\x00\x00\x00\x10\x05", 12});
}

SECTION("too few elements") {
protozero::packed_field_fixed32 field{pw, 1, 2};
field.add_element(17);
REQUIRE_THROWS_AS(field.commit(), assert_error);
}

SECTION("too many elements") {
protozero::packed_field_fixed32 field{pw, 1, 2};
field.add_element(17);
field.add_element(1);
field.add_element(2);
REQUIRE_THROWS_AS(field.commit(), assert_error);
}
}

TEST_CASE("write submessage with known size") {
std::string buffer;
protozero::pbf_writer pw{buffer};

{
protozero::pbf_writer sw{pw, 1, 2};
sw.add_uint32(1, 5);
}
pw.add_uint32(2, 5);

REQUIRE(buffer == std::string{"\x0a\x02\x08\x05\x10\x05", 6});
}
Loading