From b79b3488c89bad3e64045622b599377d233123ca Mon Sep 17 00:00:00 2001 From: Jeff Bezanson Date: Tue, 12 Jun 2018 01:25:11 -0400 Subject: [PATCH] fix #26939, segfault in WeakKeyDict due to wrong finalization order --- src/gc.c | 106 +++++++++++++++++---------------------------------- test/dict.jl | 5 +++ 2 files changed, 41 insertions(+), 70 deletions(-) diff --git a/src/gc.c b/src/gc.c index b5aa4c4a4c02de..ea41778f2b9eeb 100644 --- a/src/gc.c +++ b/src/gc.c @@ -55,10 +55,6 @@ pagetable_t memory_map; bigval_t *big_objects_marked = NULL; // finalization -// `ptls->finalizers` and `finalizer_list_marked` might have tagged pointers. -// If an object pointer has the lowest bit set, the next pointer is an unboxed -// c function pointer. -// `to_finalize` should not have tagged pointers. arraylist_t finalizer_list_marked; arraylist_t to_finalize; @@ -104,7 +100,10 @@ static void schedule_finalization(void *o, void *f) static void run_finalizer(jl_ptls_t ptls, jl_value_t *o, jl_value_t *ff) { - assert(!jl_typeis(ff, jl_voidpointer_type)); + if (jl_is_cpointer(ff)) { + ((void (*)(void*))jl_unbox_voidpointer(ff))(o); + return; + } jl_value_t *args[2] = {ff,o}; JL_TRY { size_t last_age = jl_get_ptls_states()->world_age; @@ -137,16 +136,11 @@ static void finalize_object(arraylist_t *list, jl_value_t *o, for (size_t i = 0; i < len; i += 2) { void *v = items[i]; int move = 0; - if (o == (jl_value_t*)gc_ptr_clear_tag(v, 1)) { + if (o == (jl_value_t*)v) { void *f = items[i + 1]; move = 1; - if (gc_ptr_tag(v, 1)) { - ((void (*)(void*))(uintptr_t)f)(o); - } - else { - arraylist_push(copied_list, o); - arraylist_push(copied_list, f); - } + arraylist_push(copied_list, o); + arraylist_push(copied_list, f); } if (move || __unlikely(!v)) { if (i < len - 2) { @@ -187,9 +181,12 @@ static void jl_gc_push_arraylist(jl_ptls_t ptls, arraylist_t *list) // function returns. static void jl_gc_run_finalizers_in_list(jl_ptls_t ptls, arraylist_t *list) { - size_t len = list->len; - jl_value_t **items = (jl_value_t**)list->items; + // empty out the first two entries for the GC frame + arraylist_push(list, list->items[0]); + arraylist_push(list, list->items[1]); jl_gc_push_arraylist(ptls, list); + jl_value_t **items = (jl_value_t**)list->items; + size_t len = list->len; JL_UNLOCK_NOGC(&finalizers_lock); // from jl_apply_with_saved_exception_state; to hoist state saving out of the loop jl_value_t *exc = ptls->exception_in_transit; @@ -200,8 +197,11 @@ static void jl_gc_run_finalizers_in_list(jl_ptls_t ptls, arraylist_t *list) jl_get_backtrace(&bt, &bt2); ptls->bt_size = 0; } - for (size_t i = 2;i < len;i += 2) + // run finalizers in reverse order they were added, so lower-level finalizers run last + for (size_t i = len-4; i >= 2; i -= 2) run_finalizer(ptls, items[i], items[i + 1]); + // first entries were moved last to make room for GC frame metadata + run_finalizer(ptls, items[len-2], items[len-1]); ptls->exception_in_transit = exc; if (bt != NULL) { // This is sufficient because bt2 roots the managed values @@ -233,9 +233,6 @@ static void run_finalizers(jl_ptls_t ptls) copied_list.items = copied_list._space; } arraylist_new(&to_finalize, 0); - // empty out the first two entries for the GC frame - arraylist_push(&copied_list, copied_list.items[0]); - arraylist_push(&copied_list, copied_list.items[1]); // This releases the finalizers lock. jl_gc_run_finalizers_in_list(ptls, &copied_list); arraylist_free(&copied_list); @@ -262,12 +259,7 @@ static void schedule_all_finalizers(arraylist_t *flist) void *f = items[i + 1]; if (__unlikely(!v)) continue; - if (!gc_ptr_tag(v, 1)) { - schedule_finalization(v, f); - } - else { - ((void (*)(void*))(uintptr_t)f)(gc_ptr_clear_tag(v, 1)); - } + schedule_finalization(v, f); } flist->len = 0; } @@ -311,25 +303,17 @@ static void gc_add_finalizer_(jl_ptls_t ptls, void *v, void *f) jl_gc_unsafe_leave(ptls, gc_state); } -STATIC_INLINE void gc_add_ptr_finalizer(jl_ptls_t ptls, jl_value_t *v, void *f) +JL_DLLEXPORT void jl_gc_add_finalizer_th(jl_ptls_t ptls, jl_value_t *v, jl_function_t *f) { - gc_add_finalizer_(ptls, (void*)(((uintptr_t)v) | 1), f); -} - -JL_DLLEXPORT void jl_gc_add_finalizer_th(jl_ptls_t ptls, jl_value_t *v, - jl_function_t *f) -{ - if (__unlikely(jl_typeis(f, jl_voidpointer_type))) { - gc_add_ptr_finalizer(ptls, v, jl_unbox_voidpointer(f)); - } - else { - gc_add_finalizer_(ptls, v, f); - } + gc_add_finalizer_(ptls, v, f); } JL_DLLEXPORT void jl_gc_add_ptr_finalizer(jl_ptls_t ptls, jl_value_t *v, void *f) { - gc_add_ptr_finalizer(ptls, v, f); + jl_value_t *rt = jl_box_voidpointer(f); + JL_GC_PUSH1(&rt); + gc_add_finalizer_(ptls, v, rt); + JL_GC_POP(); } JL_DLLEXPORT void jl_finalize_th(jl_ptls_t ptls, jl_value_t *o) @@ -340,8 +324,6 @@ JL_DLLEXPORT void jl_finalize_th(jl_ptls_t ptls, jl_value_t *o) // This list is also used as the GC frame when we are running the finalizers arraylist_t copied_list; arraylist_new(&copied_list, 0); - arraylist_push(&copied_list, NULL); // GC frame size to be filled later - arraylist_push(&copied_list, NULL); // pgcstack to be filled later // No need to check the to_finalize list since the user is apparently // still holding a reference to the object for (int i = 0;i < jl_n_threads;i++) { @@ -349,7 +331,7 @@ JL_DLLEXPORT void jl_finalize_th(jl_ptls_t ptls, jl_value_t *o) finalize_object(&ptls2->finalizers, o, &copied_list, ptls != ptls2); } finalize_object(&finalizer_list_marked, o, &copied_list, 0); - if (copied_list.len > 2) { + if (copied_list.len > 0) { // This releases the finalizers lock. jl_gc_run_finalizers_in_list(ptls, &copied_list); } @@ -1991,11 +1973,6 @@ finlist: { new_obj = *begin; if (__unlikely(!new_obj)) continue; - if (gc_ptr_tag(new_obj, 1)) { - new_obj = (jl_value_t*)gc_ptr_clear_tag(new_obj, 1); - begin++; - assert(begin < end); - } uintptr_t nptr = 0; if (!gc_try_setmark(new_obj, &nptr, &tag, &bits)) continue; @@ -2293,18 +2270,11 @@ static void sweep_finalizer_list(arraylist_t *list) { void **items = list->items; size_t len = list->len; + size_t j = 0; for (size_t i=0; i < len; i+=2) { - void *v0 = items[i]; - int is_cptr = gc_ptr_tag(v0, 1); - void *v = gc_ptr_clear_tag(v0, 1); - if (__unlikely(!v0)) { + void *v = items[i]; + if (__unlikely(!v)) { // remove from this list - if (i < len - 2) { - items[i] = items[len - 2]; - items[i + 1] = items[len - 1]; - i -= 2; - } - len -= 2; continue; } @@ -2312,32 +2282,28 @@ static void sweep_finalizer_list(arraylist_t *list) int isfreed = !gc_marked(jl_astaggedvalue(v)->bits.gc); int isold = (list != &finalizer_list_marked && jl_astaggedvalue(v)->bits.gc == GC_OLD_MARKED && - (is_cptr || jl_astaggedvalue(fin)->bits.gc == GC_OLD_MARKED)); + jl_astaggedvalue(fin)->bits.gc == GC_OLD_MARKED); if (isfreed || isold) { // remove from this list - if (i < len - 2) { - items[i] = items[len - 2]; - items[i + 1] = items[len - 1]; - i -= 2; + } + else { + if (j < i) { + items[j] = items[i]; + items[j+1] = items[i+1]; } - len -= 2; + j += 2; } if (isfreed) { - // schedule finalizer or execute right away if it is not julia code - if (is_cptr) { - ((void (*)(void*))(uintptr_t)fin)(jl_data_ptr(v)); - continue; - } schedule_finalization(v, fin); } if (isold) { // The caller relies on the new objects to be pushed to the end of // the list!! - arraylist_push(&finalizer_list_marked, v0); + arraylist_push(&finalizer_list_marked, v); arraylist_push(&finalizer_list_marked, fin); } } - list->len = len; + list->len = j; } // collector entry point and control diff --git a/test/dict.jl b/test/dict.jl index 912209b2efa6c9..ec5dabab136e44 100644 --- a/test/dict.jl +++ b/test/dict.jl @@ -829,6 +829,11 @@ Dict(1 => rand(2,3), 'c' => "asdf") # just make sure this does not trigger a dep @test isa(wkd, WeakKeyDict) @test_throws ArgumentError WeakKeyDict([1, 2, 3]) + + # issue #26939 + d26939 = WeakKeyDict() + d26939[big"1.0" + 1.1] = 1 + GC.gc() # make sure this doesn't segfault end @testset "issue #19995, hash of dicts" begin