From 76f7a4e7c109b91207e0e4c9def8f34ed22061fa Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Tue, 3 Nov 2026 02:08:33 -0800 Subject: [PATCH] sbe: diagnose a stale descriptor in Codec::New, as protowire-go does A file or message carrying its SBE option at the pre-v1.12.0 number is reported as stale instead of missing or silently skipped, and the missing-schema-id error uses Go's words. The check moves to annotations.cc, shared with ProtoToXML. --- CHANGELOG.md | 1 + include/protowire/sbe/annotations.h | 12 ++++ src/sbe/annotations.cc | 40 +++++++++++++ src/sbe/codec.cc | 10 +++- src/sbe/prototoxml.cc | 32 ----------- test/sbe_codec_test.cc | 89 +++++++++++++++++++++++++++++ 6 files changed, 150 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89bb2d8..d968258 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ number says nothing about any other port. See the Releases section of - **`pb::Unmarshal` refuses a `pxf.BigFloat` whose `prec` is past the precision-suffix bound** (#108, protowire#499). That is a `prec` whose literal needs more than `max_numeric_literal_digits` digits: above 13603 bits at the default, zeros included. It is checked before the value is built. An 11-byte message at prec 2^31-1 used to decode, and writing it back widened the mantissa to 256 MiB. **This rejects messages that were accepted before.** `UnmarshalOptions::max_numeric_literal_digits` moves the bound. `pb::Marshal` still writes a hand-built value past it, because it cannot refuse yet (#93). - **A `BigFloat` `prec` varint wider than 32 bits keeps its low 32 bits** (#106), as a protobuf reader does and protowire-go now does (protowire-go#230). It was clamped to 2^32-1, so 2^32 read as 2^32-1 bits where every other reader reads prec 0, a zero. **The same bytes now decode to a different value.** - **A PXF `BigFloat` literal wider than 2^31-1 bits is refused** (protowire-go#231), with Go's `precision p exceeds the widest mantissa big.Float can hold, 2147483647 bits`. Only a `max_numeric_literal_digits` raised past 646 million admits such a suffix. It used to build the mantissa: 256 MiB at 2^31 bits. A zero is still accepted. +- **`sbe::Codec::New` diagnoses a stale descriptor, in protowire-go's words** (#122). A file or message that carries `(sbe.schema_id)` or `(sbe.template_id)` at the pre-v1.12.0 option numbers (50100, 50200) now gets an error saying it must be recompiled. A stale message used to be skipped silently, as if it were not a template. **Error text changes:** a file with no schema id now says `sbe: file missing (sbe.schema_id) option`, as Go does. - **`sbe::Codec::Unmarshal` keeps the sign of a narrow signed encoding** (#120). An `int32` or `int64` field with `(sbe.encoding)` of `int8`, `int16` or `int32` decoded a negative value as a large positive one: -1234 in an `int16` came back as 64302. It is now sign-extended, as protowire-go and `sbe::View` read it. **Such values now decode differently.** - **A PXF Decimal `-0` or `-0.00` keeps its sign** (#110), as protowire-go reads and writes it. It was read with `negative` unset and written back as `0`. **The PB bytes for such a literal change** (`negative` is now set). The spec's write gate found the difference. - **`pxf::Marshal` writes a Duration past the literal's range as a block** (#116), as protowire-go does (protowire-go#226). A literal holds 64 bits of nanoseconds, about 292 years. A longer Duration was written as ``, which no reader accepts. It is now `f { seconds = 9500000000 }`, which keeps the exact value, in a field, list, map or `@dataset` cell. Shorter Durations are written as before. diff --git a/include/protowire/sbe/annotations.h b/include/protowire/sbe/annotations.h index a3c5e12..ec1df3a 100644 --- a/include/protowire/sbe/annotations.h +++ b/include/protowire/sbe/annotations.h @@ -37,4 +37,16 @@ std::optional GetFieldUint32Option(const google::protobuf::FieldDescri std::optional GetFieldStringOption(const google::protobuf::FieldDescriptor* fd, int field_number); +// Before protowire v1.12.0, (sbe.schema_id) and (sbe.template_id) were 50100 +// and 50200. A descriptor compiled then carries them as unknown option fields. +inline constexpr int kRetiredSchemaID = 50100; +inline constexpr int kRetiredTemplateID = 50200; + +// StaleFileError and StaleMessageError are protowire-go's diagnosis for a +// descriptor that carries its option at the retired number, or empty when it +// does not. A number the descriptor's pool declares as an extension is a known +// option, not a stale one. +std::string StaleFileError(const google::protobuf::FileDescriptor* fd); +std::string StaleMessageError(const google::protobuf::Descriptor* md); + } // namespace protowire::sbe diff --git a/src/sbe/annotations.cc b/src/sbe/annotations.cc index e14ed80..3632e73 100644 --- a/src/sbe/annotations.cc +++ b/src/sbe/annotations.cc @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -110,4 +111,43 @@ std::optional GetFieldStringOption(const pb::FieldDescriptor* fd, i return GetStringOption(fd->options(), field_number); } +namespace { + +// CarriesRetired reports whether opts, the options of a descriptor in pool, +// has field number as an unknown field the pool does not declare. +bool CarriesRetired(const google::protobuf::DescriptorPool* pool, + const google::protobuf::Message& opts, + int number) { + // The pool's own options type, which is not the generated one when the file + // was compiled with descriptor.proto in its pool. + const google::protobuf::Descriptor* type = pool->FindMessageTypeByName(opts.GetTypeName()); + if (type == nullptr) type = opts.GetDescriptor(); + if (pool->FindExtensionByNumber(type, number) != nullptr) return false; + const google::protobuf::UnknownFieldSet& unknown = opts.GetReflection()->GetUnknownFields(opts); + for (int i = 0; i < unknown.field_count(); ++i) { + if (unknown.field(i).number() == number) return true; + } + return false; +} + +std::string StaleText(const std::string& what, const char* option, int retired, int registered) { + return "sbe: " + what + " carries (sbe." + option + ") at retired option number " + + std::to_string(retired) + " (registered as " + std::to_string(registered) + + " since protowire v1.12.0): the descriptor predates the registered extension block and " + "must be recompiled against the current sbe/annotations.proto"; +} + +} // namespace + +std::string StaleFileError(const google::protobuf::FileDescriptor* fd) { + if (!CarriesRetired(fd->pool(), fd->options(), kRetiredSchemaID)) return ""; + return StaleText("file " + std::string(fd->name()), "schema_id", kRetiredSchemaID, kExtSchemaID); +} + +std::string StaleMessageError(const google::protobuf::Descriptor* md) { + if (!CarriesRetired(md->file()->pool(), md->options(), kRetiredTemplateID)) return ""; + return StaleText( + "message " + std::string(md->full_name()), "template_id", kRetiredTemplateID, kExtTemplateID); +} + } // namespace protowire::sbe diff --git a/src/sbe/codec.cc b/src/sbe/codec.cc index f11abe6..0be9074 100644 --- a/src/sbe/codec.cc +++ b/src/sbe/codec.cc @@ -26,7 +26,9 @@ StatusOr Codec::New(std::vector files, CodecOp for (const pb::FileDescriptor* fd : files) { auto schema = GetFileUint32Option(fd, kExtSchemaID); if (!schema.has_value()) { - return Status::Error(std::string(fd->name()) + " missing (sbe.schema_id) file option"); + if (std::string stale = StaleFileError(fd); !stale.empty()) return Status::Error(stale); + return Status::Error("sbe: file " + std::string(fd->name()) + + " missing (sbe.schema_id) option"); } auto version = GetFileUint32Option(fd, kExtVersion).value_or(0); for (int i = 0; i < fd->message_type_count(); ++i) { @@ -39,7 +41,11 @@ StatusOr Codec::New(std::vector files, CodecOp } Status Codec::RegisterMessage(const pb::Descriptor* md, uint16_t schema_id, uint16_t version) { - if (GetMessageUint32Option(md, kExtTemplateID).has_value()) { + if (!GetMessageUint32Option(md, kExtTemplateID).has_value()) { + // Only a message with a template_id is registered; one that carries it at + // the retired number is a stale descriptor, not a plain message. + if (std::string stale = StaleMessageError(md); !stale.empty()) return Status::Error(stale); + } else { auto tmpl = BuildTemplate(md, schema_id, version); if (!tmpl.ok()) return tmpl.status(); auto owned = std::make_unique(std::move(tmpl).consume()); diff --git a/src/sbe/prototoxml.cc b/src/sbe/prototoxml.cc index f0b6411..a80a87a 100644 --- a/src/sbe/prototoxml.cc +++ b/src/sbe/prototoxml.cc @@ -12,8 +12,6 @@ #include #include -#include -#include #include "protowire/sbe/annotations.h" #include "protowire/sbe/xml.h" @@ -27,36 +25,6 @@ namespace gpb = google::protobuf; using xml_names::SnakeToCamel; using xml_names::StripEnumPrefix; -// (sbe.schema_id)'s number before protowire v1.12.0. Nothing declares it any -// more, so it survives only in an options message's unknown fields. -constexpr int kRetiredSchemaID = 50100; - -// StaleFileError is the diagnosis for a file whose (sbe.schema_id) sits at the -// retired number, as protowire-go's staleFileError; empty when it does not. -std::string StaleFileError(const gpb::FileDescriptor* fd) { - // An extension the pool declares at that number is a known option, as Go's - // compiler stores it, even where libprotobuf keeps it as unknown bytes. - // The pool's own FileOptions, which is not the generated one when the file - // was compiled with descriptor.proto in its pool. - const gpb::Descriptor* file_options = - fd->pool()->FindMessageTypeByName("google.protobuf.FileOptions"); - if (file_options == nullptr) file_options = gpb::FileOptions::descriptor(); - if (fd->pool()->FindExtensionByNumber(file_options, kRetiredSchemaID) != nullptr) return ""; - const gpb::Message& opts = fd->options(); - const gpb::UnknownFieldSet& unknown = opts.GetReflection()->GetUnknownFields(opts); - for (int i = 0; i < unknown.field_count(); ++i) { - if (unknown.field(i).number() == kRetiredSchemaID) { - return "sbe: file " + std::string(fd->name()) + - " carries (sbe.schema_id) at retired option number " + - std::to_string(kRetiredSchemaID) + " (registered as " + std::to_string(kExtSchemaID) + - " since protowire v1.12.0): the descriptor predates the registered extension block " - "and " - "must be recompiled against the current sbe/annotations.proto"; - } - } - return ""; -} - // IsList is protoreflect's IsList: repeated and not a map. bool IsList(const gpb::FieldDescriptor* f) { return f->is_repeated() && !f->is_map(); diff --git a/test/sbe_codec_test.cc b/test/sbe_codec_test.cc index 02265a5..399a1e7 100644 --- a/test/sbe_codec_test.cc +++ b/test/sbe_codec_test.cc @@ -1,6 +1,7 @@ // SPDX-License-Identifier: MIT // Copyright (c) 2026 TrendVidia, LLC. #include "protowire/sbe.h" +#include "protowire/sbe/annotations.h" #include #include "fixture_dir.h" @@ -8,11 +9,13 @@ #include #include +#include #include #include #include #include +#include #include namespace { @@ -539,3 +542,89 @@ TEST(SbeSigned, NarrowSignedEncodingsKeepTheirSign) { } } // namespace + +namespace { + +// Codec::New diagnoses a descriptor compiled against the pre-v1.12.0 +// annotations, in protowire-go's words (#122). +const pb::FileDescriptor* BuildStale(pb::DescriptorPool* pool, bool stale_file, bool stale_msg) { + pb::FileDescriptorProto proto; + proto.set_name("stale.proto"); + proto.set_package("stale.v1"); + proto.set_syntax("proto3"); + pb::DescriptorProto* m = proto.add_message_type(); + m->set_name("M"); + pb::FieldDescriptorProto* f = m->add_field(); + f->set_name("a"); + f->set_number(1); + f->set_type(pb::FieldDescriptorProto::TYPE_INT32); + f->set_label(pb::FieldDescriptorProto::LABEL_OPTIONAL); + pb::FileOptions* fo = proto.mutable_options(); + if (stale_file) { + fo->GetReflection()->MutableUnknownFields(fo)->AddVarint(50100, 1); + } else { + fo->GetReflection()->MutableUnknownFields(fo)->AddVarint(protowire::sbe::kExtSchemaID, 1); + } + if (stale_msg) { + pb::MessageOptions* mo = m->mutable_options(); + mo->GetReflection()->MutableUnknownFields(mo)->AddVarint(50200, 1); + } + return pool->BuildFile(proto); +} + +TEST(SbeStale, CodecNewDiagnosesStaleDescriptors) { + { + pb::DescriptorPool pool; + const pb::FileDescriptor* fd = BuildStale(&pool, true, false); + ASSERT_NE(fd, nullptr); + auto codec = protowire::sbe::Codec::New({fd}); + ASSERT_FALSE(codec.ok()); + EXPECT_EQ(codec.status().message(), + "sbe: file stale.proto carries (sbe.schema_id) at retired option number 50100 " + "(registered as 1319 since protowire v1.12.0): the descriptor predates the " + "registered extension block and must be recompiled against the current " + "sbe/annotations.proto"); + } + { + pb::DescriptorPool pool; + const pb::FileDescriptor* fd = BuildStale(&pool, false, true); + ASSERT_NE(fd, nullptr); + auto codec = protowire::sbe::Codec::New({fd}); + ASSERT_FALSE(codec.ok()); + EXPECT_EQ(codec.status().message(), + "sbe: message stale.v1.M carries (sbe.template_id) at retired option number 50200 " + "(registered as 1321 since protowire v1.12.0): the descriptor predates the " + "registered extension block and must be recompiled against the current " + "sbe/annotations.proto"); + } + { // Neither: a plain message is skipped, as before. + pb::DescriptorPool pool; + const pb::FileDescriptor* fd = BuildStale(&pool, false, false); + ASSERT_NE(fd, nullptr); + EXPECT_TRUE(protowire::sbe::Codec::New({fd}).ok()); + } +} + +TEST(SbeStale, CodecNewMissingSchemaIDText) { + pb::compiler::DiskSourceTree tree; + tree.MapPath("", protowire_test::FixtureDir()); + tree.MapPath("", WKT_PROTO_DIR); + { + std::ofstream(protowire_test::FixtureDir() + "/declared50100.proto") + << "syntax = \"proto3\";\npackage d.v1;\nimport \"google/protobuf/descriptor.proto\";\n" + "extend google.protobuf.FileOptions { uint32 old_schema_id = 50100; }\n" + "option (old_schema_id) = 1;\nmessage M { int32 a = 1; }\n"; + } + CollectErrors errors; + pb::compiler::Importer importer(&tree, &errors); + const pb::FileDescriptor* fd = importer.Import("declared50100.proto"); + ASSERT_NE(fd, nullptr) << errors.last_; + // 50100 is declared here, so it is a known option, as Go's compiler stores + // it: the file is missing its schema id, not stale. + auto codec = protowire::sbe::Codec::New({fd}); + ASSERT_FALSE(codec.ok()); + EXPECT_EQ(codec.status().message(), + "sbe: file declared50100.proto missing (sbe.schema_id) option"); +} + +} // namespace