diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fb397f..d78a014 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -97,6 +97,7 @@ number says nothing about any other port. See the Releases section of - **A bool default is exactly `true` or `false`** (protowire-go#90). Any other spelling silently became `false`; it now fails the decode. **A schema with `(pxf.default) = "True"` now fails `UnmarshalFull`.** - **`ViolationKindName` and `Violation::ToString` use protowire-go's text:** `keyed field option`, `default field option`, `required field option`, and `invalid (pxf.key) = "..."`. - **`AppendDatasetRow` writes an empty `(pxf.required)` repeated field or map as `[]` or `{}`** (protowire-go#219). The reader counts both as supplied. This port returned `unsupported kind`, and inside a block cell it left the field out, so the reader refused the row. +- **A PXF `BigFloat` literal whose exponent less its `prec` is below `INT32_MIN` is refused** (#125, protowire-go#231), with Go's `big float literal is below the wire's exponent range`. The wire carries that number as an `int32`, and it used to wrap, so `0.125p2147483647` was stored as a different value. Both exponent checks now run before the value is widened to `prec` bits, so such a literal is refused at once instead of after building a 256 MiB mantissa. Only a `max_numeric_literal_digits` raised past 646 million admits a `prec` that wide. ### Changed diff --git a/src/pxf/bigfloat.cc b/src/pxf/bigfloat.cc index 7a42607..c0d7548 100644 --- a/src/pxf/bigfloat.cc +++ b/src/pxf/bigfloat.cc @@ -89,6 +89,19 @@ class Nat { } } + // Sub returns a - b. a must not be less than b. + static Nat Sub(const Nat& a, const Nat& b) { + Nat out = a; + int64_t borrow = 0; + for (size_t i = 0; i < out.w_.size(); ++i) { + const int64_t t = int64_t{out.w_[i]} - (i < b.w_.size() ? int64_t{b.w_[i]} : 0) - borrow; + out.w_[i] = static_cast(t); + borrow = t < 0 ? 1 : 0; + } + out.Trim(); + return out; + } + // DivSmall divides in place and returns the remainder. uint32_t DivSmall(uint32_t d) { uint64_t rem = 0; @@ -281,6 +294,35 @@ bool ExceedsBound(int64_t e, int limit) { return e > limit || e < -static_cast(limit); } +// RoundedBinaryExponent is the binary exponent e, value in [2^(e-1), 2^e), +// of num/den rounded to prec bits, ties to even, as ParseBigFloatLiteral +// rounds it. It never builds a prec-bit number, so a wide prec costs +// nothing here. num and den must not be zero. +// +// The exact value's exponent comes from the two bit lengths and one +// comparison. Rounding raises it by one exactly when the value rounds up to +// 2^e: when it is at least 2^e (1 - 2^-(prec+1)), the tie between 2^e and +// the largest prec-bit value below it, whose mantissa is odd. With A the +// integer for 2^e and B the one for the value on a common scale, and +// g = A - B > 0, that is g 2^(prec+1) <= A. Since g >= 1, it cannot hold +// once prec + 1 reaches A's bit length, so the shift below is never wider +// than A, whose size the literal bounds. +int64_t RoundedBinaryExponent(const Nat& num, const Nat& den, uint32_t prec) { + // num/den lies in (2^(e-1), 2^(e+1)) for e the difference of bit lengths. + int64_t e = static_cast(num.BitLen()) - static_cast(den.BitLen()); + const auto scaled = [&](int64_t at) { + // A for 2^at, and B for the value, both scaled to integers. + return std::pair{at >= 0 ? den.Shl(static_cast(at)) : den, + at >= 0 ? num : num.Shl(static_cast(-at))}; + }; + auto [a, b] = scaled(e); + if (Nat::Cmp(b, a) >= 0) std::tie(a, b) = scaled(++e); + const uint64_t shift = uint64_t{prec} + 1; + if (shift >= a.BitLen()) return e; + const bool carries = Nat::Cmp(Nat::Sub(a, b).Shl(shift), a) <= 0; + return carries ? e + 1 : e; +} + // ParsePrecision reads a suffix's digits. Longer than any uint32 is refused // on its length, so `1p999…` costs nothing. bool ParsePrecision(std::string_view digits, uint32_t* out) { @@ -441,6 +483,21 @@ Status ParseBigFloatLiteral(std::string_view literal, } else { den = Nat::Pow10(-lit.exp10); } + // Both limits are on the value rounded to prec, and both are asked before + // the value is widened to prec bits, which for a wide prec is the costly + // part. The wire carries the exponent less prec as an int32 + // (pxf/bignum.proto), so a small value at a wide prec has no encoding + // (protowire-go#231). protowire-go asks this one first. + const int64_t bin = RoundedBinaryExponent(num, den, prec); + if (bin - static_cast(prec) < std::numeric_limits::min()) { + return Status::Error("big float literal " + Clip(literal) + + " is below the wire's exponent range"); + } + if (ExceedsBound(bin, max_exponent)) { + return Status::Error( + "big float literal " + Clip(literal) + " has binary exponent " + std::to_string(bin) + + ", and MaxBigFloatExponent=" + std::to_string(max_exponent) + " bounds its magnitude"); + } // Scale so the integer quotient has at least prec + 2 bits; the remainder // then only ever acts as a sticky bit. const int64_t scale = static_cast(prec) + 2 - static_cast(num.BitLen()) + @@ -456,11 +513,11 @@ Status ParseBigFloatLiteral(std::string_view literal, q = q.Shr(1); ++exponent; } - const int64_t bin = BinaryExponent(q, exponent); - if (ExceedsBound(bin, max_exponent)) { + // RoundedBinaryExponent found this exponent without the division. + if (BinaryExponent(q, exponent) != bin) { return Status::Error( - "big float literal " + Clip(literal) + " has binary exponent " + std::to_string(bin) + - ", and MaxBigFloatExponent=" + std::to_string(max_exponent) + " bounds its magnitude"); + "big float literal " + Clip(literal) + ": internal error: rounded binary exponent " + + std::to_string(BinaryExponent(q, exponent)) + ", expected " + std::to_string(bin)); } out->mantissa = q.ToBytes(); out->exponent = exponent; diff --git a/test/pxf_bignum_test.cc b/test/pxf_bignum_test.cc index 66d1019..870e849 100644 --- a/test/pxf_bignum_test.cc +++ b/test/pxf_bignum_test.cc @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -300,4 +301,143 @@ rate = 0.5 EXPECT_NE(out->find("rate = 0.5p256"), std::string::npos); } +// The wire carries a BigFloat's exponent less its prec as an int32 +// (pxf/bignum.proto), so a small value at a wide prec has no encoding, and +// protowire-go refuses it in these words (protowire-go#231). 0.125 is +// 2^-3, binary exponent -2, and -2 - (2^31 - 1) is below INT32_MIN. The +// refusal comes before the value is widened to prec bits, 256 MiB here, so +// this test costs what any other does. +TEST_F(PxfBigNum, BigFloatBelowTheWiresExponentRangeIsRefused) { + protowire::pxf::UnmarshalOptions opts; + opts.max_numeric_literal_digits = 2000000000; + for (const char* lit : {"0.125", "-0.125", "1e-300", "3e-4900"}) { + SCOPED_TRACE(lit); + auto msg = NewWallet(); + auto st = + protowire::pxf::Unmarshal(std::string("rate = ") + lit + "p2147483647\n", msg.get(), opts); + ASSERT_FALSE(st.ok()); + EXPECT_NE(st.message().find(std::string("big float literal ") + lit + + " is below the wire's exponent range"), + std::string::npos) + << st.ToString(); + // No exponent is stored, wrapped or otherwise. + const auto& rate = msg->GetReflection()->GetMessage(*msg, desc_->FindFieldByName("rate")); + EXPECT_EQ(rate.ByteSizeLong(), 0u); + } + // protowire-go asks this before MaxBigFloatExponent, so a value past both + // gets this refusal: 0.125 has binary exponent -2, past a bound of 1. + opts.max_big_float_exponent = 1; + auto msg = NewWallet(); + auto st = protowire::pxf::Unmarshal("rate = 0.125p2147483647\n", msg.get(), opts); + EXPECT_NE(st.message().find("is below the wire's exponent range"), std::string::npos) + << st.ToString(); +} + +// DecimalTimes multiplies a decimal digit string by a small number. +std::string DecimalTimes(const std::string& d, uint32_t m) { + std::string out; + uint64_t carry = 0; + for (size_t i = d.size(); i > 0; --i) { + const uint64_t v = uint64_t(d[i - 1] - '0') * m + carry; + out.push_back(static_cast('0' + v % 10)); + carry = v / 10; + } + for (; carry != 0; carry /= 10) out.push_back(static_cast('0' + carry % 10)); + while (out.size() > 1 && out.back() == '0') out.pop_back(); + return std::string(out.rbegin(), out.rend()); +} + +// DyadicLiteral writes m 2^j exactly: m 5^-j e j for a negative j. A +// non-zero nudge moves it by one unit in a decimal place past its last +// digit, down for a negative nudge and up for a positive one. That is +// 10^(j-1) for a negative j and 0.1 otherwise, either way less than 2^(j+1), +// so a tie nudged this way stays nearer the tie than any other boundary. +std::string DyadicLiteral(uint64_t m, int j, int nudge) { + std::string d = std::to_string(m); + int exp10 = 0; + for (int i = 0; i < (j > 0 ? j : -j); ++i) d = DecimalTimes(d, j > 0 ? 2 : 5); + if (j < 0) exp10 = j; + if (nudge != 0) { + // d - 0.1 or d + 0.1 in the last place: one more digit. + if (nudge < 0) { + size_t i = d.size(); + while (d[i - 1] == '0') d[--i] = '9'; + d[i - 1] = static_cast(d[i - 1] - 1); + d += "9"; + } else { + d += "1"; + } + --exp10; + } + return d + "e" + std::to_string(exp10); +} + +// The parser asks both exponent limits of the value rounded to prec before +// it divides, from the literal's bit lengths and one comparison +// (#125). Rounding can carry the exponent one higher, and these cases are +// built around that carry, so their expected exponent does not come from +// the arithmetic under test: (2^(p+1) - 1) 2^(e-p-1) is the tie between the +// largest p-bit value below 2^e and 2^e itself, and rounds up to 2^e, +// binary exponent e + 1; a hair below the tie it stays at e, and a hair +// above it carries. Each is read, its exponent compared, and the bound +// probed at that exponent, which is where the early arithmetic answers. +TEST_F(PxfBigNum, BigFloatRoundedExponentIsFoundBeforeDividing) { + const auto* rate_fd = desc_->FindFieldByName("rate"); + const auto binary_exponent = [&](const pbuf::Message& wallet) { + const pbuf::Message& rate = wallet.GetReflection()->GetMessage(wallet, rate_fd); + const pbuf::Descriptor* d = rate.GetDescriptor(); + const std::string m = rate.GetReflection()->GetString(rate, d->FindFieldByName("mantissa")); + int64_t bits = 0; + if (!m.empty()) { + bits = int64_t(m.size() - 1) * 8; + for (auto top = static_cast(m[0]); top != 0; top >>= 1) ++bits; + } + return bits + rate.GetReflection()->GetInt32(rate, d->FindFieldByName("exponent")); + }; + const auto check = [&](const std::string& lit, uint32_t prec, int64_t want) { + SCOPED_TRACE(lit + "p" + std::to_string(prec)); + const std::string doc = "rate = " + lit + "p" + std::to_string(prec) + "\n"; + auto msg = NewWallet(); + auto st = protowire::pxf::Unmarshal(doc, msg.get()); + ASSERT_TRUE(st.ok()) << st.ToString(); + const int64_t bin = binary_exponent(*msg); + ASSERT_EQ(bin, want); + protowire::pxf::UnmarshalOptions at; + at.max_big_float_exponent = static_cast(bin < 0 ? -bin : bin); + auto ok = NewWallet(); + EXPECT_TRUE(protowire::pxf::Unmarshal(doc, ok.get(), at).ok()); + // A bound of 0 means the default, so the refusal is probed only above 1. + if (at.max_big_float_exponent <= 1) return; + --at.max_big_float_exponent; + auto past = NewWallet(); + auto refused = protowire::pxf::Unmarshal(doc, past.get(), at); + EXPECT_NE(refused.message().find("has binary exponent " + std::to_string(bin) + ","), + std::string::npos) + << refused.ToString(); + }; + + std::mt19937_64 rng(125); + int cases = 0; + for (int i = 0; i < 400; ++i) { + const uint32_t p = 1 + static_cast(rng() % 62); + const int e = static_cast(rng() % 601) - 300; + const uint64_t top = (uint64_t{1} << (p + 1)) - 1; // the tie's mantissa + const int j = e - static_cast(p) - 1; + check(DyadicLiteral(top, j, 0), p, e + 1); + check(DyadicLiteral(top, j, -1), p, e); + check(DyadicLiteral(top, j, +1), p, e + 1); + check(DyadicLiteral(1, e, 0), p, e + 1); // 2^e itself + cases += 4; + } + // Hand-picked: ties to even on either side, and no carry. + check("15.5", 4, 5); // 1111.1: the tie rounds up to 10000 + check("14.5", 4, 4); // 1110.1: the tie rounds down to 1110 + check("0.75", 1, 1); // 0.11: the tie rounds up to 1 + check("1023.4", 10, 10); + check("1023.6", 10, 11); + check("-0.99999", 3, 1); // past the tie at 0.9375: rounds to 1 + cases += 6; + EXPECT_EQ(cases, 1606); +} + } // namespace