classification
Title: math.prod(range(10)) caues segfault
Type: crash Stage: patch review
Components: Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: mark.dickinson, pablogsal, remi.lapeyre, rhettinger, xtreak
Priority: normal Keywords: patch, patch, patch

Created on 2019-02-10 19:08 by xtreak, last changed 2019-02-11 15:09 by pablogsal.

Pull Requests
URL Status Linked Edit
PR 11808 merged pablogsal, 2019-02-10 19:23
PR 11808 merged pablogsal, 2019-02-10 19:23
PR 11808 merged pablogsal, 2019-02-10 19:23
PR 11809 merged pablogsal, 2019-02-10 20:32
PR 11809 merged pablogsal, 2019-02-10 20:32
PR 11809 merged pablogsal, 2019-02-10 20:32
Messages (10)
msg335168 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-02-10 19:08
math.prod introduced with issue35606 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 "<string>", line 1 in <module>
[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 "<string>", line 1 in <module>
[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 "<string>", line 1 in <module>
[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
msg335169 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) 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.
msg335170 - (view) 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.
msg335172 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-02-10 19:24
PR11808 fixes the error and add some basic test. Please review the PR instead :)
msg335221 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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.
msg335223 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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):

https://github.com/python/cpython/blob/2f1a317d5fdc45b9d714b067906f612f636ba08e/Objects/intobject.c#L496-L518
msg335224 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) 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.
msg335225 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) 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?
msg335226 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) 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.
msg335229 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) 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!
History
Date User Action Args
2019-02-11 15:09:35pablogsalsetkeywords: patch, patch, patch

messages: + msg335229
2019-02-11 14:57:33mark.dickinsonsetkeywords: patch, patch, patch

messages: + msg335226
2019-02-11 14:53:25pablogsalsetkeywords: patch, patch, patch

messages: + msg335225
2019-02-11 14:48:55xtreaksetstatus: closed -> open
messages: + msg335224

keywords: patch, patch, patch
resolution: fixed ->
stage: resolved -> patch review
2019-02-11 14:45:41mark.dickinsonsetkeywords: patch, patch, patch

messages: + msg335223
2019-02-11 14:36:05mark.dickinsonsetkeywords: patch, patch, patch
nosy: + mark.dickinson
messages: + msg335221

2019-02-10 20:32:19pablogsalsetpull_requests: + pull_request11827
2019-02-10 20:32:11pablogsalsetpull_requests: + pull_request11826
2019-02-10 20:32:02pablogsalsetpull_requests: + pull_request11825
2019-02-10 19:57:08pablogsalsetkeywords: patch, patch, patch
status: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-02-10 19:24:24pablogsalsetkeywords: patch, patch, patch

messages: + msg335172
2019-02-10 19:23:19pablogsalsetkeywords: + patch
stage: patch review
pull_requests: + pull_request11824
2019-02-10 19:23:13pablogsalsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11823
2019-02-10 19:23:07pablogsalsetkeywords: + patch
stage: (no value)
pull_requests: + pull_request11822
2019-02-10 19:20:17remi.lapeyresetnosy: + remi.lapeyre
messages: + msg335170
2019-02-10 19:18:40pablogsalsetmessages: + msg335169
2019-02-10 19:08:53xtreakcreate