Home Original page

bpo-34423: Fix check for overflow when casting from a double to integral types. by enedil · Pull Request #8802 · python/cpython

@enedil

@enedil enedil commented

Aug 17, 2018

•

edited by serhiy-storchaka

Loading

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.

https://bugs.python.org/issue34423

@enedil 

…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.

@enedil

@asottile

@enedil

@asottile Yes, you're right. My branch is wrongly named too. Should I create new pull request?

@asottile

It might be enough to edit the commit message and title? I don't actually know

@enedil enedil changed the title bpo-32367: Fix check for overflow when casting from a double to integ… bpo-34423: Fix check for overflow when casting from a double to integ…

Aug 19, 2018

@serhiy-storchaka serhiy-storchaka changed the title bpo-34423: Fix check for overflow when casting from a double to integ… bpo-34423: Fix check for overflow when casting from a double to integral types.

Aug 20, 2018

vstinner

Choose a reason for hiding this comment

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

A test should be added to test_time.

@enedil

@enedil

asottile

# _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)

Choose a reason for hiding this comment

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

unintentional change?

@enedil

jdemeyer

Choose a reason for hiding this comment

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

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))

@vstinner

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.

@jdemeyer

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?

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 double (53 assuming IEEE-754) is less than the number of bits in a int64_t (63).

iritkatriel

Choose a reason for hiding this comment

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

This has merge conflicts, as well as several unresolved issues raised by reviewers.

@bedevere-bot

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

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

@enedil

I'm fine with leaving this as unresolved.

@iritkatriel

I'm fine with leaving this as unresolved.

Sure, thanks.

@enedil enedil mannequin mentioned this pull request

Dec 8, 2022