Skip to content

Commit

Permalink
gh-34997: Fix bug due to UB in conversion from python int to ZZ (pyth…
Browse files Browse the repository at this point in the history
…on 3.11, 32 bit, gcc12)

    
This affects 32 bit architectures, where the representation of python
integers changed in cpython 3.11, when compiled with gcc12.

As part of #33842, the function
`sage.arith.long.integer_check_long_py()` was rewritten to support the
new representation.  Unfortunately a bug remained that triggers UB for
the conversion of integers between 2^60 and 2^63-1. Alas, the undesired
behaviour does not happen with gcc10; it only started when I switched to
gcc12.

The bug manifests in lots of doctests failing, but a quick way to
demonstrate the issue is

    sage: ZZ ( int(1152921504606847018) )  # 2^60 + 42
    42

The function `integer_check_long_py()` has good unit testing, checking
values around the word size, but this range was missing.

This commit adds a simple fix and new test cases for a few integers in
this range.

Technical explanation:

The UB is in the line

    cdef long lead_3_overflow = (<long>1) << (BITS_IN_LONG - 2 *
PyLong_SHIFT)

In our case we have `BITS_IN_LONG == 31` and `PyLong_SHIFT == 30` so the
computed value is `<long>1 << -29` which is UB and it happens to
evaluate to 0 with gcc10 but 8 with gcc12.

The solution is to set the value to 0 when `BITS_IN_LONG < 2 *
PyLong_SHIFT` (which only happens for 32 bit python 3.11)

---

TESTING:
 - With gcc10 the fix in #33842 was extensively tested (e.g. in
void-linux/void-packages#41085) and everything
seemed ok, that's why we missed this bug.
 - When using gcc 12, without this PR around 200 tests fail on 32 bit.
 - With this PR all tests pass as shown in https://github.com/void-
linux/void-packages/pull/42048 (I'm actually testing sagemath-9.7 with
python 3.11 support backported since that's easier for me, but it
shouldn't make a difference).
    
URL: #34997
Reported by: Gonzalo Tornaría
Reviewer(s): Gonzalo Tornaría, Matthias Köppe
  • Loading branch information
Release Manager committed Feb 21, 2023
2 parents 27d575c + 15f37f7 commit 3380d29
Showing 1 changed file with 17 additions and 1 deletion.
18 changes: 17 additions & 1 deletion src/sage/arith/long.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,17 @@ cdef inline bint integer_check_long_py(x, long* value, int* err):
sage: L += [-x for x in L] + [0, long_min()]
sage: for v in L:
....: assert check_long_py(int(v)) == v
sage: check_long_py(int(2^60))
1152921504606846976 # 64-bit
'Overflow (...)' # 32-bit
sage: check_long_py(int(2^61))
2305843009213693952 # 64-bit
'Overflow (...)' # 32-bit
sage: check_long_py(int(2^62))
4611686018427387904 # 64-bit
'Overflow (...)' # 32-bit
sage: check_long_py(int(2^63))
'Overflow (...)'
sage: check_long_py(int(2^100))
'Overflow (...)'
sage: check_long_py(int(long_max() + 1))
Expand Down Expand Up @@ -309,7 +320,12 @@ cdef inline bint integer_check_long_py(x, long* value, int* err):

cdef long lead
cdef long lead_2_overflow = (<long>1) << (BITS_IN_LONG - PyLong_SHIFT)
cdef long lead_3_overflow = (<long>1) << (BITS_IN_LONG - 2 * PyLong_SHIFT)
cdef long lead_3_overflow
if BITS_IN_LONG < 2 * PyLong_SHIFT:
# in this case 3 digit is always overflow
lead_3_overflow = 0
else:
lead_3_overflow = (<long>1) << (BITS_IN_LONG - 2 * PyLong_SHIFT)
if size == 0:
value[0] = 0
err[0] = 0
Expand Down

0 comments on commit 3380d29

Please sign in to comment.