From d8e83e3c7e1b42cde5a6d76acd5b0ef5b6ca5c33 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Thu, 8 Oct 2026 15:11:20 +0200 Subject: [PATCH] Enforce parent tag type when creating a pbf_builder submessage The submessage constructor of basic_pbf_builder took the parent as a basic_pbf_writer& and accepted a tag of any type, so a tag from the wrong enum compiled silently and wrote the wrong field number. Add an overload taking a basic_pbf_builder parent. It is an exact match and therefore preferred over the derived-to-base conversion of the existing constructor, and a static_assert requires the tag to have the parent's enum type. Plain pbf_writer parents keep working unchanged. Adds compile-fail tests run through ctest to verify mismatched enum and plain integer tags are rejected. Fixes #155 --- CHANGELOG.md | 1 + doc/tutorial.md | 10 ++++ include/protozero/basic_pbf_builder.hpp | 21 ++++++-- test/CMakeLists.txt | 1 + test/compile_fail/CMakeLists.txt | 37 ++++++++++++++ .../builder_parent_integer_tag.cpp | 22 +++++++++ .../builder_parent_wrong_enum.cpp | 22 +++++++++ test/unit/CMakeLists.txt | 1 + test/unit/test_builder.cpp | 49 +++++++++++++++++++ 9 files changed, 161 insertions(+), 3 deletions(-) create mode 100644 test/compile_fail/CMakeLists.txt create mode 100644 test/compile_fail/builder_parent_integer_tag.cpp create mode 100644 test/compile_fail/builder_parent_wrong_enum.cpp create mode 100644 test/unit/test_builder.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c3e8a95f..7fcc08705 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ This project adheres to [Semantic Versioning](https://semver.org/). ### Changed - The packed varint iterators (`const_varint_iterator`, `const_svarint_iterator`) now only store a single pointer instead of two, which speeds up decoding by 30-150%, depending on varint length. The constructor now validates once that the byte range ends on a varint boundary and stores only the data pointer. A packed field whose last byte still has its continuation bit set (i.e. a truncated trailing varint, which is illegal) now throws `end_of_buffer_exception` when the iterator range is created (from `get_packed_*()`) rather than later during iteration. +- The `basic_pbf_builder` constructor for submessages now checks that, when the parent is a `basic_pbf_builder`, the tag has the parent's enum type. Passing a tag of a different enum type or a plain integer tag is now a compile error. If you really need an untyped tag, cast the parent to `basic_pbf_writer&` first. (#155) ### Fixed diff --git a/doc/tutorial.md b/doc/tutorial.md index cccb69af4..a1c138d8e 100644 --- a/doc/tutorial.md +++ b/doc/tutorial.md @@ -622,5 +622,15 @@ instantiated using the same `enum class` described above and used exactly like the `pbf_writer` class but using the values of the enum instead of bare integers. +When creating a `pbf_builder` for a submessage from a parent `pbf_builder`, +the tag must be a value of the parent's enum type. Using a tag from a different +enum or a plain integer will not compile: + +```cpp +protozero::pbf_builder outer{buffer}; +protozero::pbf_builder inner{outer, Outer::sub}; // ok +protozero::pbf_builder wrong{outer, Inner::x}; // compile error +``` + See the `test/t/complex` test case for a complete example using this interface. diff --git a/include/protozero/basic_pbf_builder.hpp b/include/protozero/basic_pbf_builder.hpp index ce7640d43..b2ea10685 100644 --- a/include/protozero/basic_pbf_builder.hpp +++ b/include/protozero/basic_pbf_builder.hpp @@ -60,10 +60,10 @@ class basic_pbf_builder : public basic_pbf_writer { } /** - * Construct a pbf_builder for a submessage from the pbf_message or - * pbf_writer of the parent message. + * Construct a pbf_builder for a submessage from the pbf_writer of the + * parent message. * - * @param parent_writer The parent pbf_message or pbf_writer + * @param parent_writer The parent pbf_writer * @param tag Tag of the field that will be written */ template @@ -71,6 +71,21 @@ class basic_pbf_builder : public basic_pbf_writer { basic_pbf_writer{parent_writer, pbf_tag_type(tag)} { } + /** + * Construct a pbf_builder for a submessage from the pbf_builder of the + * parent message. The tag must be of the enum type of the parent + * pbf_builder, otherwise this will not compile. + * + * @param parent_builder The parent pbf_builder + * @param tag Tag of the field that will be written + */ + template + basic_pbf_builder(basic_pbf_builder& parent_builder, Q tag) : + basic_pbf_writer{parent_builder, pbf_tag_type(tag)} { + static_assert(std::is_same::value, + "tag must be of the parent builder's enum type"); + } + /// @cond INTERNAL #define PROTOZERO_WRITER_WRAP_ADD_SCALAR(name, type) \ void add_##name(T tag, type value) { \ diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b9a8c2dac..7c012cc0a 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -10,6 +10,7 @@ include_directories(SYSTEM "${CMAKE_CURRENT_SOURCE_DIR}/catch") include_directories("${CMAKE_CURRENT_SOURCE_DIR}/include") add_subdirectory(unit) +add_subdirectory(compile_fail) set(TEST_DIRS alignment bool diff --git a/test/compile_fail/CMakeLists.txt b/test/compile_fail/CMakeLists.txt new file mode 100644 index 000000000..0029e96e2 --- /dev/null +++ b/test/compile_fail/CMakeLists.txt @@ -0,0 +1,37 @@ +#----------------------------------------------------------------------------- +# +# CMake config +# +# protozero compile failure tests +# +# Each source file in here must fail to compile with the expected error +# message. This is used to test that misuse of the API is caught at compile +# time, for instance by a static_assert. +# +# To add a test, create a new source file and add its name (without the +# .cpp suffix) to COMPILE_FAIL_TESTS below. +# +#----------------------------------------------------------------------------- + +set(COMPILE_FAIL_TESTS builder_parent_wrong_enum + builder_parent_integer_tag) + +foreach(_test IN LISTS COMPILE_FAIL_TESTS) + # Compile-only target (no main() needed), excluded from the normal build. + add_library(compile_fail_${_test} OBJECT EXCLUDE_FROM_ALL ${_test}.cpp) + set_target_properties(compile_fail_${_test} PROPERTIES EXCLUDE_FROM_DEFAULT_BUILD ON) + + # The test builds the target above, so its output is the compiler output. + add_test(NAME compile_fail_${_test} + COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR} + --target compile_fail_${_test} + --config $) + + # Pass only if the expected error shows up (the exit code is ignored). + # Unlike WILL_FAIL, this doesn't accept unrelated compile errors. + set_tests_properties(compile_fail_${_test} PROPERTIES + PASS_REGULAR_EXPRESSION "tag must be of the parent builder's enum type") +endforeach() + + +#----------------------------------------------------------------------------- diff --git a/test/compile_fail/builder_parent_integer_tag.cpp b/test/compile_fail/builder_parent_integer_tag.cpp new file mode 100644 index 000000000..d47f58051 --- /dev/null +++ b/test/compile_fail/builder_parent_integer_tag.cpp @@ -0,0 +1,22 @@ + +// This file must not compile: A pbf_builder parent requires a tag of its enum +// type, not a plain integer. + +#include + +#include + +enum class Outer : protozero::pbf_tag_type { + sub = 1 +}; + +enum class Inner : protozero::pbf_tag_type { + value = 2 +}; + +void test() { + std::string buffer; + protozero::pbf_builder outer{buffer}; + protozero::pbf_builder inner{outer, 1}; +} + diff --git a/test/compile_fail/builder_parent_wrong_enum.cpp b/test/compile_fail/builder_parent_wrong_enum.cpp new file mode 100644 index 000000000..e6a3523f7 --- /dev/null +++ b/test/compile_fail/builder_parent_wrong_enum.cpp @@ -0,0 +1,22 @@ + +// This file must not compile: The tag used to open a submessage must have the +// enum type of the parent pbf_builder. + +#include + +#include + +enum class Outer : protozero::pbf_tag_type { + sub = 1 +}; + +enum class Inner : protozero::pbf_tag_type { + value = 2 +}; + +void test() { + std::string buffer; + protozero::pbf_builder outer{buffer}; + protozero::pbf_builder inner{outer, Inner::value}; +} + diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt index 0d1d75a3c..9979e4edd 100644 --- a/test/unit/CMakeLists.txt +++ b/test/unit/CMakeLists.txt @@ -9,6 +9,7 @@ set(UNIT_TESTS data_view basic buffer + builder endian exceptions iterators diff --git a/test/unit/test_builder.cpp b/test/unit/test_builder.cpp new file mode 100644 index 000000000..01d2b7db5 --- /dev/null +++ b/test/unit/test_builder.cpp @@ -0,0 +1,49 @@ + +#include + +#include + +namespace { + +enum class Outer : protozero::pbf_tag_type { + sub = 1 +}; + +enum class Inner : protozero::pbf_tag_type { + value = 2 +}; + +const std::string expected{"\x0a\x02\x10\x2a"}; + +} // anonymous namespace + +TEST_CASE("pbf_builder submessage from pbf_builder parent with matching tag type") { + std::string buffer; + { + protozero::pbf_builder outer{buffer}; + protozero::pbf_builder inner{outer, Outer::sub}; + inner.add_uint32(Inner::value, 42); + } + REQUIRE(buffer == expected); +} + +TEST_CASE("pbf_builder submessage from pbf_writer parent with integer tag") { + std::string buffer; + { + protozero::pbf_writer outer{buffer}; + protozero::pbf_builder inner{outer, 1}; + inner.add_uint32(Inner::value, 42); + } + REQUIRE(buffer == expected); +} + +TEST_CASE("pbf_builder submessage from pbf_builder parent cast to pbf_writer") { + std::string buffer; + { + protozero::pbf_builder outer{buffer}; + protozero::pbf_builder inner{static_cast(outer), 1}; + inner.add_uint32(Inner::value, 42); + } + REQUIRE(buffer == expected); +} +