Crash in Decimal.from_float

[serhiy-storchaka]

BPO	26974
Nosy	@rhettinger, @facundobatista, @mdickinson, @skrah, @serhiy-storchaka
Files	decimal_from_float_check_result.patch decimal_from_float_exact_float.patch issue26974.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/skrah'
closed_at = <Date 2016-07-23.15:55:18.679>
created_at = <Date 2016-05-08.05:13:47.704>
labels = ['extension-modules', 'type-crash']
title = 'Crash in Decimal.from_float'
updated_at = <Date 2016-07-23.15:55:18.678>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2016-07-23.15:55:18.678>
actor = 'skrah'
assignee = 'skrah'
closed = True
closed_date = <Date 2016-07-23.15:55:18.679>
closer = 'skrah'
components = ['Extension Modules']
creation = <Date 2016-05-08.05:13:47.704>
creator = 'serhiy.storchaka'
dependencies = []
files = ['42781', '42782', '43756']
hgrepos = []
issue_num = 26974
keywords = ['patch']
message_count = 15.0
messages = ['265113', '265148', '265149', '265186', '265193', '269025', '269031', '269793', '269892', '270586', '270634', '270635', '270638', '270639', '270641']
nosy_count = 6.0
nosy_names = ['rhettinger', 'facundobatista', 'mark.dickinson', 'skrah', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue26974'
versions = ['Python 3.5', 'Python 3.6']

[serhiy-storchaka]

Following example causes a segmentation fault.

from decimal import Decimal
class BadFloat(float):
    def as_integer_ratio(self):
        return 1
    def __abs__(self):
        return self

Decimal.from_float(BadFloat(1.2))

[serhiy-storchaka]

Here are two alternative patches that fix the crash.

The first patch adds checks for as_integer_ratio() result. The only disadvantage is that error messages are quite arbitrary and differ from error messages in Python version.

[serhiy-storchaka]

The second patch converts an instance of float subclass to exact float. Disadvantages are that this changes behavior ignoring overriding as_integer_ratio() and __abs__() and slightly slows down Python implementation. Advantages are that this fixes also bpo-26975 and slightly speeds up C implementation.

[mdickinson]

[Serhiy, on the second patch]

Disadvantages are that this changes behavior ignoring overriding as_integer_ratio() and __abs__() and slightly slows down Python implementation.

None of those seem like big issues to me. Certainly we shouldn't care too much about slowing down the Python implementation a bit more, and I think it's reasonable to say that code that's overriding as_integer_ratio (or __abs__) and then expecting that override to be picked up and used in Decimal.from_float is on its own.

Advantages are that this fixes also bpo-26975 and slightly speeds up C implementation.

Sounds pretty good to me!

I'll let Stefan review the patch properly, but this seems like a good solution to me.

[serhiy-storchaka]

Actually the part of the second patch for Python implementation not always works correctly due to a bug (bpo-26983). We should either first fix bpo-26983 or use more complicated code.

[serhiy-storchaka]

Stefan?

[skrah]

I'll look at it soon -- I just don't have much time right now.

[serhiy-storchaka]

Ping.

[rhettinger]

My preference is to leave the Python implementation of from_float() as-is. Pure Python code is not obligated to defend itself against bizarre code. The C code however is obliged to not segfault.

[skrah]

The approaches look good, but for clarity I want to replace all method calls that should never be overridden by the plain C functions of their corresponding static types.

I have no opinion about the Python version. The diff also "fixes" bpo-26975 for the C version, so perhaps the Python version should be in sync. It is an academic question, since no one will write a class that triggers it.

Preemptively, I'm aware that long_bit_length() is defined with a single argument, then cast to a CFunction, then called with two arguments.

ceval.c does the same. -- We had a discussion about that with MvL a while ago, he preferred to define all CFunctions with two arguments. I'd also prefer that, but that is another issue.

[python-dev]

New changeset f8cb955efd6a by Stefan Krah in branch '3.5':
Issue bpo-26974: Fix segfault in the presence of absurd subclassing. Proactively
https://hg.python.org/cpython/rev/f8cb955efd6a

[serhiy-storchaka]

Couldn't keeping references in static variables cause problems in subinterpreters?

[skrah]

These are builtin static types. Even with non-builtin static types, the address of the type should always be the same. C-extensions aren't reloaded.

[skrah]

Also, IMO the whole capsule mechanism would be broken if function pointers in dynamic libs could just be invalidated due to reloading.

[skrah]

I'm leaving this open in case anyone wants to do something about the Python version. I tend to agree with Raymond:

It is impractical to "fix" all such constructs in the Python version, unless one consistently uses a style like:

   float.as_integer_ratio(float.__abs__(-1.5))