Issue 35959: math.prod(range(10)) caues segfault (original) (raw)
Created on 2019-02-10 19:08 by xtreak, last changed 2022-04-11 14:59 by admin. This issue is now closed.
Messages (12)
Author: Karthikeyan Singaravelan (xtreak) * 
Date: 2019-02-10 19:08
math.prod introduced with seems to segfault when zero is present on some cases like start or middle of the iterable. I couldn't find the exact cause of this. This also occurs in optimized builds.
Python information built with ./configure && make
➜ cpython git:(master) ./python.exe Python 3.8.0a1+ (heads/master:8a03ff2ff4, Feb 11 2019, 00:13:49) [Clang 7.0.2 (clang-700.1.81)] on darwin Type "help", "copyright", "credits" or "license" for more information.
Segfaults with range(10), [0, 1, 2, 3] and [1, 0, 2, 3]
➜ cpython git:(master) ./python.exe -X faulthandler -c 'import math; print(math.prod(range(10)))' Fatal Python error: Floating point exception
Current thread 0x00007fff7939f300 (most recent call first): File "", line 1 in [1] 40465 floating point exception ./python.exe -X faulthandler -c 'import math; print(math.prod(range(10)))'
➜ cpython git:(master) ./python.exe -X faulthandler -c 'import math; print(math.prod([0, 1, 2, 3]))' Fatal Python error: Floating point exception
Current thread 0x00007fff7939f300 (most recent call first): File "", line 1 in [1] 40414 floating point exception ./python.exe -X faulthandler -c 'import math; print(math.prod([0, 1, 2, 3]))' ➜ cpython git:(master) ./python.exe -X faulthandler -c 'import math; print(math.prod([1, 0, 2, 3]))' Fatal Python error: Floating point exception
Current thread 0x00007fff7939f300 (most recent call first): File "", line 1 in [1] 40425 floating point exception ./python.exe -X faulthandler -c 'import math; print(math.prod([1, 0, 2, 3]))'
No segfault when zero is at the end and floats seem to work fine.
➜ cpython git:(master) ./python.exe -X faulthandler -c 'import math; print(math.prod([1, 2, 3, 0]))' 0 ➜ cpython git:(master) ./python.exe -c 'import math; print(math.prod(map(float, range(10))))' 0.0
Author: Pablo Galindo Salgado (pablogsal) *
Date: 2019-02-10 19:18
The problem is that the intermediate result (i_result) can be 0 when doing the overflow check:
x / i_result != b
i am working on a fix.
Author: Rémi Lapeyre (remi.lapeyre) *
Date: 2019-02-10 19:20
Could it be https://github.com/python/cpython/blob/master/Modules/mathmodule.c#L2565
When 0 is in the iterator, i_result get sets to 0 and then on the next iteration x/i_result is 0/0 which is undefined behavior?
C99 6.5.5p5 - The result of the / operator is the quotient from the division of the first operand by the second; the result of the % operator is the remainder. In both operations, if the value of the second operand is zero, the behavior is undefined.
I will do some tests, if it's that I will post a patch.
Author: Pablo Galindo Salgado (pablogsal) *
Date: 2019-02-10 19:24
PR11808 fixes the error and add some basic test. Please review the PR instead :)
Author: Mark Dickinson (mark.dickinson) *
Date: 2019-02-11 14:36
This approach to overflow checking needs reworking; the current code, in lines like:
long x = i_result * b;induces undefined behaviour on overflow. That's something we need to avoid.
Author: Mark Dickinson (mark.dickinson) *
Date: 2019-02-11 14:45
See below for a link to the Python 2.7 code for int * int multiplication (which needs to detect overflow so that it knows when to promote to long):
Author: Karthikeyan Singaravelan (xtreak) *
Date: 2019-02-11 14:48
@pablogsal I am reopening this since this has an open PR though original issue was fixed. Feel free to close this if this can be discussed in a separate issue.
Author: Pablo Galindo Salgado (pablogsal) *
Date: 2019-02-11 14:53
@Mark Dickinson Thanks for the links. I had opened yesterday PR11809 addressing the UB and adding more tests. Could you review the PR?
Author: Mark Dickinson (mark.dickinson) *
Date: 2019-02-11 14:57
@Pablo: Thanks; I saw the PR.
It looks as though the troublesome line
long x = i_result * b;
is still there in that PR, though. Or am I misreading? The difficulty here is that the check for overflow has to happen as a means of preventing invoking undefined behaviour; it doesn't really work to invoke the undefined behaviour first and then check to see what went wrong.
Author: Pablo Galindo Salgado (pablogsal) *
Date: 2019-02-11 15:09
@Mark
long x = i_result * b;
is still on the PR but the check for overflow does not use x. I will update the PR to move that line inside the overflow-safe block to prevent UB from happening and affecting the check itself (or the rest of the code).
Thanks for pointing that out!
Author: Cheryl Sabella (cheryl.sabella) *
Date: 2019-04-17 22:50
Hi Pablo,
Was this something you still wanted to clean up or can this be closed? Thanks!
Author: Mark Dickinson (mark.dickinson) *
Date: 2019-04-19 17:26
I think this can be closed; I did look at the PR post-merge (sorry that I didn't get to it before it was merged), and I agree that it should fix the segfault.
There's scope for refactoring / improving the implementation, but that would belong in a different issue.
I'll close, but @pabolgsal: please feel free to re-open if I've misunderstood.
History
Date
User
Action
Args
2022-04-11 14:59:11
admin
set
github: 80140
2019-04-19 17:26:15
mark.dickinson
set
status: open -> closed
messages: +
keywords:patch, patch, patch
resolution: fixed
stage: patch review -> resolved
2019-04-17 22:50:01
cheryl.sabella
set
keywords:patch, patch, patch
nosy: + cheryl.sabella
messages: +
2019-02-11 15:09:35
pablogsal
set
messages: +
2019-02-11 14:57:33
mark.dickinson
set
messages: +
2019-02-11 14:53:25
pablogsal
set
messages: +
2019-02-11 14:48:55
xtreak
set
status: closed -> open
messages: +
keywords:patch, patch, patch
resolution: fixed -> (no value)
stage: resolved -> patch review
2019-02-11 14:45:41
mark.dickinson
set
messages: +
2019-02-11 14:36:05
mark.dickinson
set
keywords:patch, patch, patch
nosy: + mark.dickinson
messages: +
2019-02-10 20:32:19
pablogsal
set
pull_requests: + <pull%5Frequest11827>
2019-02-10 20:32:11
pablogsal
set
pull_requests: + <pull%5Frequest11826>
2019-02-10 20:32:02
pablogsal
set
pull_requests: + <pull%5Frequest11825>
2019-02-10 19:57:08
pablogsal
set
keywords:patch, patch, patch
status: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-02-10 19:24:24
pablogsal
set
messages: +
2019-02-10 19:23:19
pablogsal
set
keywords: + patch
stage: patch review
pull_requests: + <pull%5Frequest11824>
2019-02-10 19:23:13
pablogsal
set
keywords: + patch
stage: (no value)
pull_requests: + <pull%5Frequest11823>
2019-02-10 19:23:07
pablogsal
set
keywords: + patch
stage: (no value)
pull_requests: + <pull%5Frequest11822>
2019-02-10 19:20:17
remi.lapeyre
set
nosy: + remi.lapeyre
messages: +
2019-02-10 19🔞40
pablogsal
set
messages: +
2019-02-10 19:08:53
xtreak
create