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 d04bf35
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 57 deletions.
101 changes: 44 additions & 57 deletions src/gc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,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 (gc_ptr_tag(o, 1)) {
((void (*)(void*))ff)(gc_ptr_clear_tag(o, 1));
return;
}
jl_value_t *args[2] = {ff,o};
JL_TRY {
size_t last_age = jl_get_ptls_states()->world_age;
Expand Down Expand Up @@ -134,29 +137,28 @@ static void finalize_object(arraylist_t *list, jl_value_t *o,
size_t len = need_sync ? jl_atomic_load_acquire(&list->len) : list->len;
size_t oldlen = len;
void **items = list->items;
size_t j = 0;
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)) {
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, v);
arraylist_push(copied_list, f);
}
if (move || __unlikely(!v)) {
if (i < len - 2) {
items[i] = items[len - 2];
items[i + 1] = items[len - 1];
i -= 2;
// remove item
}
else {
if (j < i) {
items[j] = items[i];
items[j+1] = items[i+1];
}
len -= 2;
j += 2;
}
}
len = j;
if (oldlen == len)
return;
if (need_sync) {
Expand Down Expand Up @@ -187,9 +189,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 +205,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 +241,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 +267,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,27 +311,21 @@ 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_ptr_finalizer(jl_ptls_t ptls, jl_value_t *v, void *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)
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));
jl_gc_add_ptr_finalizer(ptls, v, jl_unbox_voidpointer(f));
}
else {
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_DLLEXPORT void jl_finalize_th(jl_ptls_t ptls, jl_value_t *o)
{
JL_LOCK_NOGC(&finalizers_lock);
Expand All @@ -340,16 +334,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 @@ -1867,6 +1859,11 @@ stack: {
}
else {
new_obj = (jl_value_t*)gc_read_stack(&rts[i], offset, lb, ub);
if (gc_ptr_tag(new_obj, 1)) {
// handle tagged pointers in finalizer list
new_obj = gc_ptr_clear_tag(new_obj, 1);
i++;
}
}
if (!gc_try_setmark(new_obj, &nptr, &tag, &bits))
continue;
Expand Down Expand Up @@ -2293,42 +2290,32 @@ 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)) {
// 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);
schedule_finalization(v0, fin);
}
if (isold) {
// The caller relies on the new objects to be pushed to the end of
Expand All @@ -2337,7 +2324,7 @@ static void sweep_finalizer_list(arraylist_t *list)
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 d04bf35

Please sign in to comment.