From cf69717819e0477370f8be31dfbaee54a8902664 Mon Sep 17 00:00:00 2001 From: Tobias Hansen Date: Sat, 24 Oct 2026 20:57:33 +0100 Subject: [PATCH 1/2] Fix GAP objects in local variables of the caller of `GAP_Enter` being garbage collected on 32-bit ARM GAP_EnterStack() recorded __builtin_frame_address(0) as the stack bottom for GASMAN. On some architectures, such as 32-bit ARM in Thumb mode, this address lies below the local variables of the calling function, so GAP objects referenced only from them were not marked and could be freed while still in use, causing crashes and memory corruption in e.g. SageMath. Use the canonical frame address instead, which is the top of the stack frame. Assisted-by: Claude Code (Opus 5.5) --- .gitignore | 1 + Makefile.rules | 2 +- src/libgap-api.h | 29 +++++++++++++++++++----- tst/testlibgap/enterstack.c | 39 ++++++++++++++++++++++++++++++++ tst/testlibgap/enterstack.expect | 1 + 5 files changed, 65 insertions(+), 7 deletions(-) create mode 100644 tst/testlibgap/enterstack.c create mode 100644 tst/testlibgap/enterstack.expect diff --git a/.gitignore b/.gitignore index 2116ce6466..e9a08cdef5 100644 --- a/.gitignore +++ b/.gitignore @@ -114,6 +114,7 @@ doc/ref/user_pref_list.xml /tst/testlibgap/wscreate /tst/testlibgap/wsload /tst/testlibgap/trycatch +/tst/testlibgap/enterstack /tst/testlibgap/*.out /tst/testkernel/dstruct diff --git a/Makefile.rules b/Makefile.rules index cfbf627a15..b15967665c 100644 --- a/Makefile.rules +++ b/Makefile.rules @@ -1050,7 +1050,7 @@ check: all citests: all SRCDIR=$(abs_srcdir) dev/ci.sh testmockpkg testspecial test-compile testlibgap testkernel testworkspace -LIBGAPTESTS := $(addprefix tst/testlibgap/,basic api wscreate wsload trycatch) +LIBGAPTESTS := $(addprefix tst/testlibgap/,basic api wscreate wsload trycatch enterstack) build-testlibgap: ${LIBGAPTESTS} diff --git a/src/libgap-api.h b/src/libgap-api.h index 8abfae030b..edd43d5b0b 100644 --- a/src/libgap-api.h +++ b/src/libgap-api.h @@ -78,16 +78,33 @@ static inline int GAP_Error_Postjmp_(int JumpRet) // entered recursively (you don't want the GAP error handling code to cause a // longjmp into the error callback itself since then the error callback will // never be returned from). -#ifdef __GNUC__ +// +// GASMAN scans the stack from the address passed to GAP_EnterStack_() down +// to the stack pointer, so it must lie above the caller's local variables. +// We use the canonical frame address, the top of the caller's stack frame; +// __builtin_frame_address(0) is not enough, as e.g. on 32-bit ARM in Thumb +// mode it points to the bottom of the frame. +#if defined(__has_builtin) +#if __has_builtin(__builtin_dwarf_cfa) && !defined(__EMSCRIPTEN__) +#define GAP_STACK_FRAME_TOP_() __builtin_dwarf_cfa() +#endif +#elif defined(__GNUC__) +#define GAP_STACK_FRAME_TOP_() __builtin_dwarf_cfa() +#endif +#if !defined(GAP_STACK_FRAME_TOP_) && defined(__GNUC__) +#define GAP_STACK_FRAME_TOP_() __builtin_frame_address(0) +#endif + +#ifdef GAP_STACK_FRAME_TOP_ #define GAP_EnterStack() \ do { \ GAP_ENTER_DEBUG_MESSAGE("EnterStack", __FILE__, __LINE__); \ - GAP_EnterStack_(__builtin_frame_address(0)); \ + GAP_EnterStack_(GAP_STACK_FRAME_TOP_()); \ } while (0) #elif defined(USE_GASMAN) -#error GASMAN requires a way to get the current stack frame base address \ +#error GASMAN requires a way to get the top of the current stack frame \ for the GAP_EnterStack() macro; normally this uses the \ - __builtin_frame_address GNU extension so if this is not available \ + __builtin_dwarf_cfa GNU extension so if this is not available \ it is necessary to provide your own implementation here. #else // If we're not using GASMAN in the first place GAP_EnterStack_() is not @@ -136,8 +153,8 @@ static inline int GAP_Error_Postjmp_(int JumpRet) // is the return value of GAP_Error_Setjmp). // // * GAP_EnterStack() defined and explained above must be a macro since it -// needs to figure out (to the extent possible) the base address of the -// stack frame from which it is called. +// needs to figure out (to the extent possible) the top of the stack frame +// from which it is called. // // * GAP_Error_Setjmp() effectively calls setjmp to the STATE(ReadJmpError) // longjmp buffer, so that read errors which occur in GAP that are not diff --git a/tst/testlibgap/enterstack.c b/tst/testlibgap/enterstack.c new file mode 100644 index 0000000000..547bd3c2ac --- /dev/null +++ b/tst/testlibgap/enterstack.c @@ -0,0 +1,39 @@ +/* + * Test that GAP objects referenced only from local variables of the + * function calling GAP_Enter() survive garbage collection + */ +#include "common.h" + +// overwrite stale copies of GAP objects left on the stack by earlier calls +static __attribute__((noinline)) void clobber_stack(void) +{ + char volatile buf[4096]; + for (size_t i = 0; i < sizeof(buf); i++) + buf[i] = 0; +} + +static void test_local(void) +{ + // volatile keeps the list in this function's stack frame + Obj volatile list; + int ok = GAP_Enter(); + if (ok) { + list = GAP_NewPlist(1); + GAP_AssList(list, 1, GAP_NewObjIntFromInt(42)); + clobber_stack(); + GAP_CollectBags(1); + // reuse the memory of the list if it was wrongly freed + for (int i = 0; i < 1000; i++) + GAP_NewPlist(1); + printf("%d %d\n", (int)GAP_LenList(list), + (int)GAP_ValueInt(GAP_ElmList(list, 1))); + } + GAP_Leave(); +} + +int main(int argc, char ** argv) +{ + GAP_Initialize(argc, argv, 0, 0, 1); + test_local(); + return 0; +} diff --git a/tst/testlibgap/enterstack.expect b/tst/testlibgap/enterstack.expect new file mode 100644 index 0000000000..a07a6bfa29 --- /dev/null +++ b/tst/testlibgap/enterstack.expect @@ -0,0 +1 @@ +1 42 From 6c97f47cb570e9d776d390e4a2946ccd7de5a6cf Mon Sep 17 00:00:00 2001 From: Tobias Hansen Date: Mon, 26 Oct 2026 18:14:06 +0000 Subject: [PATCH 2/2] libgap: Keep using __builtin_frame_address on Windows clang computes a wrong canonical frame address on Windows, which would break Cygwin, so restore the previous behaviour there. Assisted-by: Claude Code (Opus 5.5) --- src/libgap-api.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libgap-api.h b/src/libgap-api.h index edd43d5b0b..fdcfae7272 100644 --- a/src/libgap-api.h +++ b/src/libgap-api.h @@ -84,7 +84,10 @@ static inline int GAP_Error_Postjmp_(int JumpRet) // We use the canonical frame address, the top of the caller's stack frame; // __builtin_frame_address(0) is not enough, as e.g. on 32-bit ARM in Thumb // mode it points to the bottom of the frame. -#if defined(__has_builtin) +#if defined(_WIN32) || defined(__CYGWIN__) +// keep using __builtin_frame_address(0), as clang's __builtin_dwarf_cfa() is +// wrong on Windows, see https://github.com/llvm/llvm-project/issues/227606 +#elif defined(__has_builtin) #if __has_builtin(__builtin_dwarf_cfa) && !defined(__EMSCRIPTEN__) #define GAP_STACK_FRAME_TOP_() __builtin_dwarf_cfa() #endif