classification
Title: Crash in Decimal.from_float
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: skrah Nosy List: facundobatista, mark.dickinson, python-dev, rhettinger, serhiy.storchaka, skrah
Priority: normal Keywords: patch

Created on 2016-05-08 05:13 by serhiy.storchaka, last changed 2016-07-23 15:55 by skrah. This issue is now closed.

Files
File name Uploaded Description Edit
decimal_from_float_check_result.patch serhiy.storchaka, 2016-05-08 15:46 review
decimal_from_float_exact_float.patch serhiy.storchaka, 2016-05-08 15:52 review
issue26974.diff skrah, 2016-07-16 22:09 review
Messages (15)
msg265113 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-08 05:13
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))
msg265148 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-08 15:46
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.
msg265149 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-08 15:52
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 issue26975 and slightly speeds up C implementation.
msg265186 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2016-05-09 09:36
[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 issue26975 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.
msg265193 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-09 13:49
Actually the part of the second patch for Python implementation not always works correctly due to a bug (issue26983). We should either first fix issue26983 or use more complicated code.
msg269025 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-21 21:06
Stefan?
msg269031 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-06-21 22:05
I'll look at it soon -- I just don't have much time right now.
msg269793 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-04 16:35
Ping.
msg269892 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-07-06 17:39
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.
msg270586 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-07-16 22:09
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" #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.
msg270634 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-07-17 12:14
New changeset f8cb955efd6a by Stefan Krah in branch '3.5':
Issue #26974: Fix segfault in the presence of absurd subclassing. Proactively
https://hg.python.org/cpython/rev/f8cb955efd6a
msg270635 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-07-17 12:20
Couldn't keeping references in static variables cause problems in subinterpreters?
msg270638 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-07-17 12:41
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.
msg270639 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-07-17 12:50
Also, IMO the whole capsule mechanism would be broken if function pointers in dynamic libs could just be invalidated due to reloading.
msg270641 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-07-17 13:03
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))
History
Date User Action Args
2016-07-23 15:55:18skrahsetstatus: open -> closed
stage: commit review -> resolved
2016-07-17 13:03:36skrahsetresolution: fixed
messages: + msg270641
stage: patch review -> commit review
2016-07-17 12:50:12skrahsetmessages: + msg270639
2016-07-17 12:41:53skrahsetmessages: + msg270638
2016-07-17 12:20:16serhiy.storchakasetmessages: + msg270635
2016-07-17 12:14:09python-devsetnosy: + python-dev
messages: + msg270634
2016-07-16 22:09:28skrahsetfiles: + issue26974.diff

messages: + msg270586
2016-07-06 17:39:07rhettingersetmessages: + msg269892
2016-07-04 16:35:10serhiy.storchakasetmessages: + msg269793
2016-06-21 22:05:15skrahsetassignee: skrah
messages: + msg269031
2016-06-21 21:06:31serhiy.storchakasetmessages: + msg269025
2016-05-09 13:49:40serhiy.storchakasetmessages: + msg265193
2016-05-09 09:36:34mark.dickinsonsetmessages: + msg265186
2016-05-08 15:52:14serhiy.storchakasetfiles: + decimal_from_float_exact_float.patch

messages: + msg265149
stage: patch review
2016-05-08 15:46:45serhiy.storchakasetfiles: + decimal_from_float_check_result.patch
keywords: + patch
messages: + msg265148
2016-05-08 05:13:47serhiy.storchakacreate