Skip to content

Commit 0bb40ed

Browse files
author
Release Manager
committed
gh-40556: Avoid mutating value field of constructed Integer ## Problem Run the following code ``` a = [Integer(i) for i in range(100)] try: b = pow(2^100000, 3^100000, 5^100000) finally: del a ``` it will take a while. Press Ctrl-C before it finishes. Then **it will hang** for a while before printing `KeyboardInterrupt`. Meanwhile, the similar-looking code ``` a = [int(i) for i in range(100)] try: b = pow(2^100000, 3^100000, 5^100000) finally: del a ``` does not suffer from the problem. ## Reason There are several places in SageMath code that does something like the following ``` cdef Integer x = <Integer>PY_NEW(Integer) sig_on() mpz_powm(x.value, ...) # mutate x.value, slow sig_off() return x ``` If the user press ctrl-C, the destructor for `x` will be called, but because `mpz_powm` is interrupted, `mpz_powm` may be in an **inconsistent state**, so the destructor might do the wrong thing. ## The band-aid fix The band-aid is described in #24986 : a function `sig_occurred()` is implemented in cysignals that checks whether a signal thrown by `sig_on()` is currently being processed, and the destructor of `Integer` checks: ``` if sig_occurred(): leak this Integer object just in case else: put this Integer object into a common object pool ``` ## The issue There are two problems with this. * This leaks integers **indiscriminately**. In particular, in the example above, **all** the 100 `Integer` objects in `a` are leaked, even though only one is being mutated, being a local variable in the powering method. At any point in the code, we know exactly which object is currently being mutated, as such this indiscriminate leak is unnecessary. * `sig_occurred()` is **slow**: in the example above, it runs the **garbage collector** each time it's called. So, the reason for the hang is that after the user presses Ctrl-C, Python needs to run the garbage collector 100 times before reporting `KeyboardInterrupt`. ## The actual fix I look at the example reported in #24986 — `coerce_actions.pyx`. The problematic doctest is ``` E = EllipticCurve(GF(5), [4,0]) P = E([2,1,1]) from sage.doctest.util import ensure_interruptible_after with ensure_interruptible_after(0.001): 2^(10^8) * P ``` by some analysis, I determine that the responsible function is `_pow_long`. As such, this pull request makes sure to not call the destructor when the user interrupts. After this pull request, we can remove the `sig_occurred()` workaround introduced in #24986 . ## Other reported issues * #39224 * sagemath/cysignals#215 (comment) * sagemath/cysignals#227 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - #12345: short description why this is a dependency --> <!-- - #34567: ... --> URL: #40556 Reported by: user202729 Reviewer(s): Michael Orlitzky, Travis Scrimshaw, user202729
2 parents e85de39 + 9a5e056 commit 0bb40ed

File tree

1 file changed

+61
-35
lines changed

1 file changed

+61
-35
lines changed

src/sage/rings/integer.pyx

Lines changed: 61 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,42 @@ cdef class IntegerWrapper(Integer):
401401
Element.__init__(self, parent=parent)
402402
Integer.__init__(self, x, base=base)
403403

404+
405+
cdef inline Integer move_integer_from_mpz(mpz_t x):
406+
"""
407+
Construct an :class:`Integer` by moving it from a ``mpz_t`` object.
408+
409+
This function is intend to be used as follows::
410+
411+
cdef mpz_t x # local variable
412+
mpz_init(x)
413+
sig_on()
414+
mpz_SOMETHING_MUTATE_X(x, ...)
415+
sig_off()
416+
return move_integer_from_mpz(x) # no need mpz_clear(x)
417+
418+
The reason to do so is explained in :issue:`24986`:
419+
if the user interrupts the operation, ``x`` may be in an inconsistent state,
420+
as such we don't want to call ``fast_tp_dealloc`` on it, because
421+
putting a corrupted object in the integer pool may lead to incorrect results
422+
afterwards.
423+
424+
Here we do not call ``mpz_clear(x)`` either, but double ``free()``
425+
is also undesirable. Compared to these issues, memory leak is the least problematic.
426+
427+
In this case:
428+
429+
- if ``sig_on()`` throws, :func:`move_integer_from_mpz` will not be called, as such
430+
``x`` will not be cleared;
431+
432+
- if ``sig_on()`` does not throw, :func:`move_integer_from_mpz` will call ``mpz_clear(x)``.
433+
"""
434+
cdef Integer y = <Integer>PY_NEW(Integer)
435+
mpz_swap(y.value, x)
436+
mpz_clear(x)
437+
return y
438+
439+
404440
cdef class Integer(sage.structure.element.EuclideanDomainElement):
405441
r"""
406442
The :class:`Integer` class represents arbitrary precision
@@ -447,6 +483,8 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
447483
"""
448484

