bpo-34423: Fix check for overflow when casting from a double to integral types. #8802
Conversation
…ral types The added comment explains the issue. This fix disallows overflow: if a double `d` passes the test, it can mean that `d == 2**63`. When casted to `int64_t`, the variable overflows to -2**63.
|
this appears linked to the wrong issue -- is this the right one? https://bugs.python.org/issue34423 |
|
@asottile Yes, you're right. My branch is wrongly named too. Should I create new pull request? |
|
It might be enough to edit the commit message and title? I don't actually know |
enedil
changed the title
serhiy-storchaka
changed the title
Lib/test/test_time.py
Outdated
| @@ -917,7 +918,8 @@ class TestCPyTime(CPyTimeTestCase, unittest.TestCase): | |||
| Test the C _PyTime_t API. | |||
| """ | |||
| # _PyTime_t is a 64-bit signed integer | |||
| OVERFLOW_SECONDS = math.ceil((2**63 + 1) / SEC_TO_NS) | |||
| OVERFLOW_SECONDS = math.ceil(( | |||
| + 1) / SEC_TO_NS) | |||
There was a problem hiding this comment.
I don't think that the condition sizeof(*v*) == sizeof(*type*) is really the right condition. Besides, you are applying the fix (changing <= to <) unconditionally, also when the maximal value can be represented exactly.
The problem is that the maximal value 2^N - 1 is hard to work with. So I suggest to replace
v < _Py_IntegralTypeMax(type)
by something like
v < ldexp(_Py_IntegralTypeSigned(type) ? 0.5 : 1.0, CHAR_BIT * sizeof(type))
|
I'm not sure that we will be able to find a solution using a "simple" macro. Maybe we need new functions which takes a double as input and return an integer as output and report overflow. I'm not sure that it's possible to write a generic function. Right now, _Py_InIntegralTypeRange() is only used to convert a double to _PyTime_t (int64_t) and time_t in pytime.c. Maybe 2 functions would be enough? About generic code, we can maybe use the preprocessor as a template engine to generate these functions. |
It would certainly be easier to support specific types only instead of being generic. We could for example assume that the number of bits in a |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be put in the comfy chair! |
|
I'm fine with leaving this as unresolved. |
Sure, thanks. |
The added comment explains the issue. This fix disallows overflow: if a double
dpasses the test, it can mean thatd == 2**63. When casted toint64_t, the variable overflows to -2**63.https://bugs.python.org/issue34423