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

bpo-39277, pytime: Fix overflow check on double to int cast #17933

Closed
wants to merge 1 commit into from
Closed

bpo-39277, pytime: Fix overflow check on double to int cast #17933

wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jan 10, 2020

Fix time.sleep() to properly detect integer overflow when converting
a floating-point number of seconds to an integer.

https://bugs.python.org/issue39277

Fix time.sleep() to properly detect integer overflow when converting
a floating-point number of seconds to an integer.
@vstinner
Copy link
Member Author

cc @mdickinson

@csabella csabella requested a review from mdickinson May 28, 2020 23:04
/* Check if the floating-point number v (double) would overflow when casted to
* the integral type 'type'.
*
* Test (double)type_min(type) <= v <= (double)type_max(type) where v is a
Copy link
Member

Choose a reason for hiding this comment

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

The two conditions "v would overflow when cast to 'type'" and "type_MIN <= v <= type_MAX" are not equivalent. Which one do you want to test here?

For example, assuming a 32-bit int type: type_MAX would be 2147483647. If v = 2147483647.5 (which is exactly representable in IEEE 754 binary64 floating-point), then it satisfies the first condition - it doesn't overflow when cast to int - but not the second.

*
* In short, nextafter((double)x, 0.0) rounds the integer x towards zero. */
#define _Py_DoubleInIntegralTypeRange(type, v) \
(nextafter((double)_Py_IntegralTypeMin(type), 0.0) <= v \
Copy link
Member

@mdickinson mdickinson May 29, 2020

Choose a reason for hiding this comment

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

Sorry, but this isn't right.

For a 32-bit signed integer type, this condition becomes:

-2147483647.9999998 <= v <= 2147483646.9999998

but what we actually want is:

-2147483649.0 < v < 2147483648.0

or if we want to use <= and express the limits in terms of doubles:

-2147483648.9999995 <= v <= 2147483647.9999998

For a 64-bit signed integer type, this condition becomes:

-9223372036854774784.0 <= v <= 9223372036854774784.0

but the actual limits we need are

-9223372036854775808.0 <= v <= 9223372036854774784.0

Copy link
Member

@mdickinson mdickinson Jul 22, 2021

Choose a reason for hiding this comment

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

Suggested alternative approach: given a double v that we want to safely convert to an int (say), first use modf to extract the fractional part fv and the integer part iv of v (both fv and iv are still doubles at this point). We can just ignore the fractional part fv: it doesn't affect whether we can safely cast to int or not. Now for iv, check whether it satisfies (double)INT_MIN <= iv and iv < -(double)INT_MIN. Note that INT_MIN is (always in practice, even though not guaranteed by the standard) the negation of a power of two, so the conversion to double doesn't change the value and we're effectively checking whether iv lies in the half-open interval [INT_MIN, -INT_MIN). Since iv is an integer, that's equivalent to checking whether iv lies in the closed interval [INT_MIN, -INT_MIN-1]. But making all our usual assumptions about integer representation (two's complement, no trap representation, no padding bits, etc.), -INT_MIN-1 is the same as INT_MAX, so we're effectively checking that the integer part of v is representable as an int, which is exactly what we want to be checking.

Similarly for long, long long, etc.

@mdickinson
Copy link
Member

mdickinson commented May 29, 2020

A safe way to do what you want is:

(1) Take the integral part of the double v that you want to check, for example using modf, still represented as a double. (Note that the integral part of an IEEE 754 floating-point number is always exactly representable in the same format.)
(2) Check that (double)TYPE_MIN <= int_part_of_v < (double)(TYPE_MAX + 1) (using a mixture of C casts and Python chained comparisons here, but you know what I mean)

Computing TYPE_MAX + 1 while avoiding overflow may take some type-specific effort.

The reason that step (2) is safe is that, making the fairly safe assumption of 2s complement (no trap representation, etc., etc.) for signed integers, TYPE_MIN and TYPE_MAX + 1 will always be exactly representable as a double (well, assuming that we're not dealing with 1024-bit integers, that is). But TYPE_MAX can't be assumed to be exactly representable as a double - whether it is or not will depend on the width of the type being tested.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

This isn't the right way to check for convertibility; see the other comments I made on the PR.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be put in the comfy chair!

@graingert
Copy link
Contributor

@vstinner any update on this?

@vstinner
Copy link
Member Author

I failed finding time to finish the PR. I prefer to abandon it.

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

Successfully merging this pull request may close these issues.

5 participants