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

Roundtrip error while parsing "1000000000000000010E5" #465

Closed
nlohmann opened this issue Feb 20, 2017 · 6 comments
Closed

Roundtrip error while parsing "1000000000000000010E5" #465

nlohmann opened this issue Feb 20, 2017 · 6 comments

Comments

@nlohmann
Copy link
Owner

Google OSS-Fuzz found an error executing fuzzer-parse_json.cpp:

  1. input "1000000000000000010E5" is parsed to JSON value j1.
  2. j1 is dumped to string s1; result is "1e+23".
  3. s1 is parsed to JSON value j2; result is 9.9999999999999991E+22 (Xcode debugger).
  4. j2 is dumped to string s2; result is "9.999999999999999e+22".
  5. s1 and s2 are different.

This is an error in the roundtrip check.

Any ideas on this? (maybe related to #378, #362, and #454)

@nlohmann nlohmann added kind: bug state: help needed the issue needs help to proceed labels Feb 20, 2017
@nlohmann nlohmann changed the title Roudtrip error while parsing "1000000000000000010E5" Roundtrip error while parsing "1000000000000000010E5" Feb 20, 2017
@TurpentineDistillery
Copy link

I can reproduce the problem: "1e+23" roundtrips to "9.999999999999999e+22" with the new code, and back to "1e+23" with the old code.

I think this is the problem:

            static constexpr auto fmt = d == 6  ? "%.7g"
                                        : d == 15 ? "%.16g"
                                        : d == 16 ? "%.17g"
                                        : d == 17 ? "%.18g"
                                        :           "%.19g";
            // I'm not sure why we need to +1 the precision,
            // but without it there's a unit-test that fails
            // that asserts precision of the output

If I change the the values within the format string to the values of d, as one would expect in the first place, then the problem-case round-trips correctly. On the other hand, such change triggers a unit-test failure:

src/unit-inspection.cpp:257: FAILED:
  CHECK( ss.str() == "3.141592653589793" )
with expansion:
  "3.14159265358979" == "3.141592653589793"

@nlohmann
Copy link
Owner Author

After changing the precision, "3.141592653589793" roundtrips stably to "3.14159265358979". Not sure if this is correct though.

@TurpentineDistillery
Copy link

TurpentineDistillery commented Feb 20, 2017

It appears that it's the right thing to do, and it is like a problem related to the the unit-test.

    const char* d_str = "3.141592653589793";
    const double d = strtod(d_str, nullptr);
    assert(d == 3.141592653589793);
    
    const auto digits10 = numeric_limits<double>::digits10;
    cout << "digits10:" << digits10 << endl; //15
    cout.precision(digits10);
    cout << d << endl; // 3.14159265358979 (no 3 at the end)

    // with old code that used operator << internally
    // and with the code that uses snrpintf("%.15g");
    const nlohmann::json j = nlohmann::json::parse(d_str);
    std::cout << j << endl; // 3.14159265358979 (no 3 at the end)

@nlohmann
Copy link
Owner Author

So the code

            static constexpr auto fmt = d == 6  ? "%.7g"
                                        : d == 15 ? "%.16g"
                                        : d == 16 ? "%.17g"
                                        : d == 17 ? "%.18g"
                                        :           "%.19g";

would be changed to

            static constexpr auto fmt = d == 6  ? "%.6g"
                                        : d == 15 ? "%.15g"
                                        : d == 16 ? "%.16g"
                                        : "%.17g"; // we already assert that d can only be 6, 15, 16, or 17

?

@TurpentineDistillery
Copy link

Yes.

@nlohmann nlohmann self-assigned this Feb 20, 2017
@nlohmann nlohmann added confirmed and removed state: help needed the issue needs help to proceed labels Feb 20, 2017
@nlohmann nlohmann added this to the Release 2.1.1 milestone Feb 20, 2017
nlohmann added a commit that referenced this issue Feb 20, 2017
nlohmann added a commit that referenced this issue Feb 21, 2017
@nlohmann
Copy link
Owner Author

OSS-Fuzz marked the issue as fixed. I shall close this ticket.

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

No branches or pull requests

2 participants