Skip to content

Commit

Permalink
fix #26939, segfault in WeakKeyDict due to wrong finalization order
Browse files Browse the repository at this point in the history
  • Loading branch information
@JeffBezanson
JeffBezanson committed Jun 12, 2018
1 parent 83a0dd7 commit b79b348
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 70 deletions.
106 changes: 36 additions & 70 deletions src/gc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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)
Expand All @@ -340,16 +324,14 @@ 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++) {
jl_ptls_t ptls2 = jl_all_tls_states[i];
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);
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -2293,51 +2270,40 @@ 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;
}

void *fin = items[i+1];
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
Expand Down
5 changes: 5 additions & 0 deletions test/dict.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit b79b348

Please sign in to comment.