pxf: bound a Decimal before rendering it

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<std::string>`,** 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

cmd/check_decode/main.cc

@@ -299,6 +299,39 @@ 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<pb::Message> 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 <class T>
 bool PbDecodeAs(std::string_view data, const Limits& lim, std::string* err) {
   T msg;
@@ -427,6 +460,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") {

include/protowire/pb_big.h

@@ -6,6 +6,10 @@
 #include <string>
 #include <vector>
 
+// 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 {
 
 // Byte-backed BigInt. Magnitude is stored as unsigned big-endian bytes; sign
@@ -42,8 +46,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<std::string> FormatBigInt(const BigInt& v);
+StatusOr<std::string> FormatDecimal(const Decimal& v);
 
 }  // namespace protowire::pb

include/protowire/pxf/wellknown.h

@@ -10,6 +10,8 @@
 #include <google/protobuf/descriptor.h>
 #include <google/protobuf/message.h>
 
+#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<std::string> ReadBigIntAsString(const google::protobuf::Message& msg);
+StatusOr<std::string> 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

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<std::string> BoundLiteral(const char* kind, std::string literal) {
+  if (literal.size() > static_cast<size_t>(kMaxNumericLiteralDigits)) {
+    size_t n = SignificantDigits(literal);
+    if (n > static_cast<size_t>(kMaxNumericLiteralDigits)) {
+      return Status::Error(
+          std::string(kind) + " renders to " + std::to_string(n) +
+          " digits; MaxNumericLiteralDigits=" + std::to_string(kMaxNumericLiteralDigits));
+    }
+  }
+  return literal;
+}
+
+}  // namespace
+
+StatusOr<std::string> 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<std::string> 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<size_t>(-scale), '0');
+    return BoundLiteral("pxf.Decimal", std::move(out));
   }
   // Pad with leading zeros so we have at least scale+1 digits.
-  while (static_cast<int32_t>(digits.size()) <= v.scale) digits = "0" + digits;
-  size_t int_len = digits.size() - static_cast<size_t>(v.scale);
+  const size_t want = static_cast<size_t>(scale) + 1;
+  if (digits.size() < want) digits.insert(0, want - digits.size(), '0');
+  size_t int_len = digits.size() - static_cast<size_t>(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) -----

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)) {

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<std::string> 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<std::string> ReadDecimalAsString(const pb::Message& msg) {
   protowire::pb::Decimal d;
   d.unscaled = GetBytes(msg, "unscaled");
   d.scale = GetInt32Field(msg, "scale");

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 {

test/pxf_bignum_test.cc

@@ -7,6 +7,7 @@
 #include <gtest/gtest.h>
 #include "protoc_compat.h"
 
+#include <cstdint>
 #include <cstdio>
 #include <cstring>
 #include <memory>
@@ -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