bpo-44698: Fix undefined behaviour in complex exponentiation. #27278 8000
Conversation
The undefined behaviour is already triggered by an existing check in test_complex (test_truediv), which was commented out until Python 3.8. Clang's UBSan, specifically -fsanitize=float-cast-overflow, caught the error.
Objects/complexobject.c
Outdated
| @@ -526,7 +526,10 @@ complex_pow(PyObject *v, PyObject *w, PyObject *z) | |||
| } | |||
| errno = 0; | |||
| exponent = b; | |||
| int_exponent = (long)exponent.real; | |||
| if ((double)LONG_MIN <= exponent.real && exponent.real <= (double)LONG_MAX) | |||
There was a problem hiding this comment.
if the sanitizer is happy with this, i'm happy with this. It's basically asking the compiler to do the cast from high precision 64-bit int to less precision 53-bit *2^11 int in the right direction.
There was a problem hiding this comment.
Yes, UBSan is happy with it; tested internally with all the sanitizers, and with Debian's clang 7.0 with CFLAGS=-fsanitize=float-cast-overflow LDFLAGS=-fsanitize=float-cast-overflow.
There was a problem hiding this comment.
Does this condition still allow the case of exponent.real equal to 2**63 (because (double)LONG_MAX will be rounded up to 2**63, assuming 64-bit longs)?
There was a problem hiding this comment.
If you look at the next bit of code, it's checking that int_exponent == exponent.real before using int_exponent. It's checking for a fast path for complex ** int (or float with an integer value) after creating a complex out of int. I believe the new check has exactly the same effect while avoiding undefined behaviour, but the failure mode here is taking the slightly slower path for integer values that don't round-trip through the complex's 'real' double.
There was a problem hiding this comment.
Right, but that check occurs after the undefined behaviour has already taken place. If exponent.real is 0x1.0p+63, the guard in the if condition will be true, and then we invoke undefined behaviour when we do (long)exponent.real.
There was a problem hiding this comment.
Whoops, sorry; wrong section reference. That should be C11 §6.3.1.4p2, not C11 §6.3.1.5 (but the same quoted text).
There was a problem hiding this comment.
I think changing the second inequality to exponent.real < -(double)LONG_MIN would fix this corner case. Unlike LONG_MAX, the conversion of LONG_MIN to a double should be exact (assuming two's complement, no trap representation, etc., etc., but those are assumptions that we've been happy to make in many other places in the code).
There was a problem hiding this comment.
Yep, you're right. I didn't realise a double that's equal to LONG_MAX could end up being an invalid cast to long. I've added a test that triggered the behaviour, and was correctly caught by UBSan without the fix:
test_pow (main.ComplexTest) ... ../../cpython/Objects/complexobject.c:530:24: runtime error: 9.22337e+18 is outside the range of representable values of type 'long'
There was a problem hiding this comment.
Well, technically the double 0x1.0p+63 isn't equal to LONG_MAX; it's equal to the result of converting LONG_MAX to a double - the error introduced in that conversion is exactly the cause of difficulties here.
Thanks for the fix! (BTW, I'm not Mark Shannon. :-))
There was a problem hiding this comment.
D'oh, I'm sorry, Mark! I know you're not Mark Shannon, I have no idea what I was thinking while I wrote that commit message.
|
FWIW, the only use of int_exponent is guarded by the test on the subsequent line: Presumably, that has prevented any real word errors by checking after the conversion rather than beforehand. |
Objects/complexobject.c
Outdated
| @@ -526,7 +526,10 @@ complex_pow(PyObject *v, PyObject *w, PyObject *z) | |||
| } | |||
| errno = 0; | |||
| exponent = b; | |||
| int_exponent = (long)exponent.real; | |||
| if ((double)LONG_MIN <= exponent.real && exponent.real < (double)LONG_MAX) | |||
There was a problem hiding this comment.
Just as a comment: the reason I suggested -(double)LONG_MIN instead of (double)LONG_MAX is that (a) it's easier to reason about, since LONG_MIN is (almost certainly) a power of 2 and so there's no error introduced in the long-to-double conversion, and (b) it makes a difference in the case of 32-bit longs: if exponent.real has value exactly equal to 2**31 - 1 then it can safely be converted to a long, but the if condition excludes it. So we miss one particular integer case here. It doesn't matter much - there's no undefined behaviour either way, but I do think -(double)LONG_MIN is a bit cleaner here.
There was a problem hiding this comment.
FWIW I find -(double)LONG_MIN looking odd but the explanation why is very clear. Since it's strictly more correct (even if the difference is academic), I'd suggest adopting it with a comment essentially copying what Mark wrote above. It'll be a gem to find for future code readers. Think of the children!
There was a problem hiding this comment.
Actually, looking at the code a bit more, I think we should stop doing this altogether. The code is taking 'w' and converting it to a complex, and then unpacking the complex to check if w is an integer value that fits in a C long. We have a much easier way of checking for that...
… both undefined behaviour and unnecessary conversion back and forth.
|
GH-27366 is a backport of this pull request to the 3.10 branch. |
|
GH-27367 is a backport of this pull request to the 3.9 branch. |
The undefined behaviour is already triggered by an existing check in test_complex (test_truediv, later moved to test_pow), which was commented out until Python 3.8. Clang's UBSan, specifically -fsanitize=float-cast-overflow, caught the error. It's present in all Python versions that I have access to, but it's probably not worth fixing in 3.8 and earlier.
https://bugs.python.org/issue44698