diff --git a/CHANGELOG.md b/CHANGELOG.md index d968258..263821a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ number says nothing about any other port. See the Releases section of - **A decoder rejects `null` on a field with `(pxf.required)` or `@required`** (#131, decided by the owner; trendvidia/protowire#569, protowire-go#268). **This narrows what decodes.** A required field holds a value, so `null` is now rejected as absence is, whatever the field's type and presence. Before, `null` counted as present, so `r = null` on `optional int32 r [(pxf.required) = true]` decoded and left a required field with no value. The error is `null is not allowed on required field "r": a required field holds a value`, in a body, a nested body, a `@dataset` cell and a dotted cell, from `Unmarshal` as well as `UnmarshalFull`. A plain scalar with `(pxf.required)`, which already rejected `null`, now names the field as required instead of suggesting `optional`. A zero value still satisfies the field. A `_null` mask that names an unset required field no longer makes `AppendDatasetRow` write `null` for it: the writer refuses it, as it does without the mask. - **`pxf::Marshal` writes an unset field that has `(pxf.default)` or `(pxf.required)` so that it reads back** (#129, protowire-go#259; draft -01 § Common Schema Layer, "`decode_PXF(encode_PXF(v)) == v`"), as protowire-go does. This changes what Marshal writes for annotated fields, and narrows what encodes. Marshal left out every unset field, so a reader that applies defaults filled it in: `int32 n = 1 [(pxf.default) = "5"]` holding 0 read back as 5, and a required scalar holding 0 made the full decode refuse the document. A proto3 scalar without presence is now written as its zero value, a required list or map as `[]` or `{}`, and a message-typed field with a default, such as a wrapper or a Timestamp, as `null`. An unset `optional` or oneof scalar with a default is still left out and reads back as its default, the owner's choice in protowire-go#267. An unset `optional` field that is `(pxf.required)` is now an error, `pxf: is unset, and the field is (pxf.required)`; with `emit_defaults` it is written as zero, as before. An Any whose body has such a field stays as `type_url` and `value`. Inside a `@dataset` value cell, the same error now names the field by its full name. Unannotated fields are unchanged. +- **`sbe` tells two schemas' template ids apart** (#134, protowire-go#253 and #257; draft -01 § SBE Annotations, "sbe.template_id MUST be unique among messages within a schema", and § Header and Block Length). This narrows what binds and what decodes. `Codec::New` refuses two different messages with one `(sbe.template_id)` in one schema, with `sbe: messages A and B share (sbe.template_id) = 7 in schema 1; it must be unique within a schema`; the second used to replace the first, and `NewView` then read one message's bytes as the other's. A file passed twice still binds, and so do two schemas that each use a template id. `NewView` now finds a template by the header's schema id and template id together, and refuses an unknown pair with `sbe: unknown template ID 9 in schema 2`, even if another schema has that template id; it used to say `unknown template id 9`. `Unmarshal` and `UnmarshalDescriptor` refuse a header whose schema id is not the target message's, with `sbe: schema ID mismatch: got 1, want 2 for template 7`; the version is not checked. A caller that builds headers by hand must now set the schema id. - **`pxf::Marshal` refuses a message nested past `kMaxNestingDepth`** (#107, protowire#502), as protowire-go does (protowire-go#236), with `pxf: nesting depth exceeds MaxNestingDepth=100`. Depth is counted as a decoder counts it: the root is depth 0, and each `{` or `[` is one level, so a list of lists stops at 50. It used to write a document that a reader at the same limit refused. **This narrows what encodes.** `AppendDatasetRow` refuses too. An Any whose body would be too deep stays as `type_url` and `value`, so an Any's contents never cause a refusal; only an Any that itself sits past the limit does. An Any's value is also no longer cut off at libprotobuf's 100-level parse limit when the writer's limit is raised. `pb::Marshal` cannot refuse yet (#93). - **A schema that redefines a well-known type with the wrong shape is refused at bind time** (draft -01 §wkt-shapes, protowire#487, protowire-go#218). This narrows what binds. A message named, say, `pxf.Secret` or `google.protobuf.Timestamp` without the fields the codec reads now gives a violation of the new kind `ViolationKind::kWellKnownShape`. Copies of the canonical schemas are unaffected, and field numbers are not checked. The check also reaches a schema's own file at a `google/protobuf/` path, which the closure walk used to skip. diff --git a/include/protowire/sbe.h b/include/protowire/sbe.h index 3d49ded..94e0aa0 100644 --- a/include/protowire/sbe.h +++ b/include/protowire/sbe.h @@ -108,7 +108,17 @@ class Codec { size_t pos) const; std::unordered_map> by_name_; - std::unordered_map by_id_; + // A registered message, found by its schema id and template id together: + // a template id is unique only within a schema (draft -01 § SBE + // Annotations), so two schemas in one codec may each use one. + struct ByID { + std::string name; + MessageTemplate* tmpl = nullptr; + }; + static uint32_t IDKey(uint16_t schema_id, uint16_t template_id) { + return (static_cast(schema_id) << 16U) | static_cast(template_id); + } + std::unordered_map by_id_; int max_message_size_ = kMaxMessageSize; int max_repeated_ = kMaxRepeatedCount; check::Validator* validator_ = nullptr; diff --git a/src/sbe/codec.cc b/src/sbe/codec.cc index 0be9074..3a44c17 100644 --- a/src/sbe/codec.cc +++ b/src/sbe/codec.cc @@ -49,8 +49,21 @@ Status Codec::RegisterMessage(const pb::Descriptor* md, uint16_t schema_id, uint auto tmpl = BuildTemplate(md, schema_id, version); if (!tmpl.ok()) return tmpl.status(); auto owned = std::make_unique(std::move(tmpl).consume()); - by_id_[owned->template_id] = owned.get(); - by_name_[std::string(md->full_name())] = std::move(owned); + // draft -01 § SBE Annotations: "sbe.template_id MUST be unique among + // messages within a schema". A second message with the same id would + // replace the first in by_id_, and NewView would then read one as the + // other. The same message seen again, as when a file is passed twice, + // is not a duplicate. + std::string name(md->full_name()); + const uint32_t key = IDKey(schema_id, owned->template_id); + if (auto prev = by_id_.find(key); prev != by_id_.end() && prev->second.name != name) { + return Status::Error("sbe: messages " + prev->second.name + " and " + name + + " share (sbe.template_id) = " + std::to_string(owned->template_id) + + " in schema " + std::to_string(schema_id) + + "; it must be unique within a schema"); + } + by_id_[key] = ByID{name, owned.get()}; + by_name_[name] = std::move(owned); } for (int i = 0; i < md->nested_type_count(); ++i) { Status st = RegisterMessage(md->nested_type(i), schema_id, version); @@ -113,27 +126,28 @@ StatusOr Codec::NewView(std::span data) const { } uint16_t block_length = LoadU16(data.data()); uint16_t template_id = LoadU16(data.data() + 2); - auto it = by_id_.find(template_id); - if (it == by_id_.end()) { - return Status::Error("sbe: unknown template id " + std::to_string(template_id)); + uint16_t schema_id = LoadU16(data.data() + 4); + // The header's schema id and template id together name the template: a + // template id is unique only within a schema. + auto found = by_id_.find(IDKey(schema_id, template_id)); + if (found == by_id_.end()) { + return Status::Error("sbe: unknown template ID " + std::to_string(template_id) + " in schema " + + std::to_string(schema_id)); } + const MessageTemplate* tmpl = found->second.tmpl; // Schema evolution may push the wire block past the template's (newer // schemas append fields); a smaller one would underrun field reads. - if (block_length < it->second->block_length) { + if (block_length < tmpl->block_length) { return Status::Error("sbe: wire blockLength " + std::to_string(block_length) + - " < schema blockLength " + std::to_string(it->second->block_length) + + " < schema blockLength " + std::to_string(tmpl->block_length) + " for template " + std::to_string(template_id)); } size_t end = kHeaderSize + block_length; if (data.size() < end) { return Status::Error("sbe: data too short for root block"); } - if (Status st = ValidateGroups(data, *it->second, end); !st.ok()) return st; - return View(data, - data.subspan(kHeaderSize, block_length), - it->second, - &it->second->fields, - &it->second->groups); + if (Status st = ValidateGroups(data, *tmpl, end); !st.ok()) return st; + return View(data, data.subspan(kHeaderSize, block_length), tmpl, &tmpl->fields, &tmpl->groups); } } // namespace protowire::sbe diff --git a/src/sbe/unmarshal.cc b/src/sbe/unmarshal.cc index d9467bb..2f7be84 100644 --- a/src/sbe/unmarshal.cc +++ b/src/sbe/unmarshal.cc @@ -192,6 +192,15 @@ Status Codec::UnmarshalInto(std::span data, return Status::Error("sbe: template ID mismatch: got " + std::to_string(template_id) + ", want " + std::to_string(tmpl->template_id)); } + // A template id is unique only within a schema, so the template id alone + // does not say the bytes are this message's: they may be another schema's + // message with the same id. The version is not checked. + uint16_t schema_id = LoadU16(data.data() + 4); + if (schema_id != tmpl->schema_id) { + return Status::Error("sbe: schema ID mismatch: got " + std::to_string(schema_id) + ", want " + + std::to_string(tmpl->schema_id) + " for template " + + std::to_string(tmpl->template_id)); + } if (block_length < tmpl->block_length) { return Status::Error("sbe: wire blockLength " + std::to_string(block_length) + " < schema blockLength " + std::to_string(tmpl->block_length) + diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index b62a2dd..1cc89dc 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -63,7 +63,8 @@ endif() if(TARGET protowire_sbe) set(_sbe_test_srcs "") - foreach(f sbe_codec_test sbe_convert_test sbe_navigation_test sbe_stream_test) + foreach(f sbe_codec_test sbe_convert_test sbe_navigation_test sbe_stream_test + sbe_template_id_test) if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/${f}.cc) list(APPEND _sbe_test_srcs ${f}.cc) endif() diff --git a/test/sbe_files.h b/test/sbe_files.h new file mode 100644 index 0000000..6cb576d --- /dev/null +++ b/test/sbe_files.h @@ -0,0 +1,76 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 TrendVidia, LLC. +// +// SbeFiles compiles .proto sources written at runtime, each importing +// sbe/annotations.proto, and keeps the pool and a message factory alive for +// the test. It mirrors the in-memory compiles of protowire-go's SBE tests. + +#pragma once + +#include + +#include +#include +#include +#include +#include + +#include +#include + +#include "fixture_dir.h" +#include "protoc_compat.h" + +namespace protowire_test { + +class SbeFiles { + public: + // Compile writes each (name, source) pair into the fixture directory and + // imports them in order. It returns false, with the error in error(), if + // any fails to compile. + bool Compile(const std::vector>& srcs) { + tree_.MapPath("", PROTO_DIR); + tree_.MapPath("", WKT_PROTO_DIR); + tree_.MapPath("", FixtureDir()); + for (const auto& [name, src] : srcs) { + std::ofstream(FixtureDir() + "/" + name) << src; + } + importer_ = std::make_unique(&tree_, &errors_); + for (const auto& [name, src] : srcs) { + const google::protobuf::FileDescriptor* fd = importer_->Import(name); + if (fd == nullptr) return false; + files_.push_back(fd); + } + factory_ = std::make_unique(importer_->pool()); + return true; + } + + const std::vector& files() const { return files_; } + const std::string& error() const { return errors_.last_; } + + const google::protobuf::Descriptor* Message(const std::string& full_name) const { + return importer_->pool()->FindMessageTypeByName(full_name); + } + + std::unique_ptr New(const google::protobuf::Descriptor* md) { + return std::unique_ptr(factory_->GetPrototype(md)->New()); + } + + private: + class Errors : public google::protobuf::compiler::MultiFileErrorCollector { + public: + PROTOWIRE_PROTOC_RECORD_ERROR(filename, line, column, msg) { + last_ = std::string(filename) + ":" + std::to_string(line) + ":" + std::to_string(column) + + ": " + std::string(msg); + } + std::string last_; + }; + + google::protobuf::compiler::DiskSourceTree tree_; + Errors errors_; + std::unique_ptr importer_; + std::unique_ptr factory_; + std::vector files_; +}; + +} // namespace protowire_test diff --git a/test/sbe_template_id_test.cc b/test/sbe_template_id_test.cc new file mode 100644 index 0000000..4a309ea --- /dev/null +++ b/test/sbe_template_id_test.cc @@ -0,0 +1,222 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 TrendVidia, LLC. +// +// A template id is unique only within a schema (draft -01 § SBE +// Annotations). Ported from protowire-go's encoding/sbe/template_id_test.go +// (protowire-go#253, #257). + +#include + +#include +#include +#include +#include +#include + +#include "protowire/sbe.h" +#include "sbe_files.h" + +namespace { + +namespace pb = google::protobuf; +using protowire::sbe::Codec; +using protowire_test::SbeFiles; + +// IdFiles compiles one file per body, a.proto, b.proto, ..., each in its own +// package ids.a, ids.b, ... and with the schema id given for it. +bool IdFiles(SbeFiles* files, + const std::vector& schema_ids, + const std::vector& bodies) { + std::vector> srcs; + for (size_t i = 0; i < bodies.size(); ++i) { + const char c = static_cast('a' + i); + srcs.emplace_back(std::string(1, c) + ".proto", + "syntax = \"proto3\";\npackage ids." + std::string(1, c) + + ";\nimport \"sbe/annotations.proto\";\noption (sbe.schema_id) = " + + std::to_string(schema_ids[i]) + ";\noption (sbe.version) = 0;\n" + + bodies[i] + "\n"); + } + return files->Compile(srcs); +} + +uint16_t Le16(const std::vector& b, size_t at) { + return static_cast(b[at] | (b[at + 1] << 8U)); +} + +// TestTemplateIDUniqueInSchema: "sbe.template_id MUST be unique among +// messages within a schema". Before the fix Codec::New bound both messages, +// the second replaced the first in the id lookup, and NewView read A's bytes +// as B's. +TEST(SbeTemplateID, UniqueInSchema) { + struct Case { + const char* name; + std::vector schema_ids; + std::vector bodies; + const char* reject; // empty: binds + }; + const std::vector cases = { + {"two messages in one file", + {1}, + {"message A { option (sbe.template_id) = 7; uint32 x = 1; } message B { option " + "(sbe.template_id) = 7; uint64 y = 1; }"}, + "sbe: messages ids.a.A and ids.a.B share (sbe.template_id) = 7 in schema 1; it must be " + "unique within a schema"}, + {"two files of one schema", + {1, 1}, + {"message A { option (sbe.template_id) = 7; uint32 x = 1; }", + "message B { option (sbe.template_id) = 7; uint64 y = 1; }"}, + "sbe: messages ids.a.A and ids.b.B share (sbe.template_id) = 7 in schema 1"}, + {"a nested message", + {1}, + {"message A { option (sbe.template_id) = 7; uint32 x = 1; message N { option " + "(sbe.template_id) = 7; uint32 z = 1; } }"}, + "sbe: messages ids.a.A and ids.a.A.N share (sbe.template_id) = 7 in schema 1"}, + {"distinct ids", + {1}, + {"message A { option (sbe.template_id) = 7; uint32 x = 1; } message B { option " + "(sbe.template_id) = 8; uint64 y = 1; }"}, + ""}, + }; + for (const Case& tc : cases) { + SCOPED_TRACE(tc.name); + SbeFiles files; + if (!IdFiles(&files, tc.schema_ids, tc.bodies)) { + ADD_FAILURE() << files.error(); + continue; + } + auto codec = Codec::New(files.files()); + if (std::string(tc.reject).empty()) { + EXPECT_TRUE(codec.ok()) << codec.status().ToString(); + continue; + } + if (codec.ok()) { + ADD_FAILURE() << "bound; want " << tc.reject; + continue; + } + EXPECT_NE(codec.status().message().find(tc.reject), std::string::npos) + << codec.status().message(); + } +} + +// TestTemplateIDSameFileTwice: a file passed twice registers the same +// messages again, which is not a duplicate. +TEST(SbeTemplateID, SameFileTwice) { + SbeFiles files; + ASSERT_TRUE(IdFiles(&files, {1}, {"message A { option (sbe.template_id) = 7; uint32 x = 1; }"})) + << files.error(); + const pb::FileDescriptor* fd = files.files()[0]; + auto codec = Codec::New({fd, fd}); + ASSERT_TRUE(codec.ok()) << codec.status().ToString(); + + const pb::Descriptor* md = files.Message("ids.a.A"); + auto msg = files.New(md); + msg->GetReflection()->SetUInt32(msg.get(), md->FindFieldByName("x"), 42); + auto data = codec->Marshal(*msg); + ASSERT_TRUE(data.ok()) << data.status().ToString(); + auto v = codec->NewView(*data); + ASSERT_TRUE(v.ok()) << v.status().ToString(); + EXPECT_EQ(v->Uint("x"), 42u); +} + +// TestTemplateIDAcrossSchemas: two schemas may each use template 7 and both +// bind. NewView picks the template from the header's schemaId and templateId +// together. Before, it looked the id up alone and found whichever message +// was registered last. +TEST(SbeTemplateID, AcrossSchemas) { + SbeFiles files; + ASSERT_TRUE(IdFiles(&files, + {1, 2}, + {"message A { option (sbe.template_id) = 7; uint32 x = 1; message Only1 { " + "option (sbe.template_id) = 9; uint32 z = 1; } }", + "message B { option (sbe.template_id) = 7; uint64 y = 1; }"})) + << files.error(); + const pb::Descriptor* md_a = files.Message("ids.a.A"); + const pb::Descriptor* md_b = files.Message("ids.b.B"); + auto a = files.New(md_a); + a->GetReflection()->SetUInt32(a.get(), md_a->FindFieldByName("x"), 0xA); + auto b = files.New(md_b); + b->GetReflection()->SetUInt64(b.get(), md_b->FindFieldByName("y"), 0xB); + + // Registration order does not matter: each schema's bytes reach their own + // message whichever file came last. + const std::vector> orders = { + files.files(), {files.files()[1], files.files()[0]}}; + for (const auto& order : orders) { + auto codec = Codec::New(order); + ASSERT_TRUE(codec.ok()) << "two schemas may each use template 7: " << codec.status().ToString(); + auto data_a = codec->Marshal(*a); + ASSERT_TRUE(data_a.ok()) << data_a.status().ToString(); + auto data_b = codec->Marshal(*b); + ASSERT_TRUE(data_b.ok()) << data_b.status().ToString(); + EXPECT_EQ(Le16(*data_a, 4), 1) << "A's header names schema 1"; + EXPECT_EQ(Le16(*data_b, 4), 2) << "B's header names schema 2"; + + auto v = codec->NewView(*data_a); + ASSERT_TRUE(v.ok()) << v.status().ToString(); + EXPECT_EQ(v->Uint("x"), 0xAu); + v = codec->NewView(*data_b); + ASSERT_TRUE(v.ok()) << v.status().ToString(); + EXPECT_EQ(v->Uint("y"), 0xBu); + } + + // Template 9 exists in schema 1 only. A header naming it in schema 2 is + // refused, where an id-only lookup would have found it. + auto codec = Codec::New(files.files()); + ASSERT_TRUE(codec.ok()) << codec.status().ToString(); + auto only1 = files.New(files.Message("ids.a.A.Only1")); + auto data9 = codec->Marshal(*only1); + ASSERT_TRUE(data9.ok()) << data9.status().ToString(); + EXPECT_TRUE(codec->NewView(*data9).ok()); + (*data9)[4] = 2; + auto v = codec->NewView(*data9); + ASSERT_FALSE(v.ok()); + EXPECT_EQ(v.status().message(), "sbe: unknown template ID 9 in schema 2"); +} + +// TestUnmarshalChecksSchemaID: Unmarshal refuses bytes whose header names +// another schema than the target message's, even when the template id +// matches. A's block is no shorter than B's, so before the check A's bytes +// decoded as B with no error, B.y holding the low half of A.x. +TEST(SbeTemplateID, UnmarshalChecksSchemaID) { + SbeFiles files; + ASSERT_TRUE(IdFiles(&files, + {1, 2}, + {"message A { option (sbe.template_id) = 7; uint64 x = 1; }", + "message B { option (sbe.template_id) = 7; uint32 y = 1; }"})) + << files.error(); + const pb::Descriptor* md_a = files.Message("ids.a.A"); + const pb::Descriptor* md_b = files.Message("ids.b.B"); + auto codec = Codec::New(files.files()); + ASSERT_TRUE(codec.ok()) << codec.status().ToString(); + + auto a = files.New(md_a); + a->GetReflection()->SetUInt64(a.get(), md_a->FindFieldByName("x"), 0xA0000000AULL); + auto data_a = codec->Marshal(*a); + ASSERT_TRUE(data_a.ok()) << data_a.status().ToString(); + + const std::string want = "sbe: schema ID mismatch: got 1, want 2 for template 7"; + auto into_b = files.New(md_b); + EXPECT_EQ(codec->Unmarshal(*data_a, into_b.get()).message(), want); + auto got = codec->UnmarshalDescriptor(*data_a, md_b); + ASSERT_FALSE(got.ok()); + EXPECT_EQ(got.status().message(), want); + + // Its own message still reads it, and so does B once the header names + // schema 2: the check is the header's schema id, not the layout. + got = codec->UnmarshalDescriptor(*data_a, md_a); + ASSERT_TRUE(got.ok()) << got.status().ToString(); + EXPECT_EQ((*got)->GetReflection()->GetUInt64(**got, md_a->FindFieldByName("x")), 0xA0000000AULL); + (*data_a)[4] = 2; + got = codec->UnmarshalDescriptor(*data_a, md_b); + ASSERT_TRUE(got.ok()) << got.status().ToString(); + EXPECT_EQ((*got)->GetReflection()->GetUInt32(**got, md_b->FindFieldByName("y")), 0xAu); + + // A header with schema id 0, as an emitter that does not set it writes, is + // refused for a schema that declares another id. + (*data_a)[4] = 0; + auto into_a = files.New(md_a); + EXPECT_EQ(codec->Unmarshal(*data_a, into_a.get()).message(), + "sbe: schema ID mismatch: got 0, want 1 for template 7"); +} + +} // namespace