Skip to content
New issue

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

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

Already on GitHub? # to your account

Avoid fairly easy to trigger overflow in Windows Time.nanos code #4227

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

SeanTAllen
Copy link
Member

For millis, micros, and nanos support on Windows, we use the QueryPerformanceCounter as a monotonically increasing "source of time". Given the counter is 64-bits, for an imaginary 2ghz processor that probably gives us about 290ish years of ticks before we would wrap around and our monotonic nature would be broken. All this means, its an excellent "source of time".

To turn the result from the QueryPerformanceCounter into seconds, you divide its value by the QueryPerformanceFrequency. One my computer, the qpf value is 10_000_000.

The previous code for determining nanos on Windows was unfortunately, far more prone to wrapping around than the QueryPerformanceCounter. Previously, we did the following for nanos:

(qpc * 1_000_000_000) / qpf

Because we were multiplying the qpc (a 64 bit integer) by a large integer (1_000_000_000), we were greatly increasing the chance of wraparound.

I discovered this was happening in the wild because we had timers that would "hang" on Windows. It turns out they weren't hanging. What would happen is that eventually, the value from nanos would wrap around and based on the logic in Timers, this would take a 30 second timer and turn it into a 30 minute timer. (Numbers not exact, given more for a feel of the change). This looked like a runtime hang or otherwise a runtime bug, when in actuality, the timer was "doing the correct" thing based on the values it was given.

The change made to millis, micros, and nanos in this commit will greatly decrease our overflow chance with nanos as we are no longer doing the "first multiple by really big number".

Note that we change our algo to use based on whether the qpf value is greater than then number of subseconds to calculate. It is important that we do this so that we don't start returning incorrect values. If we
didn't do that we could end up with a situation like this for millis:

QPC * (1_000 / 10_000_000) which for integer math is

QPC * 0

aka "not good"

For millis, micros, and nanos support on Windows, we use the
QueryPerformanceCounter as a monotonically increasing "source of time".
Given the counter is 64-bits, for an imaginary 2ghz processor that
probably gives us about 290ish years of ticks before we would wrap around
and our monotonic nature would be broken. All this means, its an
excellent "source of time".

To turn the result from the QueryPerformanceCounter into seconds, you
divide its value by the QueryPerformanceFrequency. One my computer, the
qpf value is 10_000_000.

The previous code for determining nanos on Windows was unfortunately, far
more prone to wrapping around than the QueryPerformanceCounter. Previously,
we did the following for nanos:

(qpc * 1_000_000_000) / qpf

Because we were multiplying the qpc (a 64 bit integer) by a large integer
(1_000_000_000), we were greatly increasing the chance of wraparound.

I discovered this was happening in the wild because we had timers that would
"hang" on Windows. It turns out they weren't hanging. What would happen is that
eventually, the value from nanos would wrap around and based on the logic in
Timers, this would take a 30 second timer and turn it into a 30 minute timer.
(Numbers not exact, given more for a feel of the change). This looked like
a runtime hang or otherwise a runtime bug, when in actuality, the timer was
"doing the correct" thing based on the values it was given.

The change made to millis, micros, and nanos in this commit will greatly decrease
our overflow chance with nanos as we are no longer doing the "first multiple by
really big number".

Note that we change our algo to use based on whether the qpf value is greater than
then number of subseconds to calculate. It is important that we do this so that
we don't start returning incorrect values. If we
didn't do that we could end up with a situation like this for millis:

QPC * (1_000 / 10_000_000) which for integer math is

QPC * 0

aka "not good"
@SeanTAllen SeanTAllen requested a review from a team November 4, 2022 17:42
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Nov 4, 2022
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Nov 4, 2022
@SeanTAllen SeanTAllen changed the title Avoid fairly easy to trigger overflow in Windows time code Avoid fairly easy to trigger overflow in Windows Time.nanos code Nov 4, 2022
@SeanTAllen SeanTAllen merged commit 2b40cc4 into main Nov 4, 2022
@SeanTAllen SeanTAllen deleted the windows-nanos branch November 4, 2022 19:10
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Nov 4, 2022
github-actions bot pushed a commit that referenced this pull request Nov 4, 2022
github-actions bot pushed a commit that referenced this pull request Nov 4, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants