Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSON parsing of large 32-bit floating point values fails. #10509

Closed
jskeet opened this issue Sep 7, 2022 · 3 comments · Fixed by #10514
Closed

JSON parsing of large 32-bit floating point values fails. #10509

jskeet opened this issue Sep 7, 2022 · 3 comments · Fixed by #10514
Assignees

Comments

@jskeet
Copy link
Contributor

jskeet commented Sep 7, 2022

The proto3 JSON spec doesn't give very much guidance on exactly how floating point values are represented.

The maximal finite 32-bit floating point value is precisely 340282346638528859811704183484516925440. Both .NET and (I believe) Go format this as 3.4028235e+38.

Unfortunately that fails to round-trip in .NET, because of the way we handle JSON parsing of 32-bit floating point numbers:

  • Parse as a 64-bit float
  • Compare with the maximal 32-bit float - if it's less than or equal, proceed, otherwise throw an exception

There are 64-bit floating point values closer to 3.4028235e+38 than the max 32-bit value, so we end up throwing an exception.
We should, instead, try casting to float and see whether the result is infinite.

@jskeet
Copy link
Contributor Author

jskeet commented Sep 7, 2022

@mcy: This feels like something that could usefully be in the JSON conformance test suite. (I'm adding unit tests to the C# code of course.)

@haberman
Copy link
Member

haberman commented Sep 7, 2022

The conformance tests already have a "max float" case, but they are testing for 3.402823e+38 rather than 3.4028235e+38:

RunValidJsonTest(
"FloatFieldMaxPositiveValue", REQUIRED,
R"({"optionalFloat": 3.402823e+38})",
"optional_float: 3.402823e+38");

Also it appears the min float case has a copy-and-paste bug, where it's accidentally checking the max float value again:

RunValidJsonTest(
"FloatFieldMinNegativeValue", REQUIRED,
R"({"optionalFloat": 3.402823e+38})",
"optional_float: 3.402823e+38");

@jskeet
Copy link
Contributor Author

jskeet commented Sep 8, 2022

@haberman: Would you like me to make changes in that file in #10514, or should we keep that separate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants