Skip to content

Commit 91b1b76

Browse files
author
Release Manager
committed
gh-40594: Fix segmentation fault in libgap function call Fixes #37026. As explained in https://trofi.github.io/posts/312-the-sagemath- saga.html, the relevant code are the following. ```c static PyObject *__pyx_pf_4sage_4libs_3gap_7element_19GapElement_Functio n_2__call__(struct __pyx_obj_4sage_4libs_3gap_7element_GapElement_Function *__pyx_v_self, PyObject *__pyx_v_args) { PyObject *__pyx_t_6 = NULL; sig_GAP_Enter(); __pyx_t_6 = __Pyx_GetItemInt_List(__pyx_v_a, 2, long, 1, __Pyx_PyInt_From_long, 1, 0, 1); if (unlikely(!__pyx_t_6)) __PYX_ERR(0, 2528, __pyx_L14_error) __pyx_v_result = GAP_CallFunc3Args(__pyx_v_self->__pyx_base.value, ((struct __pyx_obj_4sage_4libs_3gap_7element_GapElement *)__pyx_t_5)->value, ((struct __pyx_obj_4sage_4libs_3gap_7element_GapElement *)__pyx_t_4)->value, ((struct __pyx_obj_4sage_4libs_3gap_7element_GapElement *)__pyx_t_6)->value); __Pyx_DECREF(__pyx_t_6); __pyx_t_6 = 0; GAP_Leave(); __Pyx_XDECREF(__pyx_t_6); } ``` where ```c #define __Pyx_XDECREF(r) do { if((r) == NULL); else {__Pyx_DECREF(r); }} while(0) ``` so that simplifies to ```c t_6 = 0; if (!setjmp()) { t_6 = 1; GAP_CallFunc3Args(); // longjmp inside here t_6 = 0; } if (t_6 != 0) access(t_6); ``` The "easy" fix is to declare `t_6` volatile, but we cannot do that since it's a Cython-generated temporary variable. A better way is to avoid mutating `t_6` between `setjmp()` and `longjmp()`. We look at the original code to see why `t_6` is being mutated in the first place. It's because we're accessing `a[i]`, where `a` is a Python list, so Cython needs to store the result into a Python-reference-counted temporary variable. By changing `a` from a Python list to a C array, the new code becomes ```c sig_GAP_Enter(); __pyx_t_12 = sig_on(); if (unlikely(__pyx_t_12 == ((int)0))) __PYX_ERR(0, 2514, __pyx_L9_error) switch (__pyx_v_n) { case 0: __pyx_v_result = GAP_CallFunc0Args(__pyx_v_self->__pyx_base.value); break; case 1: __pyx_v_result = GAP_CallFunc1Args(__pyx_v_self->__pyx_base.value, (__pyx_v_a[0])); break; case 2: __pyx_v_result = GAP_CallFunc2Args(__pyx_v_self->__pyx_base.value, (__pyx_v_a[0]), (__pyx_v_a[1])); break; case 3: __pyx_v_result = GAP_CallFunc3Args(__pyx_v_self->__pyx_base.value, (__pyx_v_a[0]), (__pyx_v_a[1]), (__pyx_v_a[2])); break; default: __pyx_v_result = GAP_CallFuncList(__pyx_v_self->__pyx_base.value, __pyx_v_arg_list); break; } sig_off(); } GAP_Leave(); goto __pyx_L10; } ``` Moral of the story: **do not touch any Python object within `sig_on()`...`sig_off()`!** The [cysignals documentation](https://cysignals.readthedocs.io/en/latest /interrupt.html) also warned about this: > The code inside `sig_on()` should be pure C or Cython code. If you call any Python code or manipulate any Python object (even something trivial like `x = []`), an interrupt can mess up Python’s internal state. When in doubt, try to use sig_check() instead. ------ This does not violate the GAP API either. ``` Code which uses the GAP API exposed by this header file should sandwich any such calls between uses of the GAP_Enter() and GAP_Leave() macro as follows: int ok = GAP_Enter(); if (ok) { ... // any number of calls to GAP APIs } GAP_Leave(); This is in particular crucial if your code keeps references to any GAP functions in local variables: Calling GAP_Enter() ensures that GAP is aware of such references, and will not garbage collect the referenced objects. Failing to use these macros properly can lead to crashes, or worse, silent memory corruption. You have been warned! ``` ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] 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: #40594 Reported by: user202729 Reviewer(s):
2 parents b08d1e2 + a0e12f5 commit 91b1b76

File tree

1 file changed

+14
-16
lines changed

1 file changed

+14
-16
lines changed

src/sage/libs/gap/element.pyx

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2498,32 +2498,30 @@ cdef class GapElement_Function(GapElement):
24982498
cdef Obj result = NULL
24992499
cdef Obj arg_list
25002500
cdef int n = len(args)
2501-
cdef volatile Obj v2
2502-
2503-
if n > 0 and n <= 3:
2504-
libgap = self.parent()
2505-
a = [x if isinstance(x, GapElement) else libgap(x) for x in args]
2501+
cdef Obj a[3]
2502+
2503+
if n <= 3:
2504+
if not all(isinstance(x, GapElement) for x in args):
2505+
libgap = self.parent()
2506+
args = tuple(x if isinstance(x, GapElement) else libgap(x) for x in args)
2507+
for i in range(n):
2508+
x = args[i]
2509+
a[i] = (<GapElement>x).value
2510+
else:
2511+
arg_list = make_gap_list(args)
25062512

25072513
try:
25082514
sig_GAP_Enter()
25092515
sig_on()
25102516
if n == 0:
25112517
result = GAP_CallFunc0Args(self.value)
25122518
elif n == 1:
2513-
result = GAP_CallFunc1Args(self.value,
2514-
(<GapElement>a[0]).value)
2519+
result = GAP_CallFunc1Args(self.value, a[0])
25152520
elif n == 2:
2516-
result = GAP_CallFunc2Args(self.value,
2517-
(<GapElement>a[0]).value,
2518-
(<GapElement>a[1]).value)
2521+
result = GAP_CallFunc2Args(self.value, a[0], a[1])
25192522
elif n == 3:
2520-
v2 = (<GapElement>a[2]).value
2521-
result = GAP_CallFunc3Args(self.value,
2522-
(<GapElement>a[0]).value,
2523-
(<GapElement>a[1]).value,
2524-
v2)
2523+
result = GAP_CallFunc3Args(self.value, a[0], a[1], a[2])
25252524
else:
2526-
arg_list = make_gap_list(args)
25272525
result = GAP_CallFuncList(self.value, arg_list)
25282526
sig_off()
25292527
finally:

0 commit comments

Comments
 (0)