Skip to content

pxf: bound a Decimal before rendering it - #56

Merged
trendvidia merged 3 commits into
mainfrom
fix/decimal-render-bound
Oct 31, 2026
Merged

trendvidia merged 3 commits into
mainfrom
fix/decimal-render-bound

Conversation

@bfrancojr

Copy link
Copy Markdown
Collaborator

Part of trendvidia/protowire#448. Rendering a stored pxf.Decimal to PXF had no bound, so a five-byte scale could stall or exhaust the encoder. This now matches protowire-go's readDecimalStr: a scale past MaxNumericLiteralDigits fails on either sign before any work, a BigInt or Decimal literal past the digit cap fails, and padding is linear. It reaches a message read by a protobuf runtime, which a PB decoder's own bound doesn't cover.

A negative scale also kept its digits but lost its value: 5 at scale -2 came out as 5. It now renders 500.

Checked

305 tests pass locally, 4 of them new. The HARDENING corpus passes on this branch. It doesn't yet test rendering, which is what the rest of #448 adds.

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
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
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.
@trendvidia
trendvidia merged commit 11b2c2d into main Oct 31, 2026
10 checks passed
@trendvidia
trendvidia deleted the fix/decimal-render-bound branch October 31, 2026 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants