Skip to content

Fix closing submessages with known size - #166

Open
kkaefer wants to merge 1 commit into
masterfrom
kk/fix-packed-known-size
Open

kkaefer wants to merge 1 commit into
masterfrom
kk/fix-packed-known-size

Conversation

@kkaefer

@kkaefer kkaefer commented Oct 14, 2026

Copy link
Copy Markdown
Member

When a submessage was opened with a known size (e.g. packed_field_fixed32 field{pw, 1, 2}, as in the tutorial), closing it never reset the parent's m_pos. Any later write to the parent then hit the "existing submessage" assert in debug builds.

Closing now resets the parent and asserts that exactly size bytes were written. Tests cover the correct, too-few and too-many element cases, plus a plain known-size submessage.

close_submessage() returned early for submessages opened with a known
size without resetting m_pos in the parent, so any further write to the
parent hit the "existing submessage" assert. This affected e.g.
packed_field_fixed32 with a size argument, as shown in the tutorial.

m_pos now holds the expected end position for such submessages, and
closing asserts that exactly that many bytes were written.
@kkaefer
kkaefer requested a review from a team as a code owner October 14, 2026 12:53
@kkaefer
kkaefer requested a review from joto October 14, 2026 12:53
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");

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?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant