Skip to content

json: reject overflowing quoted float values - #403

Open
fallintoplace wants to merge 1 commit into
anthropics:mainfrom
fallintoplace:fix/reject-quoted-float-overflow
Open

json: reject overflowing quoted float values#403
fallintoplace wants to merge 1 commit into
anthropics:mainfrom
fallintoplace:fix/reject-quoted-float-overflow

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

What changed

  • Fixes finite quoted float values like "3.5e38" being silently decoded as positive infinity.
  • Checks the f64 parse before converting to f32, matching the existing unquoted-number behavior.
  • Keeps "NaN", "Infinity", and "-Infinity" working.

Tests

  • Focused float helper tests
  • task lint
  • cargo test --workspace

@github-actions

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A larger quoted finite value still slips through: "1e400" parses to f64::INFINITY, so value.is_finite() is false and the new overflow guard accepts it as f32::INFINITY. Since the only valid non-finite strings are handled by the explicit "Infinity" / "-Infinity" branches above, should any non-finite result from the numeric parse path be rejected?

@iainmcgin

Copy link
Copy Markdown
Collaborator

[claude code]

The shape is right — same expectation string as visit_f64, Unexpected::Str, the three legal tokens matched first — but the guard has a hole in the very case it fixes, and three sibling parsers have the identical bug:

  • "1e400" still decodes to inf. str::parse::<f64> saturates on overflow rather than erroring (the repo relies on that itself at buffa/src/text/decoder.rs:334-337), so the parse yields inf, value.is_finite() is false, the && skips the guard, and the field becomes f32::INFINITY. Since the three non-finite tokens are already consumed by the arms above, any non-finite parse result is out of range by definition: parse straight to f32 and reject !value.is_finite(). That also drops the double rounding (f64 then as f32 can differ by an ulp from a correctly rounded f32 parse) and, as a side effect, rejects "inf"/"nan"/"-INF", which Rust's grammar accepts case-insensitively and proto3 JSON does not.
  • double has the same bug at buffa/src/json_helpers.rs:1309"1e400" is accepted. The test comment at tests.rs:1754 ("f64 has no overflow check (all JSON numbers fit in f64 domain)") is true of numbers and false of quoted strings.
  • The reflect codec parses quoted floats itself (buffa-descriptor/src/reflect/json.rs:747, parse_float_str at :754): Value::F32(parse_float_str(v)? as f32) has no range check, so DynamicMessage (the BUFFA_VIA_REFLECT conformance mode) still accepts "3.5e38". Its unquoted path (scalar_from_f64, :696) does check. Route the float case through json_helpers::float::deserialize or apply the same guard.
  • buffa/src/extension_registry.rs:526 / :558 are the last copy (string arm s.parse(), saturating; number arm as_f64() as f32 unchecked). If that is out of scope, say so in the fragment.

Smaller: the fragment bolds the whole sentence and lacks (#403) — house style is **lead** (#403). prose; the module doc at json_helpers.rs:1180-1186 should mention out-of-range rejection; and the two one-off tests would be rows in float_deserialize_table (tests.rs:1689), which already holds the unquoted ("1e300", None) — add "1e400" (the regression row for the first point), "inf"/"nan", "-0.0" sign preservation, and a subnormal such as "1e-45".

Pre-existing and separate: reflect's v.abs() > f64::from(f32::MAX) at json.rs:700 rejects 3.4028235e38, the shortest round-trip decimal of f32::MAX that serde_json emits — so a DynamicMessage holding f32::MAX serializes to JSON its own decoder refuses. Worth an issue rather than folding in here.

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.

3 participants