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

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.

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.

PR11808 fixes the error and add some basic test. Please review the PR instead :)

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.

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

@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.

@Mark Dickinson Thanks for the links. I had opened yesterday PR11809 addressing the UB and adding more tests. Could you review the PR?

@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.

@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!

Hi Pablo,

Was this something you still wanted to clean up or can this be closed?  Thanks!

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.

keywords: patch, patch, patch
resolution: fixed
stage: patch review -> resolved

messages: + msg335229

messages: + msg335226

messages: + msg335225

keywords: patch, patch, patch
resolution: fixed -> (no value)
stage: resolved -> patch review

messages: + msg335223

messages: + msg335172