From 1faf857230dd1e494aaeab08a6b6cf099920f1ef Mon Sep 17 00:00:00 2001 From: Peter Bex Date: Fri, 1 Dec 2017 21:29:17 +0100 Subject: [PATCH] Fix calls to C_reclaim to use the number of saved objects (#1428) When the argvector change was introduced, several C_reclaim calls were incorrectly changed to use the size of the argvector to the C function that would invoke C_reclaim. In many cases we don't save all the arguments, which means "c" would be larger than the size of the temporary stack. The value of "c" gets set to C_restart_c, which will be used in the setjmp trampoline as the size of the buffer to set aside for the re-invoked closure's argvector. Then, we read C_restart_c words from the stack top, which might overshoot the temporary stack bottom, which means we're potentially reading beyond malloced memory. Finally, CHICKEN_initialize would save 2 objects on the temp stack but neglected to set C_restart_c to the correct value. This appeared to work only because CHICKEN_run happened to look at the effective stack size rather than the restart count. Now, we use the restart count and also assert that it's always identical to the stack size. Any mismatch must be due to a bug. This is not an exploitable bug because the copied bytes are inaccessible: they're lying just beyond the highest argument in the argvector, which is inaccessible from Scheme. The worst that can happen is that we try to read from a page that doesn't belong to us and we'll segfault. --- NEWS | 2 ++ runtime.c | 19 ++++++++++--------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/NEWS b/NEWS index 10d677b9..f3b00b40 100644 --- a/NEWS +++ b/NEWS @@ -167,6 +167,8 @@ - Correctly calculate memory requirements of Scheme objects produced from foreign types with "const" qualifiers, avoiding memory corruption (#1424, thanks to Vasilij Schneidermann and Lemonboy) + - Do not read beyond temporary stack buffer, which could lead to + a crash when returning from a foreign callback (#1428). 4.12.0 diff --git a/runtime.c b/runtime.c index 499fc922..ba79a917 100644 --- a/runtime.c +++ b/runtime.c @@ -845,6 +845,7 @@ int CHICKEN_initialize(int heap, int stack, int symbols, void *toplevel) C_set_block_item(k0, 0, (C_word)termination_continuation); C_save(k0); C_save(C_SCHEME_UNDEFINED); + C_restart_c = 2; return 1; } @@ -1536,10 +1537,9 @@ C_word CHICKEN_run(void *toplevel) /* 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)); + C_word *p = C_alloc(C_restart_c); + assert(C_restart_c == (C_temporary_stack_bottom - C_temporary_stack)); + 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); } @@ -2115,6 +2115,7 @@ C_word C_fcall C_callback(C_word closure, int argc) * any subsequent save() will otherwise clobber it. */ C_word *p = C_alloc(C_restart_c); + assert(C_restart_c == (C_temporary_stack_bottom - C_temporary_stack)); 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); @@ -9421,7 +9422,7 @@ void C_ccall C_gc(C_word c, C_word *av) } else if(f) C_fromspace_top = C_fromspace_limit; - C_reclaim((void *)gc_2, c); + C_reclaim((void *)gc_2, 1); } @@ -10740,7 +10741,7 @@ void C_ccall C_ensure_heap_reserve(C_word c, C_word *av) C_save(k); if(!C_demand(C_bytestowords(C_unfix(n)))) - C_reclaim((void *)generic_trampoline, c); + C_reclaim((void *)generic_trampoline, 1); p = C_temporary_stack; C_temporary_stack = C_temporary_stack_bottom; @@ -10764,7 +10765,7 @@ void C_ccall C_return_to_host(C_word c, C_word *av) return_to_host = 1; C_save(k); - C_reclaim((void *)generic_trampoline, c); + C_reclaim((void *)generic_trampoline, 1); } @@ -12245,7 +12246,7 @@ void C_ccall C_dump_heap_state(C_word c, C_word *av) /* make sure heap is compacted */ C_save(k); C_fromspace_top = C_fromspace_limit; /* force major GC */ - C_reclaim((void *)dump_heap_state_2, c); + C_reclaim((void *)dump_heap_state_2, 1); } @@ -12473,7 +12474,7 @@ void C_ccall C_filter_heap_objects(C_word c, C_word *av) C_save(userarg); C_save(func); C_fromspace_top = C_fromspace_limit; /* force major GC */ - C_reclaim((void *)filter_heap_objects_2, c); + C_reclaim((void *)filter_heap_objects_2, 4); } C_regparm C_word C_fcall C_i_process_sleep(C_word n) -- 2.11.0