From 74c35679a3fba707ad695c53c28918aca30b95b5 Mon Sep 17 00:00:00 2001 From: Stefan Seifert Date: Sun, 17 Oct 2021 14:05:59 +0200 Subject: [PATCH 1/2] Fix segfault when thread deopts while another tries to find a dynamic Commit e94893f897495f3de16d4898279c87e356058acb fixed a possible segfault in the frame walkercaused by a concurrent deopt in a different thread. The fix was to use a local copy of the frame's spesh_cand pointer. However MVM_spesh_deopt_find_inactive_frame_deopt_idx which is called by the same code also accesses the frame's spesh_cand. So this function could still end up dereferencing a NULL pointer if the frame's spesh_cand was cleared in the mean time. Fix by adding a spesh_cand argument to that function, so we can use our local copy consistently. --- src/6model/reprs/MVMContext.c | 2 +- src/disp/resume.c | 2 +- src/spesh/deopt.c | 25 ++++++++++++++----------- src/spesh/deopt.h | 3 ++- src/spesh/frame_walker.c | 2 +- 5 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/6model/reprs/MVMContext.c b/src/6model/reprs/MVMContext.c index fde7fa0484..3997f4c425 100644 --- a/src/6model/reprs/MVMContext.c +++ b/src/6model/reprs/MVMContext.c @@ -286,7 +286,7 @@ static void snapshot_frame_callees(MVMThreadContext *tc, MVMFrame *f) { if (extra->caller_deopt_idx) return; /* Store with + 1 to avoid semi-predicate problem. */ - extra->caller_deopt_idx = MVM_spesh_deopt_find_inactive_frame_deopt_idx(tc, f->caller) + 1; + extra->caller_deopt_idx = 1 + MVM_spesh_deopt_find_inactive_frame_deopt_idx(tc, f->caller, cand); } } f = f->caller; diff --git a/src/disp/resume.c b/src/disp/resume.c index e7c96fc7c9..bb83b7287a 100644 --- a/src/disp/resume.c +++ b/src/disp/resume.c @@ -145,7 +145,7 @@ static MVMuint32 find_internal(MVMThreadContext *tc, MVMDispResumptionData *data * it is the one in the most nested inline, if there are * any inlines. */ MVMint32 deopt_idx = MVM_spesh_deopt_find_inactive_frame_deopt_idx(tc, - frame); + frame, cand); if (deopt_idx == -1) MVM_oops(tc, "Failed to find deopt index when processing resume"); MVMuint32 i; diff --git a/src/spesh/deopt.c b/src/spesh/deopt.c index d499f8ad41..d8d0ee90ec 100644 --- a/src/spesh/deopt.c +++ b/src/spesh/deopt.c @@ -340,10 +340,12 @@ void MVM_spesh_deopt_all(MVMThreadContext *tc) { /* Takes a frame that we're lazily deoptimizing and finds the currently * active deopt index at the point of the call it was making. Returns -1 if * none can be resolved. */ -MVMint32 MVM_spesh_deopt_find_inactive_frame_deopt_idx(MVMThreadContext *tc, MVMFrame *f) { +MVMint32 MVM_spesh_deopt_find_inactive_frame_deopt_idx(MVMThreadContext *tc, + MVMFrame *f, MVMSpeshCandidate *spesh_cand) +{ /* Is it JITted code? */ - if (f->spesh_cand->body.jitcode) { - MVMJitCode *jitcode = f->spesh_cand->body.jitcode; + if (spesh_cand->body.jitcode) { + MVMJitCode *jitcode = spesh_cand->body.jitcode; MVMuint32 idx = MVM_jit_code_get_active_deopt_idx(tc, jitcode, f); if (idx < jitcode->num_deopts) { MVMint32 deopt_idx = jitcode->deopts[idx].idx; @@ -355,11 +357,11 @@ MVMint32 MVM_spesh_deopt_find_inactive_frame_deopt_idx(MVMThreadContext *tc, MVM } else { /* Not JITted; see if we can find the return address in the deopt table. */ - MVMuint32 ret_offset = (f == tc->cur_frame ? *(tc->interp_cur_op) : f->return_address) - f->spesh_cand->body.bytecode; - MVMint32 n = f->spesh_cand->body.num_deopts * 2; + MVMuint32 ret_offset = (f == tc->cur_frame ? *(tc->interp_cur_op) : f->return_address) - spesh_cand->body.bytecode; + MVMint32 n = spesh_cand->body.num_deopts * 2; MVMint32 i; for (i = 0; i < n; i += 2) { - if (MVM_spesh_deopt_bytecode_pos(f->spesh_cand->body.deopts[i + 1]) == ret_offset) { + if (MVM_spesh_deopt_bytecode_pos(spesh_cand->body.deopts[i + 1]) == ret_offset) { MVMint32 deopt_idx = i / 2; #if MVM_LOG_DEOPTS fprintf(stderr, " Found deopt index for interpeter (idx %d)\n", deopt_idx); @@ -383,6 +385,7 @@ void MVM_spesh_deopt_during_unwind(MVMThreadContext *tc) { * stack top one. */ MVMCallStackRecord *record = tc->stack_top; MVMFrame *frame = MVM_callstack_record_to_frame(record); + MVMSpeshCandidate *spesh_cand = frame->spesh_cand; #if MVM_LOG_DEOPTS fprintf(stderr, "Lazy deopt on unwind of frame '%s' (cuid '%s')\n", MVM_string_utf8_encode_C_string(tc, frame->static_info->body.name), @@ -390,10 +393,10 @@ void MVM_spesh_deopt_during_unwind(MVMThreadContext *tc) { #endif /* Find the deopt index, and assuming it's found, deopt. */ - MVMint32 deopt_idx = MVM_spesh_deopt_find_inactive_frame_deopt_idx(tc, frame); + MVMint32 deopt_idx = MVM_spesh_deopt_find_inactive_frame_deopt_idx(tc, frame, spesh_cand); if (deopt_idx >= 0) { - MVMuint32 deopt_target = frame->spesh_cand->body.deopts[deopt_idx * 2]; - MVMuint32 deopt_offset = MVM_spesh_deopt_bytecode_pos(frame->spesh_cand->body.deopts[deopt_idx * 2 + 1]); + MVMuint32 deopt_target = spesh_cand->body.deopts[deopt_idx * 2]; + MVMuint32 deopt_offset = MVM_spesh_deopt_bytecode_pos(spesh_cand->body.deopts[deopt_idx * 2 + 1]); MVMFrame *top_frame; MVMROOT(tc, frame, { @@ -402,8 +405,8 @@ void MVM_spesh_deopt_during_unwind(MVMThreadContext *tc) { /* Potentially need to uninline. This leaves the top frame being the * one we're returning into. Otherwise, the top frame is the current * one. */ - if (frame->spesh_cand->body.inlines) { - uninline(tc, frame, frame->spesh_cand, deopt_offset, 1, 0); + if (spesh_cand->body.inlines) { + uninline(tc, frame, spesh_cand, deopt_offset, 1, 0); top_frame = MVM_callstack_current_frame(tc); } else { diff --git a/src/spesh/deopt.h b/src/spesh/deopt.h index 962ad60683..af3a681a0d 100644 --- a/src/spesh/deopt.h +++ b/src/spesh/deopt.h @@ -1,6 +1,7 @@ void MVM_spesh_deopt_all(MVMThreadContext *tc); void MVM_spesh_deopt_one(MVMThreadContext *tc, MVMuint32 deopt_idx); -MVMint32 MVM_spesh_deopt_find_inactive_frame_deopt_idx(MVMThreadContext *tc, MVMFrame *f); +MVMint32 MVM_spesh_deopt_find_inactive_frame_deopt_idx(MVMThreadContext *tc, + MVMFrame *frame, MVMSpeshCandidate *spesh_cand); void MVM_spesh_deopt_during_unwind(MVMThreadContext *tc); MVM_STATIC_INLINE MVMuint32 MVM_spesh_deopt_bytecode_pos(MVMuint32 deopt) { return deopt >> 1; diff --git a/src/spesh/frame_walker.c b/src/spesh/frame_walker.c index 874482c606..f4a1b2f9e5 100644 --- a/src/spesh/frame_walker.c +++ b/src/spesh/frame_walker.c @@ -96,7 +96,7 @@ static void go_to_first_inline(MVMThreadContext *tc, MVMSpeshFrameWalker *fw, MV else { MVMint32 deopt_idx = prev && prev->extra && prev->extra->caller_deopt_idx > 0 ? prev->extra->caller_deopt_idx - 1 - : MVM_spesh_deopt_find_inactive_frame_deopt_idx(tc, f); + : MVM_spesh_deopt_find_inactive_frame_deopt_idx(tc, f, spesh_cand); if (deopt_idx >= 0) { fw->deopt_offset = MVM_spesh_deopt_bytecode_pos(spesh_cand->body.deopts[2 * deopt_idx + 1]); fw->inline_idx = -1; From 80f8edf9477f4c395e22c80bdc0b161895bdaca7 Mon Sep 17 00:00:00 2001 From: Stefan Seifert Date: Sun, 17 Oct 2021 14:28:14 +0200 Subject: [PATCH 2/2] Fix segfault in frame walker when other thread clears the frame's extra Similar to another thread clearing a frame's spesh_cand leading to issues fixed in commit 74c35679a3fba707ad695c53c28918aca30b95b5, it's also possible that another thread clears a frame's extra, right after we checked it for non-NULL and before we try to dereference it. Fix by taking a local copy of the pointer before we check and dereference it. --- src/spesh/frame_walker.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/spesh/frame_walker.c b/src/spesh/frame_walker.c index f4a1b2f9e5..6648b4066f 100644 --- a/src/spesh/frame_walker.c +++ b/src/spesh/frame_walker.c @@ -72,12 +72,15 @@ static void go_to_next_inline(MVMThreadContext *tc, MVMSpeshFrameWalker *fw) { * If so, go to the innermost inline. */ static void go_to_first_inline(MVMThreadContext *tc, MVMSpeshFrameWalker *fw, MVMFrame *prev) { MVMFrame *f = fw->cur_caller_frame; + /* Get a local copy of spesh_cand so we don't stumble over another thread + * clearing the pointer after we checked it */ MVMSpeshCandidate *spesh_cand = f->spesh_cand; if (spesh_cand && spesh_cand->body.inlines) { MVMJitCode *jitcode = spesh_cand->body.jitcode; if (jitcode && f->jit_entry_label) { - void *current_position = prev && prev->extra && prev->extra->caller_jit_position - ? prev->extra->caller_jit_position + MVMFrameExtra *extra; /* Get a local copy as well for the same reason */ + void *current_position = prev && (extra = prev->extra) && extra->caller_jit_position + ? extra->caller_jit_position : MVM_jit_code_get_current_position(tc, jitcode, f); MVMuint32 idx = MVM_jit_code_get_active_inlines(tc, jitcode, current_position, 0); if (idx < jitcode->num_inlines) { @@ -94,8 +97,9 @@ static void go_to_first_inline(MVMThreadContext *tc, MVMSpeshFrameWalker *fw, MV return; } else { - MVMint32 deopt_idx = prev && prev->extra && prev->extra->caller_deopt_idx > 0 - ? prev->extra->caller_deopt_idx - 1 + MVMFrameExtra *extra; /* Get a local copy as well for the same reason */ + MVMint32 deopt_idx = prev && (extra = prev->extra) && extra->caller_deopt_idx > 0 + ? extra->caller_deopt_idx - 1 : MVM_spesh_deopt_find_inactive_frame_deopt_idx(tc, f, spesh_cand); if (deopt_idx >= 0) { fw->deopt_offset = MVM_spesh_deopt_bytecode_pos(spesh_cand->body.deopts[2 * deopt_idx + 1]);