From 1c0504ce32f4804ba9971266bd44621c3c5e3b7e Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Sat, 31 Oct 2026 03:49:16 -0700 Subject: [PATCH 1/3] pxf: bound a Decimal before rendering it, and keep a negative scale Marshal padded a scale one digit at a time, so a scale near INT32_MAX never finished, and it dropped a negative scale, changing the value. The PB decoder's bound doesn't cover a message read by a protobuf runtime. Refuse past MaxNumericLiteralDigits on both signs before any work, as protowire-go does, and bound the literal. protowire#448 --- CHANGELOG.md | 9 +++++ include/protowire/pb_big.h | 11 ++++-- include/protowire/pxf/wellknown.h | 6 ++-- src/pb/big.cc | 57 ++++++++++++++++++++++++++----- src/pxf/encode.cc | 10 +++--- src/pxf/wellknown.cc | 4 +-- test/pb_test.cc | 12 +++---- test/pxf_bignum_test.cc | 52 ++++++++++++++++++++++++++++ 8 files changed, 133 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c4d4c2..f4b3942 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,15 @@ format changes. ### Fixed +- **`pxf::Marshal` refuses a `pxf.Decimal` past the bounds, and no longer hangs** (protowire#448). A scale near `INT32_MAX` padded one digit at a time, so five input bytes took the encoder effectively forever. A scale past `MaxNumericLiteralDigits` on either sign now fails before any work, as does a `BigInt` or `Decimal` literal past the digit cap. This reaches a message read by a protobuf runtime, which the PB decoder's own bound doesn't cover. +- **A negative `Decimal` scale keeps its value.** It was dropped, so `5` at scale `-2` was written as `5` rather than `500`. + +### Changed + +- **`FormatBigInt`, `FormatDecimal`, `ReadBigIntAsString` and `ReadDecimalAsString` return `StatusOr`,** so they can refuse a value. + +### Fixed + - **`(pxf.required)` with `(pxf.default)` on one field is a bind-time violation** (#50), as draft -01 now requires (protowire#453). `ValidateFile` reports it as `ViolationKind::kRequiredOption`, over the diff --git a/include/protowire/pb_big.h b/include/protowire/pb_big.h index a2a0f86..a32bc24 100644 --- a/include/protowire/pb_big.h +++ b/include/protowire/pb_big.h @@ -6,6 +6,8 @@ #include #include +#include "protowire/detail/status.h" + namespace protowire::pb { // Byte-backed BigInt. Magnitude is stored as unsigned big-endian bytes; sign @@ -42,8 +44,11 @@ struct BigFloat { bool ParseBigInt(const std::string& s, BigInt& out); bool ParseDecimal(const std::string& s, Decimal& out); -// String formatters (big number → decimal text). Always exact. -std::string FormatBigInt(const BigInt& v); -std::string FormatDecimal(const Decimal& v); +// String formatters (big number → decimal text). Exact, or an error: a value +// past MaxNumericLiteralDigits has no literal (HARDENING.md § Arbitrary- +// precision magnitudes). A Decimal's scale is bounded before any work, since +// it is four bytes whoever wrote the message chose. +StatusOr FormatBigInt(const BigInt& v); +StatusOr FormatDecimal(const Decimal& v); } // namespace protowire::pb diff --git a/include/protowire/pxf/wellknown.h b/include/protowire/pxf/wellknown.h index 8533bfc..119def3 100644 --- a/include/protowire/pxf/wellknown.h +++ b/include/protowire/pxf/wellknown.h @@ -10,6 +10,8 @@ #include #include +#include "protowire/detail/status.h" + namespace protowire::pxf { // Detection helpers for google.protobuf.* well-known types. @@ -42,8 +44,8 @@ bool SetBigIntFromString(google::protobuf::Message* msg, std::string_view litera bool SetDecimalFromString(google::protobuf::Message* msg, std::string_view literal); bool SetBigFloatFromString(google::protobuf::Message* msg, std::string_view literal); -std::string ReadBigIntAsString(const google::protobuf::Message& msg); -std::string ReadDecimalAsString(const google::protobuf::Message& msg); +StatusOr ReadBigIntAsString(const google::protobuf::Message& msg); +StatusOr ReadDecimalAsString(const google::protobuf::Message& msg); std::string ReadBigFloatAsString(const google::protobuf::Message& msg); // Locates the `_null` `google.protobuf.FieldMask` field on a descriptor used diff --git a/src/pb/big.cc b/src/pb/big.cc index 46b16ae..c02821b 100644 --- a/src/pb/big.cc +++ b/src/pb/big.cc @@ -136,27 +136,66 @@ bool ParseDecimal(const std::string& s, Decimal& out) { return true; } -std::string FormatBigInt(const BigInt& v) { +namespace { + +// The digits a literal is bounded by: leading zeros are not significant, so +// 0.000…5 at the largest scale counts one. +size_t SignificantDigits(const std::string& literal) { + size_t n = 0; + bool leading = true; + for (char c : literal) { + if (c < '0' || c > '9' || (leading && c == '0')) continue; + leading = false; + ++n; + } + return n; +} + +StatusOr BoundLiteral(const char* kind, std::string literal) { + if (literal.size() > static_cast(kMaxNumericLiteralDigits)) { + size_t n = SignificantDigits(literal); + if (n > static_cast(kMaxNumericLiteralDigits)) { + return Status::Error(std::string(kind) + " renders to " + std::to_string(n) + + " digits; MaxNumericLiteralDigits=" + + std::to_string(kMaxNumericLiteralDigits)); + } + } + return literal; +} + +} // namespace + +StatusOr FormatBigInt(const BigInt& v) { std::string digits = FormatMagnitude(v.abs); - if (v.negative && !v.abs.empty()) return "-" + digits; - return digits; + if (v.negative && !v.abs.empty()) digits.insert(digits.begin(), '-'); + return BoundLiteral("pxf.BigInt", std::move(digits)); } -std::string FormatDecimal(const Decimal& v) { +StatusOr FormatDecimal(const Decimal& v) { + // Compared as int64 so -scale is defined at INT32_MIN. + const int64_t scale = v.scale; + if (scale > kMaxNumericLiteralDigits || scale < -int64_t{kMaxNumericLiteralDigits}) { + return Status::Error("pxf.Decimal scale " + std::to_string(scale) + + " exceeds MaxNumericLiteralDigits=" + + std::to_string(kMaxNumericLiteralDigits)); + } std::string digits = FormatMagnitude(v.unscaled); std::string out; if (v.negative && !v.unscaled.empty()) out = "-"; - if (v.scale <= 0) { + if (scale <= 0) { out += digits; - return out; + // A negative scale means trailing zeros: unscaled 5, scale -2 is 500. + if (!v.unscaled.empty()) out.append(static_cast(-scale), '0'); + return BoundLiteral("pxf.Decimal", std::move(out)); } // Pad with leading zeros so we have at least scale+1 digits. - while (static_cast(digits.size()) <= v.scale) digits = "0" + digits; - size_t int_len = digits.size() - static_cast(v.scale); + const size_t want = static_cast(scale) + 1; + if (digits.size() < want) digits.insert(0, want - digits.size(), '0'); + size_t int_len = digits.size() - static_cast(scale); out += digits.substr(0, int_len); out += '.'; out += digits.substr(int_len); - return out; + return BoundLiteral("pxf.Decimal", std::move(out)); } // --- Wire-format nested-message helpers (called from pb.h templates) ----- diff --git a/src/pxf/encode.cc b/src/pxf/encode.cc index bff5e75..1da8164 100644 --- a/src/pxf/encode.cc +++ b/src/pxf/encode.cc @@ -278,12 +278,10 @@ Status Encoder::EncodeMessageValue(const Message& sub, int level) { EncodeScalarValue(sub, inner, /*idx=*/-1); return Status::OK(); } - if (IsBigInt(d)) { - out_ += ReadBigIntAsString(sub); - return Status::OK(); - } - if (IsDecimal(d)) { - out_ += ReadDecimalAsString(sub); + if (IsBigInt(d) || IsDecimal(d)) { + auto literal = IsBigInt(d) ? ReadBigIntAsString(sub) : ReadDecimalAsString(sub); + if (!literal.ok()) return literal.status(); + out_ += *literal; return Status::OK(); } if (IsBigFloat(d)) { diff --git a/src/pxf/wellknown.cc b/src/pxf/wellknown.cc index 248e85f..465d7cd 100644 --- a/src/pxf/wellknown.cc +++ b/src/pxf/wellknown.cc @@ -207,14 +207,14 @@ bool SetBigFloatFromString(pb::Message* msg, std::string_view literal) { return true; } -std::string ReadBigIntAsString(const pb::Message& msg) { +StatusOr ReadBigIntAsString(const pb::Message& msg) { protowire::pb::BigInt bi; bi.abs = GetBytes(msg, "abs"); bi.negative = GetBoolField(msg, "negative"); return protowire::pb::FormatBigInt(bi); } -std::string ReadDecimalAsString(const pb::Message& msg) { +StatusOr ReadDecimalAsString(const pb::Message& msg) { protowire::pb::Decimal d; d.unscaled = GetBytes(msg, "unscaled"); d.scale = GetInt32Field(msg, "scale"); diff --git a/test/pb_test.cc b/test/pb_test.cc index f1bedba..6e26f0a 100644 --- a/test/pb_test.cc +++ b/test/pb_test.cc @@ -167,36 +167,36 @@ TEST(Pb, ParseFormatBigInt) { BigInt b; ASSERT_TRUE(protowire::pb::ParseBigInt("12345678901234567890", b)); EXPECT_FALSE(b.negative); - EXPECT_EQ(protowire::pb::FormatBigInt(b), "12345678901234567890"); + EXPECT_EQ(*protowire::pb::FormatBigInt(b), "12345678901234567890"); BigInt n; ASSERT_TRUE(protowire::pb::ParseBigInt("-42", n)); EXPECT_TRUE(n.negative); - EXPECT_EQ(protowire::pb::FormatBigInt(n), "-42"); + EXPECT_EQ(*protowire::pb::FormatBigInt(n), "-42"); BigInt z; ASSERT_TRUE(protowire::pb::ParseBigInt("0", z)); EXPECT_FALSE(z.negative); EXPECT_TRUE(z.abs.empty()); - EXPECT_EQ(protowire::pb::FormatBigInt(z), "0"); + EXPECT_EQ(*protowire::pb::FormatBigInt(z), "0"); } TEST(Pb, ParseFormatDecimal) { Decimal d; ASSERT_TRUE(protowire::pb::ParseDecimal("3.14", d)); EXPECT_EQ(d.scale, 2); - EXPECT_EQ(protowire::pb::FormatDecimal(d), "3.14"); + EXPECT_EQ(*protowire::pb::FormatDecimal(d), "3.14"); Decimal small; ASSERT_TRUE(protowire::pb::ParseDecimal("0.05", small)); EXPECT_EQ(small.scale, 2); - EXPECT_EQ(protowire::pb::FormatDecimal(small), "0.05"); + EXPECT_EQ(*protowire::pb::FormatDecimal(small), "0.05"); Decimal neg; ASSERT_TRUE(protowire::pb::ParseDecimal("-1.000", neg)); EXPECT_TRUE(neg.negative); EXPECT_EQ(neg.scale, 3); - EXPECT_EQ(protowire::pb::FormatDecimal(neg), "-1.000"); + EXPECT_EQ(*protowire::pb::FormatDecimal(neg), "-1.000"); } struct WithZigZag { diff --git a/test/pxf_bignum_test.cc b/test/pxf_bignum_test.cc index 0e0b2b8..887e560 100644 --- a/test/pxf_bignum_test.cc +++ b/test/pxf_bignum_test.cc @@ -9,6 +9,7 @@ #include #include +#include #include #include @@ -121,6 +122,57 @@ price = -0.05 EXPECT_NE(out->find("price = -0.05"), std::string::npos); } +// Sets Wallet.price from raw fields, as a protobuf runtime leaves it. +static void SetPrice(pbuf::Message* msg, std::string unscaled, int32_t scale) { + const pbuf::Reflection* r = msg->GetReflection(); + pbuf::Message* price = r->MutableMessage(msg, msg->GetDescriptor()->FindFieldByName("price")); + const pbuf::Reflection* pr = price->GetReflection(); + const pbuf::Descriptor* pd = price->GetDescriptor(); + pr->SetString(price, pd->FindFieldByName("unscaled"), std::move(unscaled)); + pr->SetInt32(price, pd->FindFieldByName("scale"), scale); +} + +// A producer of PXF text fails on a value past a bound (HARDENING.md § +// Arbitrary-precision magnitudes), on both signs, before doing the work: the +// scale is four bytes and was a hang at INT32_MAX. +TEST_F(PxfBigNum, DecimalScalePastTheBoundIsRefused) { + for (int32_t scale : {4097, -4097, INT32_MAX, INT32_MIN}) { + auto msg = NewWallet(); + SetPrice(msg.get(), "\x19", scale); + auto out = protowire::pxf::Marshal(*msg); + ASSERT_FALSE(out.ok()) << "scale " << scale; + EXPECT_NE(out.status().message().find("exceeds MaxNumericLiteralDigits"), std::string::npos) + << out.status().message(); + } +} + +TEST_F(PxfBigNum, DecimalNegativeScaleIsTrailingZeros) { + auto msg = NewWallet(); + SetPrice(msg.get(), "\x05", -2); + auto out = protowire::pxf::Marshal(*msg); + ASSERT_TRUE(out.ok()) << out.status().ToString(); + EXPECT_NE(out->find("price = 500"), std::string::npos) << *out; +} + +// Scale 4096 is accepted, and leading zeros don't count toward the cap. +TEST_F(PxfBigNum, DecimalAtTheBoundRenders) { + auto msg = NewWallet(); + SetPrice(msg.get(), "\x05", 4096); + auto out = protowire::pxf::Marshal(*msg); + ASSERT_TRUE(out.ok()) << out.status().ToString(); + EXPECT_NE(out->find("price = 0." + std::string(4095, '0') + "5"), std::string::npos); +} + +// Trailing zeros are significant, so these have no literal. +TEST_F(PxfBigNum, DecimalLiteralPastTheCapIsRefused) { + auto msg = NewWallet(); + SetPrice(msg.get(), "\x0a", -4096); // 4098 digits + auto out = protowire::pxf::Marshal(*msg); + ASSERT_FALSE(out.ok()); + EXPECT_NE(out.status().message().find("renders to 4098 digits"), std::string::npos) + << out.status().message(); +} + TEST_F(PxfBigNum, BigFloatRoundTripsApprox) { // BigFloat parses through double — round-trip preserves value to within // double precision. Pick a clean dyadic fraction so the textual form From c91adb0bbbcb72c634a4807cbf25e675a07d275a Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Sat, 31 Oct 2026 03:56:58 -0700 Subject: [PATCH 2/3] check-decode: add pb-render, the corpus rows for a producer Reads the bytes with the stock protobuf runtime, which stores the fields and bounds nothing, then writes PXF. Bypassing the hardened decoder is the point: otherwise the row is refused before the encoder runs. protowire#448 --- cmd/check_decode/main.cc | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/cmd/check_decode/main.cc b/cmd/check_decode/main.cc index 7f65293..0cf01af 100644 --- a/cmd/check_decode/main.cc +++ b/cmd/check_decode/main.cc @@ -299,6 +299,37 @@ bool PxfDecode(std::string_view data, return true; } +// The producer's side of the bounds (HARDENING.md § Arbitrary-precision +// magnitudes): the stock protobuf runtime reads the bytes, storing the fields +// and bounding nothing, and they are then written as PXF. pb::Unmarshal is +// bypassed on purpose, or it would refuse the row and the encoder would go +// untested, which is how a hang here stayed hidden (protowire#448). +bool PbRender(const std::string& data, const std::string& schema, const std::string& proto_path, + std::string* err) { + if (proto_path.empty()) { + *err = "--proto is required for format=pb-render"; + return false; + } + pb::compiler::DiskSourceTree tree; + CollectErrors errors; + pb::compiler::Importer importer(&tree, &errors); + const pb::Descriptor* desc = LoadDescriptor(proto_path, schema, &importer, &tree, &errors, err); + if (desc == nullptr) return false; + + pb::DynamicMessageFactory factory(importer.pool()); + std::unique_ptr msg(factory.GetPrototype(desc)->New()); + if (!msg->ParseFromString(data)) { + *err = "protobuf runtime refused the input"; + return false; + } + auto out = protowire::pxf::Marshal(*msg); + if (!out.ok()) { + *err = std::string(out.status().message()); + return false; + } + return true; +} + template bool PbDecodeAs(std::string_view data, const Limits& lim, std::string* err) { T msg; @@ -427,6 +458,8 @@ int main(int argc, char** argv) { ok = PxfDecode(data, schema, proto_path, lim, &err); } else if (format == "pb") { ok = PbDecode(data, schema, lim, &err); + } else if (format == "pb-render") { + ok = PbRender(std::string(data), schema, proto_path, &err); } else if (format == "sbe") { ok = SbeDecode(data, schema, proto_path, lim, &err); } else if (format == "envelope") { From b6d8a32d4a50a93699d1869b68afbfa17be37a6f Mon Sep 17 00:00:00 2001 From: "Benedicto Franco Jr." Date: Sat, 31 Oct 2026 04:22:17 -0700 Subject: [PATCH 3/3] Fix clang-format and clang-tidy on the Decimal bound clang-tidy runs on headers with inferred flags, which don't always carry the include root, so pb_big.h's first project include failed to resolve. --- cmd/check_decode/main.cc | 4 +++- include/protowire/pb_big.h | 4 +++- src/pb/big.cc | 12 ++++++------ test/pxf_bignum_test.cc | 2 +- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/cmd/check_decode/main.cc b/cmd/check_decode/main.cc index 0cf01af..3bee281 100644 --- a/cmd/check_decode/main.cc +++ b/cmd/check_decode/main.cc @@ -304,7 +304,9 @@ bool PxfDecode(std::string_view data, // and bounding nothing, and they are then written as PXF. pb::Unmarshal is // bypassed on purpose, or it would refuse the row and the encoder would go // untested, which is how a hang here stayed hidden (protowire#448). -bool PbRender(const std::string& data, const std::string& schema, const std::string& proto_path, +bool PbRender(const std::string& data, + const std::string& schema, + const std::string& proto_path, std::string* err) { if (proto_path.empty()) { *err = "--proto is required for format=pb-render"; diff --git a/include/protowire/pb_big.h b/include/protowire/pb_big.h index a32bc24..912301c 100644 --- a/include/protowire/pb_big.h +++ b/include/protowire/pb_big.h @@ -6,7 +6,9 @@ #include #include -#include "protowire/detail/status.h" +// Relative, because clang-tidy runs on this header with flags it infers, and +// they do not always carry the include root. +#include "detail/status.h" namespace protowire::pb { diff --git a/src/pb/big.cc b/src/pb/big.cc index c02821b..e2071af 100644 --- a/src/pb/big.cc +++ b/src/pb/big.cc @@ -155,9 +155,9 @@ StatusOr BoundLiteral(const char* kind, std::string literal) { if (literal.size() > static_cast(kMaxNumericLiteralDigits)) { size_t n = SignificantDigits(literal); if (n > static_cast(kMaxNumericLiteralDigits)) { - return Status::Error(std::string(kind) + " renders to " + std::to_string(n) + - " digits; MaxNumericLiteralDigits=" + - std::to_string(kMaxNumericLiteralDigits)); + return Status::Error( + std::string(kind) + " renders to " + std::to_string(n) + + " digits; MaxNumericLiteralDigits=" + std::to_string(kMaxNumericLiteralDigits)); } } return literal; @@ -175,9 +175,9 @@ StatusOr FormatDecimal(const Decimal& v) { // Compared as int64 so -scale is defined at INT32_MIN. const int64_t scale = v.scale; if (scale > kMaxNumericLiteralDigits || scale < -int64_t{kMaxNumericLiteralDigits}) { - return Status::Error("pxf.Decimal scale " + std::to_string(scale) + - " exceeds MaxNumericLiteralDigits=" + - std::to_string(kMaxNumericLiteralDigits)); + return Status::Error( + "pxf.Decimal scale " + std::to_string(scale) + + " exceeds MaxNumericLiteralDigits=" + std::to_string(kMaxNumericLiteralDigits)); } std::string digits = FormatMagnitude(v.unscaled); std::string out; diff --git a/test/pxf_bignum_test.cc b/test/pxf_bignum_test.cc index 887e560..b29f4a2 100644 --- a/test/pxf_bignum_test.cc +++ b/test/pxf_bignum_test.cc @@ -7,9 +7,9 @@ #include #include "protoc_compat.h" +#include #include #include -#include #include #include