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

floating point cancellation in duration leading to assert #1142

Closed
pauldreik opened this issue May 5, 2019 · 2 comments
Closed

floating point cancellation in duration leading to assert #1142

pauldreik opened this issue May 5, 2019 · 2 comments

Comments

@pauldreik
Copy link
Contributor

Hi,
I pulled the fix in #1141 (comment) and continued fuzzing. This time it survived for almost a minute!
The title is the theory of what I think is the problem.
For the floating point case, I added internal checks for incoming NaN and/or Inf and throw an exception if one is found. It also checks if any of the seconds or milliseconds are NaN and/or Inf. Looking at https://en.cppreference.com/w/cpp/chrono/duration/duration_cast it seems like it's good to avoid NaN altogether (undefined behaviour).

This was however not sufficient. As revealed by the following:

TEST(ChronoTest, OverflowingFloat2) {
  const std::chrono::duration<float, std::atto> d{1.79400457e+31f};
  fmt::format("{:%S}", d);
}

There is a problem with the calculation of seconds and milliseconds. I think the error is that such a large float value has a granularity larger than a second. The (d-s) expression has cancellation like problems. The new constructor looks like this (the real version is here):

 explicit chrono_formatter(FormatContext& ctx, OutputIt o,
                            std::chrono::duration<Rep, Period> d)
      : context(ctx), out(o), val(d.count()) {
    constexpr bool is_floating_point = std::is_floating_point<Rep>::value;
    if (is_floating_point && !std::isfinite(d.count())) {
      FMT_THROW(format_error("floating point duration is NaN or Inf"));
    }
    if (d.count() < 0) {
      d = -d;
      *out++ = '-';
    }

    if constexpr (is_floating_point) {
      auto tmpseconds = std::chrono::duration_cast<seconds>(d);
      s = seconds{std::floor(tmpseconds.count())};
      ms = std::chrono::duration_cast<milliseconds>(tmpseconds - s);
      if (!std::isfinite(s.count()) || !std::isfinite(ms.count())) {
        FMT_THROW(format_error("internal overflow of floating point duration"));
      }
    } else {
      s = std::chrono::duration_cast<seconds>(d);
      ms = std::chrono::duration_cast<milliseconds>(d - s);
    }
  }

With these changes, my floating point fuzzers have survived for a cpu hour. They also survive the crashing input I saved the last days when working with this. I will keep it running to see if it finds anything. I know I use C++17 but I couldn't resist using the wonderful constexpr if :-)

@vitaut
Copy link
Contributor

vitaut commented May 8, 2019

You are right, there is a problem with d - s which is due to limited FP precision. I tried to address it in e9bab6d. The actual output is kinda meaningless for very large numbers (garbage in -> garbage out =)) but at least it shouldn't trigger the assertion.

@vitaut
Copy link
Contributor

vitaut commented May 8, 2019

BTW NaNs should be already handled by ca978b3. I followed https://github.com/HowardHinnant/date's approach of formatting them as "nan" instead of giving an error.

@vitaut vitaut closed this as completed May 8, 2019
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

No branches or pull requests

2 participants