Hi all, The attached patch fixes #1337, which has exactly the same root cause as the "Salmonella crashes a lot" issue we struggled with back in 2015: http://lists.gnu.org/archive/html/chicken-hackers/2015-10/msg00001.html
I guess we never noticed this because the circumstances are relatively rare: You have to use a foreign callback which calls code that triggers a GC, and this GC needs to happen before the argvector is copied onto the stack by an intermediate function that happens to call another function with a larger number of arguments. I don't know how to make a test case for this that reliably triggers the error. The test case from the ticket happens to trigger it now, but it might not in the future. I also noticed that when the stack is full in C_allocate_vector, we call C_reclaim(allocate_vector_2, c), but we save one more thing than the number of arguments allocate_vector takes. Cheers, Peter
From 27925ad4cb1b0ab0e6445116623096fbaf52c3b2 Mon Sep 17 00:00:00 2001
From: Peter Bex <pe...@more-magic.net>
Date: Wed, 28 Dec 2016 19:11:35 +0100
Subject: [PATCH] Fix crashes in callbacks after GC (#1337).
This was caused by keeping argvector in temp stack, similar to 9eed2742.
Instead, we also copy the argvector into the stack before re-invoking
the trampoline when in a callback context, just like a regular CPS
context.
Also, fix an off-by-one in the number of arguments which
allocate_vector_2 accepts; simply passing the original argcount doesn't
allow for the extra 'mode' argument which gets added onto the front of
the stack.
---
NEWS | 1 +
runtime.c | 15 ++++++++++++---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/NEWS b/NEWS
index 79403b9..38af2d2 100644
--- a/NEWS
+++ b/NEWS
@@ -71,6 +71,7 @@
- Runtime system:
- "time" macro now shows peak memory usage (#1318, thanks to Kooda).
+ - Avoid crashes in ffi callbacks after GC (#1337, thanks to cosarara).
- Core libraries:
- Keywords are more consistently read/written, like symbols (#1332).
diff --git a/runtime.c b/runtime.c
index 146da1b..bfaae1a 100644
--- a/runtime.c
+++ b/runtime.c
@@ -1559,6 +1559,9 @@ C_word CHICKEN_run(void *toplevel)
serious_signal_occurred = 0;
if(!return_to_host) {
+ /* We must copy the argvector onto the stack, because
+ * any subsequent save() will otherwise clobber it.
+ */
int argcount = C_temporary_stack_bottom - C_temporary_stack;
C_word *p = C_alloc(argcount);
@@ -2134,8 +2137,11 @@ C_word C_fcall C_callback(C_word closure, int argc)
serious_signal_occurred = 0;
if(!callback_returned_flag) {
- C_word *p = C_temporary_stack;
-
+ /* We must copy the argvector onto the stack, because
+ * any subsequent save() will otherwise clobber it.
+ */
+ C_word *p = C_alloc(C_restart_c);
+ C_memcpy(p, C_temporary_stack, C_restart_c * sizeof(C_word));
C_temporary_stack = C_temporary_stack_bottom;
((C_proc)C_restart_trampoline)(C_restart_c, p);
}
@@ -9975,7 +9981,10 @@ void C_ccall C_allocate_vector(C_word c, C_word *av)
C_fromspace_top = C_fromspace_limit; /* trigger major GC */
C_save(C_SCHEME_TRUE);
- C_reclaim((void *)allocate_vector_2, c);
+ /* We explicitly pass 7 here, that's the number of things saved.
+ * That's the arguments, plus one additional thing: the mode.
+ */
+ C_reclaim((void *)allocate_vector_2, 7);
}
C_save(C_SCHEME_FALSE);
--
2.1.4
From a48ce1449a873203f2b8c11c0f1d9facf7b57468 Mon Sep 17 00:00:00 2001
From: Peter Bex <pe...@more-magic.net>
Date: Wed, 28 Dec 2016 19:04:12 +0100
Subject: [PATCH] Fix crashes in callbacks after GC (#1337).
This was caused by keeping argvector in temp stack, similar to 9eed2742.
Instead, we also copy the argvector into the stack before re-invoking
the trampoline when in a callback context, just like a regular CPS
context.
Also, fix an off-by-one in the number of arguments which
allocate_vector_2 accepts; simply passing the original argcount doesn't
allow for the extra 'mode' argument which gets added onto the front of
the stack.
---
NEWS | 1 +
runtime.c | 15 ++++++++++++---
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/NEWS b/NEWS
index 6a34708..ac51659 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,7 @@
- Runtime system:
- "time" macro now shows peak memory usage (#1318, thanks to Kooda).
+ - Avoid crashes in ffi callbacks after GC (#1337, thanks to cosarara).
- Core libraries:
- Keywords are more consistently read/written, like symbols (#1332).
diff --git a/runtime.c b/runtime.c
index 4b6300f..d9ee000 100644
--- a/runtime.c
+++ b/runtime.c
@@ -1504,6 +1504,9 @@ C_word CHICKEN_run(void *toplevel)
serious_signal_occurred = 0;
if(!return_to_host) {
+ /* We must copy the argvector onto the stack, because
+ * any subsequent save() will otherwise clobber it.
+ */
int argcount = C_temporary_stack_bottom - C_temporary_stack;
C_word *p = C_alloc(argcount);
C_memcpy(p, C_temporary_stack, argcount * sizeof(C_word));
@@ -2007,8 +2010,11 @@ C_word C_fcall C_callback(C_word closure, int argc)
serious_signal_occurred = 0;
if(!callback_returned_flag) {
- C_word *p = C_temporary_stack;
-
+ /* We must copy the argvector onto the stack, because
+ * any subsequent save() will otherwise clobber it.
+ */
+ C_word *p = C_alloc(C_restart_c);
+ C_memcpy(p, C_temporary_stack, C_restart_c * sizeof(C_word));
C_temporary_stack = C_temporary_stack_bottom;
((C_proc)C_restart_trampoline)(C_restart_c, p);
}
@@ -7508,7 +7514,10 @@ void C_ccall C_allocate_vector(C_word c, C_word *av)
C_fromspace_top = C_fromspace_limit; /* trigger major GC */
C_save(C_SCHEME_TRUE);
- C_reclaim((void *)allocate_vector_2, c);
+ /* We explicitly pass 7 here, that's the number of things saved.
+ * That's the arguments, plus one additional thing: the mode.
+ */
+ C_reclaim((void *)allocate_vector_2, 7);
}
C_save(C_SCHEME_FALSE);
--
2.1.4
signature.asc
Description: Digital signature
_______________________________________________ Chicken-hackers mailing list Chicken-hackers@nongnu.org https://lists.nongnu.org/mailman/listinfo/chicken-hackers