449485
def __cinit__(self):
486+
# this function is only called to create global_dummy_Integer,
487+
# after that it will be replaced by fast_tp_new
450488
global the_integer_ring
451489
mpz_init(self.value)
452490
self._parent = the_integer_ring
@@ -1746,9 +1784,7 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
17461784
cdef Integer temp
17471785

17481786
if mpz_sgn(self.value) == 0:
1749-
temp = PY_NEW(Integer)
1750-
mpz_set_ui(temp.value, 0)
1751-
return temp
1787+
return self
17521788

17531789
if mpz_sgn(self.value) > 0:
17541790
temp = self.exact_log(base)
@@ -1780,7 +1816,7 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
17801816
mpz_add(x.value, (<Integer>left).value, (<Integer>right).value)
17811817
return x
17821818
elif type(right) is Rational:
1783-
y = <Rational> Rational.__new__(Rational)
1819+
y = <Rational>PY_NEW(Rational)
17841820
mpq_add_z(y.value, (<Rational>right).value, (<Integer>left).value)
17851821
return y
17861822

@@ -1863,7 +1899,7 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
18631899
mpz_sub(x.value, (<Integer>left).value, (<Integer>right).value)
18641900
return x
18651901
elif type(right) is Rational:
1866-
y = <Rational> Rational.__new__(Rational)
1902+
y = <Rational>PY_NEW(Rational)
18671903
mpz_mul(mpq_numref(y.value), (<Integer>left).value,
18681904
mpq_denref((<Rational>right).value))
18691905
mpz_sub(mpq_numref(y.value), mpq_numref(y.value),
@@ -1975,7 +2011,7 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
19752011
mpz_mul(x.value, (<Integer>left).value, (<Integer>right).value)
19762012
return x
19772013
elif type(right) is Rational:
1978-
y = <Rational> Rational.__new__(Rational)
2014+
y = <Rational>PY_NEW(Rational)
19792015
mpq_mul_z(y.value, (<Rational>right).value, (<Integer>left).value)
19802016
return y
19812017

@@ -2038,14 +2074,14 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
20382074
if type(left) is type(right):
20392075
if mpz_sgn((<Integer>right).value) == 0:
20402076
raise ZeroDivisionError("rational division by zero")
2041-
x = <Rational> Rational.__new__(Rational)
2077+
x = <Rational>PY_NEW(Rational)
20422078
mpq_div_zz(x.value, (<Integer>left).value, (<Integer>right).value)
20432079
return x
20442080
elif type(right) is Rational:
20452081
if mpq_sgn((<Rational>right).value) == 0:
20462082
raise ZeroDivisionError("rational division by zero")
20472083
# left * den(right) / num(right)
2048-
y = <Rational> Rational.__new__(Rational)
2084+
y = <Rational>PY_NEW(Rational)
20492085
mpq_div_zz(y.value, (<Integer>left).value,
20502086
mpq_numref((<Rational>right).value))
20512087
mpz_mul(mpq_numref(y.value), mpq_numref(y.value),
@@ -2067,7 +2103,7 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
20672103
"""
20682104
if mpz_sgn((<Integer>right).value) == 0:
20692105
raise ZeroDivisionError("rational division by zero")
2070-
x = <Rational> Rational.__new__(Rational)
2106+
x = <Rational>PY_NEW(Rational)
20712107
mpq_div_zz(x.value, self.value, (<Integer>right).value)
20722108
return x
20732109

@@ -2299,18 +2335,18 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
22992335
elif n == 1:
23002336
return self
23012337

2302-
cdef Integer x
23032338
cdef Rational q
2339+
cdef mpz_t x
23042340
if n > 0:
2305-
x = PY_NEW(Integer)
2341+
mpz_init(x)
23062342
sig_on()
2307-
mpz_pow_ui(x.value, self.value, n)
2343+
mpz_pow_ui(x, self.value, n)
23082344
sig_off()
2309-
return x
2345+
return move_integer_from_mpz(x)
23102346
else:
23112347
if mpz_sgn(self.value) == 0:
23122348
raise ZeroDivisionError("rational division by zero")
2313-
q = Rational.__new__(Rational)
2349+
q = <Rational>PY_NEW(Rational)
23142350
sig_on()
23152351
mpz_pow_ui(mpq_denref(q.value), self.value, -n)
23162352
if mpz_sgn(mpq_denref(q.value)) > 0:
@@ -3584,7 +3620,7 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
35843620
ZeroDivisionError: rational reconstruction with zero modulus
35853621
"""
35863622
cdef Integer a
3587-
cdef Rational x = <Rational>Rational.__new__(Rational)
3623+
cdef Rational x = <Rational>PY_NEW(Rational)
35883624
try:
35893625
mpq_rational_reconstruction(x.value, self.value, m.value)
35903626
except ValueError:
@@ -6888,8 +6924,7 @@ cdef class Integer(sage.structure.element.EuclideanDomainElement):
68886924
"""
68896925
if mpz_sgn(self.value) == 0:
68906926
raise ZeroDivisionError("rational division by zero")
6891-
cdef Rational x
6892-
x = <Rational> Rational.__new__(Rational)
6927+
cdef Rational x = <Rational>PY_NEW(Rational)
68936928
mpz_set_ui(mpq_numref(x.value), 1)
68946929
mpz_set(mpq_denref(x.value), self.value)
68956930
if mpz_sgn(self.value) == -1:
@@ -7509,8 +7544,6 @@ cdef int sizeof_Integer
75097544
# from. DO NOT INITIALIZE IT AGAIN and DO NOT REFERENCE IT!
75107545
cdef Integer global_dummy_Integer
75117546
global_dummy_Integer = Integer()
7512-
# Reallocate to one limb to fix :issue:`31340` and :issue:`33081`
7513-
_mpz_realloc(global_dummy_Integer.value, 1)
75147547

75157548

75167549
def _check_global_dummy_Integer():
@@ -7523,10 +7556,10 @@ def _check_global_dummy_Integer():
75237556
sage: _check_global_dummy_Integer()
75247557
True
75257558
"""
7526-
# Check that it has exactly one limb allocated
7527-
# This is assumed later in fast_tp_new() :issue:`33081`
7559+
# Check that it has no allocation (requires GMP >= 6.2, see :issue:`31340`)
7560+
# fast_tp_new() later assumes that memcpy this mpz object gives a valid new mpz
75287561
cdef mpz_ptr dummy = global_dummy_Integer.value
7529-
if dummy._mp_alloc == 1 and dummy._mp_size == 0:
7562+
if dummy._mp_alloc == 0 and dummy._mp_size == 0:
75307563
return True
75317564

75327565
raise AssertionError(
@@ -7556,7 +7589,6 @@ cdef PyObject* fast_tp_new(type t, args, kwds) except NULL:
75567589
global integer_pool, integer_pool_count, total_alloc, use_pool
75577590

75587591
cdef PyObject* new
7559-
cdef mpz_ptr new_mpz
75607592

75617593
# for profiling pool usage
75627594
# total_alloc += 1
@@ -7589,12 +7621,10 @@ cdef PyObject* fast_tp_new(type t, args, kwds) except NULL:
75897621
# created before this tp_new started to operate.
75907622
memcpy(new, (<void*>global_dummy_Integer), sizeof_Integer)
75917623

7592-
# We allocate memory for the _mp_d element of the value of this
7593-
# new Integer. We allocate one limb. Normally, one would use
7594-
# mpz_init() for this, but we allocate the memory directly.
7595-
# This saves time both by avoiding extra function calls and
7596-
# because the rest of the mpz struct was already initialized
7597-
# fully using the memcpy above.
7624+
# In sufficiently new versions of GMP, mpz_init() does not allocate
7625+
# any memory. We assume that memcpy a newly-initialized mpz results
7626+
# in a valid new mpz. Normally, one would use mpz_init() for this.
7627+
# This saves time by avoiding extra function calls.
75987628
#
75997629
# What is done here is potentially very dangerous as it reaches
76007630
# deeply into the internal structure of GMP. Consequently things
@@ -7606,10 +7636,6 @@ cdef PyObject* fast_tp_new(type t, args, kwds) except NULL:
76067636
# various internals described here may change in future GMP releases.
76077637
# Applications expecting to be compatible with future releases should use
76087638
# only the documented interfaces described in previous chapters."
7609-
#
7610-
# NOTE: This assumes global_dummy_Integer.value._mp_alloc == 1
7611-
new_mpz = <mpz_ptr>((<Integer>new).value)
7612-
new_mpz._mp_d = <mp_ptr>check_malloc(GMP_LIMB_BITS >> 3)
76137639

76147640
# This line is only needed if Python is compiled in debugging mode
76157641
# './configure --with-pydebug' or SAGE_DEBUG=yes. If that is the
@@ -7622,7 +7648,7 @@ cdef PyObject* fast_tp_new(type t, args, kwds) except NULL:
76227648
# The global_dummy_Integer may have a reference count larger than
76237649
# one, but it is expected that newly created objects have a
76247650
# reference count of one. This is potentially unneeded if
7625-
# everybody plays nice, because the gobal_dummy_Integer has only
7651+
# everybody plays nice, because the global_dummy_Integer has only
76267652
# one reference in that case.
76277653

76287654
# Objects from the pool have reference count zero, so this
@@ -7659,7 +7685,7 @@ cdef void fast_tp_dealloc(PyObject* o) noexcept:
76597685
return
76607686

76617687
# No space in the pool, so just free the mpz_t.
7662-
sig_free(o_mpz._mp_d)
7688+
mpz_clear(o_mpz)
76637689

76647690
# Free the object. This assumes that Py_TPFLAGS_HAVE_GC is not
76657691
# set. If it was set another free function would need to be

0 commit comments

Comments
 (0)