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
Crash in Decimal.from_float #71161
Comments
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)) |
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. |
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. |
[Serhiy, on the second patch]
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
Sounds pretty good to me! I'll let Stefan review the patch properly, but this seems like a good solution to me. |
Stefan? |
I'll look at it soon -- I just don't have much time right now. |
Ping. |
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. |
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. |
New changeset f8cb955efd6a by Stefan Krah in branch '3.5': |
Couldn't keeping references in static variables cause problems in subinterpreters? |
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. |
Also, IMO the whole capsule mechanism would be broken if function pointers in dynamic libs could just be invalidated due to reloading. |
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)) |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: