From a9ddf0420a704822e47b97a4189c829500aeeaf8 Mon Sep 17 00:00:00 2001 From: cxzhong Date: Thu, 14 Aug 2025 17:30:26 +0800 Subject: [PATCH 1/5] Fix GAP libgap segfault in function calls with >3 arguments This fixes an inconsistent behavior where libgap function calls with more than 3 arguments would sometimes return normal GAP errors and sometimes cause segmentation faults. The root cause was nested GAP_Enter() calls: the main function call used sig_GAP_Enter(), and then make_gap_list() called GAP_Enter() again, causing race conditions in GAP's memory management. The fix replaces GAP_CallFuncList() with GAP_CallFuncArray() and uses C malloc/free for temporary argument arrays instead of creating GAP list objects, eliminating the nested GAP memory management calls. This ensures consistent error handling - invalid calls now always return proper GAP error messages instead of sometimes segfaulting. Fixes: Inconsistent libgap.Sum(*[1,2,3]) behavior (segfault vs GAPError) --- src/sage/libs/gap/element.pyx | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index f52a73c2ded..c71a5773aa0 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -28,6 +28,7 @@ from sage.cpython.string cimport str_to_bytes, char_to_str from sage.rings.integer_ring import ZZ from sage.rings.rational_field import QQ from sage.rings.real_double import RDF +from libc.stdlib cimport malloc, free from sage.groups.perm_gps.permgroup_element cimport PermutationGroupElement from sage.combinat.permutation import Permutation @@ -2496,11 +2497,12 @@ cdef class GapElement_Function(GapElement): hello from the shell """ cdef Obj result = NULL - cdef Obj arg_list cdef int n = len(args) cdef volatile Obj v2 + cdef Obj *arg_array = NULL + cdef int i - if n > 0 and n <= 3: + if n > 0: libgap = self.parent() a = [x if isinstance(x, GapElement) else libgap(x) for x in args] @@ -2523,8 +2525,17 @@ cdef class GapElement_Function(GapElement): (a[1]).value, v2) else: - arg_list = make_gap_list(args) - result = GAP_CallFuncList(self.value, arg_list) + # Use GAP_CallFuncArray instead of GAP_CallFuncList + # to avoid creating GAP list objects and nested GAP_Enter calls + arg_array = malloc(n * sizeof(Obj)) + if arg_array == NULL: + raise MemoryError("Failed to allocate memory for GAP function arguments") + + for i in range(n): + arg_array[i] = (a[i]).value + + result = GAP_CallFuncArray(self.value, n, arg_array) + free(arg_array) sig_off() finally: GAP_Leave() From b918272422182b4613ed1e21f3951e771b1c4f0a Mon Sep 17 00:00:00 2001 From: Chenxin Zhong Date: Thu, 14 Aug 2025 17:42:48 +0800 Subject: [PATCH 2/5] Delete some blank --- src/sage/libs/gap/element.pyx | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index c71a5773aa0..bfad5390551 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -2530,10 +2530,8 @@ cdef class GapElement_Function(GapElement): arg_array = malloc(n * sizeof(Obj)) if arg_array == NULL: raise MemoryError("Failed to allocate memory for GAP function arguments") - for i in range(n): arg_array[i] = (a[i]).value - result = GAP_CallFuncArray(self.value, n, arg_array) free(arg_array) sig_off() From f7d17ecb0678a4f7f8f99d05c5f43f45c0f75aac Mon Sep 17 00:00:00 2001 From: cxzhong Date: Fri, 15 Aug 2025 22:20:30 +0800 Subject: [PATCH 3/5] Ensure memory is freed in GAP function calls even if an error occurs --- src/sage/libs/gap/element.pyx | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index bfad5390551..b5c42ca33df 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -2530,10 +2530,12 @@ cdef class GapElement_Function(GapElement): arg_array = malloc(n * sizeof(Obj)) if arg_array == NULL: raise MemoryError("Failed to allocate memory for GAP function arguments") - for i in range(n): - arg_array[i] = (a[i]).value - result = GAP_CallFuncArray(self.value, n, arg_array) - free(arg_array) + try: + for i in range(n): + arg_array[i] = (a[i]).value + result = GAP_CallFuncArray(self.value, n, arg_array) + finally: + free(arg_array) sig_off() finally: GAP_Leave() From 1d7dba4a6ae2b78bb516402eac085194fd886fcd Mon Sep 17 00:00:00 2001 From: cxzhong Date: Fri, 15 Aug 2025 22:49:56 +0800 Subject: [PATCH 4/5] Fix reference counting in GAP function calls to prevent garbage collection issues --- src/sage/libs/gap/element.pyx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index b5c42ca33df..335e8d1f3a3 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -16,6 +16,7 @@ elements. For general information about GAP, you should read the # **************************************************************************** from cpython.object cimport Py_EQ, Py_NE, Py_LE, Py_GE, Py_LT, Py_GT +from cpython.ref cimport Py_INCREF, Py_DECREF from cysignals.signals cimport sig_on, sig_off from sage.libs.gap.gap_includes cimport * @@ -2501,6 +2502,7 @@ cdef class GapElement_Function(GapElement): cdef volatile Obj v2 cdef Obj *arg_array = NULL cdef int i + cdef int refs_incremented = 0 if n > 0: libgap = self.parent() @@ -2530,11 +2532,18 @@ cdef class GapElement_Function(GapElement): arg_array = malloc(n * sizeof(Obj)) if arg_array == NULL: raise MemoryError("Failed to allocate memory for GAP function arguments") + + # Increment reference counts first to prevent GC of GapElement objects try: for i in range(n): + Py_INCREF(a[i]) + refs_incremented = i + 1 arg_array[i] = (a[i]).value result = GAP_CallFuncArray(self.value, n, arg_array) finally: + # Decrement reference counts for all successfully incremented refs + for i in range(refs_incremented): + Py_DECREF(a[i]) free(arg_array) sig_off() finally: From 43f9064277bebe923bde902d144f12029281e5c4 Mon Sep 17 00:00:00 2001 From: cxzhong Date: Fri, 15 Aug 2025 23:23:02 +0800 Subject: [PATCH 5/5] Enhance memory management in GAP function calls to prevent garbage collection issues with multiple arguments --- src/sage/libs/gap/element.pyx | 44 ++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/src/sage/libs/gap/element.pyx b/src/sage/libs/gap/element.pyx index 335e8d1f3a3..e61137f5dae 100644 --- a/src/sage/libs/gap/element.pyx +++ b/src/sage/libs/gap/element.pyx @@ -29,7 +29,8 @@ from sage.cpython.string cimport str_to_bytes, char_to_str from sage.rings.integer_ring import ZZ from sage.rings.rational_field import QQ from sage.rings.real_double import RDF -from libc.stdlib cimport malloc, free +from libc.stdlib cimport malloc, calloc, free +from libc.string cimport memset from sage.groups.perm_gps.permgroup_element cimport PermutationGroupElement from sage.combinat.permutation import Permutation @@ -2497,12 +2498,11 @@ cdef class GapElement_Function(GapElement): sage: libgap_exec('echo hello from the shell') hello from the shell """ - cdef Obj result = NULL + cdef volatile Obj result = NULL cdef int n = len(args) cdef volatile Obj v2 - cdef Obj *arg_array = NULL - cdef int i - cdef int refs_incremented = 0 + cdef volatile Obj *arg_array = NULL + cdef int i, refs_incremented = 0 if n > 0: libgap = self.parent() @@ -2527,24 +2527,40 @@ cdef class GapElement_Function(GapElement): (a[1]).value, v2) else: - # Use GAP_CallFuncArray instead of GAP_CallFuncList - # to avoid creating GAP list objects and nested GAP_Enter calls - arg_array = malloc(n * sizeof(Obj)) + # ULTRA-ROBUST MEMORY MANAGEMENT for n >= 4 arguments + # Following the comprehensive approach to prevent all GC issues + + # 1) Pin all Python GapElement objects BEFORE entering GAP region + for i in range(n): + Py_INCREF(a[i]) + refs_incremented = n + + # 2) Allocate and zero-initialize the arg array + arg_array = calloc(n, sizeof(Obj)) if arg_array == NULL: + # rollback INCREFs on allocation failure + for i in range(refs_incremented): + Py_DECREF(a[i]) + refs_incremented = 0 raise MemoryError("Failed to allocate memory for GAP function arguments") - # Increment reference counts first to prevent GC of GapElement objects try: + # 3) Fill arg_array from pinned 'a' (these a[i] won't be GC'd now) for i in range(n): - Py_INCREF(a[i]) - refs_incremented = i + 1 arg_array[i] = (a[i]).value - result = GAP_CallFuncArray(self.value, n, arg_array) + + # 4) Single GAP call - all objects are protected + result = GAP_CallFuncArray(self.value, n, arg_array) + finally: - # Decrement reference counts for all successfully incremented refs + # 5) cleanup: free array and unpin Python objs + if arg_array != NULL: + free(arg_array) + arg_array = NULL + # DECREF exactly refs_incremented times for i in range(refs_incremented): Py_DECREF(a[i]) - free(arg_array) + refs_incremented = 0 sig_off() finally: GAP_Leave()