diff --git a/core/arch/mangle_shared.c b/core/arch/mangle_shared.c index 13431e49ec8..991c4f8ec34 100644 --- a/core/arch/mangle_shared.c +++ b/core/arch/mangle_shared.c @@ -188,6 +188,10 @@ prepare_for_clean_call(dcontext_t *dcontext, clean_call_info_t *cci, instrlist_t { uint dstack_offs = 0; + instr_t *start_label = INSTR_CREATE_label(dcontext); + instr_set_note(start_label, (void *)DR_NOTE_CALLOUT_SEQUENCE_START); + PRE(ilist, instr, start_label); + if (cci == NULL) cci = &default_clean_call_info; @@ -427,6 +431,9 @@ cleanup_after_clean_call(dcontext_t *dcontext, clean_call_info_t *cci, instrlist PRE(ilist, instr, instr_create_restore_from_dcontext(dcontext, REG_XSP, XSP_OFFSET)); } + instr_t *end_label = INSTR_CREATE_label(dcontext); + instr_set_note(end_label, (void *)DR_NOTE_CALLOUT_SEQUENCE_END); + PRE(ilist, instr, end_label); } bool diff --git a/core/lib/globals_api.h b/core/lib/globals_api.h index 229d474e8cd..590f9abf447 100644 --- a/core/lib/globals_api.h +++ b/core/lib/globals_api.h @@ -854,6 +854,13 @@ enum { * their application values, as required for DR's internal block mangling. */ DR_NOTE_REG_BARRIER, + /* Used for translation from an instruction list. These apply not only to + * client-inserted clean calls but all inserted calls whether inserted by + * clients or DR and whether fully clean or not. This is thus distinct from + * DR_NOTE_CLEAN_CALL_END. + */ + DR_NOTE_CALLOUT_SEQUENCE_START, + DR_NOTE_CALLOUT_SEQUENCE_END, }; /** diff --git a/core/translate.c b/core/translate.c index 9ce80e7b872..4274f6a2e75 100644 --- a/core/translate.c +++ b/core/translate.c @@ -97,6 +97,8 @@ typedef struct _translate_walk_t { bool in_mangle_region_epilogue; /* What is the translation target of the current mangle region */ app_pc translation; + /* Are we inside a clean call? */ + bool in_clean_call; } translate_walk_t; static void @@ -345,6 +347,12 @@ translate_walk_track_post_instr(dcontext_t *tdcontext, instr_t *inst, reg_id_t reg, r; bool spill, spill_tls; + if (instr_is_label(inst)) { + if (instr_get_note(inst) == (void *)DR_NOTE_CALLOUT_SEQUENCE_START) + walk->in_clean_call = true; + else if (instr_get_note(inst) == (void *)DR_NOTE_CALLOUT_SEQUENCE_END) + walk->in_clean_call = false; + } if (instr_is_our_mangling(inst)) { if (!walk->in_mangle_region) { walk->in_mangle_region = true; @@ -361,12 +369,12 @@ translate_walk_track_post_instr(dcontext_t *tdcontext, instr_t *inst, walk->translation); } else ASSERT(walk->translation == instr_get_translation(inst)); - /* PR 302951: we recognize a clean call by its NULL translation. + /* We recognize a clean call by explicit labels or flags. * We do not track any stack or spills: we assume we will only * fault on an argument that references app memory, in which case * we restore to the priv_mcontext_t on the stack. */ - if (walk->translation == NULL) { + if (walk->in_clean_call) { DOLOG(4, LOG_INTERP, { d_r_loginst(get_thread_private_dcontext(), 4, inst, "\tin clean call arg region"); @@ -703,9 +711,7 @@ translate_walk_restore(dcontext_t *tdcontext, translate_walk_t *walk, instr_t *i static void translate_restore_clean_call(dcontext_t *tdcontext, translate_walk_t *walk) { - /* PR 302951: we recognize a clean call by its combination of - * our-mangling and NULL translation. - * We restore to the priv_mcontext_t that was pushed on the stack. + /* We restore to the priv_mcontext_t that was pushed on the stack. * FIXME i#4219: This is not safe: see comment below. */ LOG(THREAD_GET, LOG_INTERP, 2, "\ttranslating clean call arg crash\n"); @@ -766,7 +772,7 @@ recreate_app_state_from_info(dcontext_t *tdcontext, const translation_info_t *in byte *cpc, *prev_cpc; cache_pc target_cache = mc->pc; uint i; - bool contig = true, ours = false; + bool contig = true, ours = false, in_clean_call = false; recreate_success_t res = (just_pc ? RECREATE_SUCCESS_PC : RECREATE_SUCCESS_STATE); instr_t instr; translate_walk_t walk; @@ -800,6 +806,7 @@ recreate_app_state_from_info(dcontext_t *tdcontext, const translation_info_t *in answer = info->translation[i].app; contig = !TEST(TRANSLATE_IDENTICAL, info->translation[i].flags); ours = TEST(TRANSLATE_OUR_MANGLING, info->translation[i].flags); + in_clean_call = TEST(TRANSLATE_CLEAN_CALL, info->translation[i].flags); i++; } @@ -834,6 +841,8 @@ recreate_app_state_from_info(dcontext_t *tdcontext, const translation_info_t *in instr_set_translation(&instr, answer); translate_walk_track_pre_instr(tdcontext, &instr, &walk); translate_walk_track_post_instr(tdcontext, &instr, &walk); + /* We directly set this field rather than inserting synthetic labels. */ + walk.in_clean_call = in_clean_call; /* advance translation by the stride: either instr length or 0 */ if (contig) @@ -860,7 +869,7 @@ recreate_app_state_from_info(dcontext_t *tdcontext, const translation_info_t *in * xl8 we could be before setup or after teardown of the mcontext on the * dstack, and with leaner clean calls we might not have the full mcontext. */ - if (answer == NULL && ours) + if (answer == NULL && walk.in_clean_call) translate_restore_clean_call(tdcontext, &walk); else res = RECREATE_SUCCESS_PC; /* failed on full state, but pc good */ @@ -1021,7 +1030,7 @@ recreate_app_state_from_ilist(dcontext_t *tdcontext, instrlist_t *ilist, byte *s /* PR 302951: our clean calls do show up here and have full state. * FIXME i#4219: This is not safe: see comment above. */ - if (instr_is_our_mangling(inst)) + if (walk.in_clean_call) translate_restore_clean_call(tdcontext, &walk); else res = RECREATE_SUCCESS_PC; /* failed on full state, but pc good */ @@ -1688,7 +1697,7 @@ translation_info_free(dcontext_t *dcontext, translation_info_t *info) static inline void set_translation(dcontext_t *dcontext, translation_entry_t **entries, uint *num_entries, uint entry, ushort cache_offs, app_pc app, bool identical, - bool our_mangling) + bool our_mangling, bool in_clean_call) { if (entry >= *num_entries) { /* alloc new arrays 2x as big */ @@ -1704,6 +1713,8 @@ set_translation(dcontext_t *dcontext, translation_entry_t **entries, uint *num_e (*entries)[entry].flags |= TRANSLATE_IDENTICAL; if (our_mangling) (*entries)[entry].flags |= TRANSLATE_OUR_MANGLING; + if (in_clean_call) + (*entries)[entry].flags |= TRANSLATE_CLEAN_CALL; LOG(THREAD, LOG_FRAGMENT, 4, "\tset_translation: %d +%5d => " PFX " %s%s\n", entry, cache_offs, app, identical ? "identical" : "contiguous", our_mangling ? " ours" : ""); @@ -1787,7 +1798,7 @@ record_translation_info(dcontext_t *dcontext, fragment_t *f, instrlist_t *existi if (fragment_prefix_size(f->flags) > 0) { ASSERT(f->start_pc < cpc); set_translation(dcontext, &entries, &num_entries, i, 0, f->tag, - true /*identical*/, true /*our mangling*/); + true /*identical*/, true /*our mangling*/, false /*!call*/); last_translation = f->tag; last_contig = false; i++; @@ -1798,13 +1809,19 @@ record_translation_info(dcontext_t *dcontext, fragment_t *f, instrlist_t *existi for (inst = instrlist_first(ilist); inst; inst = instr_get_next(inst)) { app_pc app = instr_get_translation(inst); uint prev_i = i; + bool in_clean_call = false; /* Should only be NULL for meta-code added by a client. * We preserve the NULL so our translation routines know to not * let this be a thread relocation point */ - /* i#739, skip label instr */ - if (instr_is_label(inst)) + if (instr_is_label(inst)) { + if (instr_get_note(inst) == (void *)DR_NOTE_CALLOUT_SEQUENCE_START) + in_clean_call = true; + else if (instr_get_note(inst) == (void *)DR_NOTE_CALLOUT_SEQUENCE_END) + in_clean_call = false; + /* i#739, skip label instr */ continue; + } /* PR 302951: clean call args are instr_is_our_mangling so no assert for that */ ASSERT(app != NULL || instr_is_meta(inst)); /* see whether we need a new entry, or the current stride (contig @@ -1832,7 +1849,7 @@ record_translation_info(dcontext_t *dcontext, fragment_t *f, instrlist_t *existi } else { set_translation(dcontext, &entries, &num_entries, i, (ushort)(cpc - f->start_pc), app, true /*identical*/, - instr_is_our_mangling(inst)); + instr_is_our_mangling(inst), in_clean_call); i++; } last_contig = false; @@ -1843,7 +1860,7 @@ record_translation_info(dcontext_t *dcontext, fragment_t *f, instrlist_t *existi */ set_translation(dcontext, &entries, &num_entries, i, (ushort)(cpc - f->start_pc), app, false /*contig*/, - instr_is_our_mangling(inst)); + instr_is_our_mangling(inst), in_clean_call); last_contig = true; i++; } /* else, contig continues */ @@ -1866,7 +1883,7 @@ record_translation_info(dcontext_t *dcontext, fragment_t *f, instrlist_t *existi /* probably a follow-ubr, so create a new contig entry */ set_translation(dcontext, &entries, &num_entries, i, (ushort)(cpc - f->start_pc), app, false /*contig*/, - instr_is_our_mangling(inst)); + instr_is_our_mangling(inst), in_clean_call); last_contig = true; i++; } @@ -1874,15 +1891,17 @@ record_translation_info(dcontext_t *dcontext, fragment_t *f, instrlist_t *existi } last_translation = app; - /* We need to make a new entry if the our-mangling flag changed */ + /* We need to make a new entry if the flags changed. */ if (i > 0 && i == prev_i && - instr_is_our_mangling(inst) != - TEST(TRANSLATE_OUR_MANGLING, entries[i - 1].flags)) { + (!BOOLS_MATCH(instr_is_our_mangling(inst), + TEST(TRANSLATE_OUR_MANGLING, entries[i - 1].flags)) || + !BOOLS_MATCH(in_clean_call, + TEST(TRANSLATE_CLEAN_CALL, entries[i - 1].flags)))) { /* our manglings are usually identical */ bool identical = instr_is_our_mangling(inst); set_translation(dcontext, &entries, &num_entries, i, (ushort)(cpc - f->start_pc), app, identical, - instr_is_our_mangling(inst)); + instr_is_our_mangling(inst), in_clean_call); last_contig = !identical; i++; } diff --git a/core/translate.h b/core/translate.h index 4091d0440ca..5feacfb3cd1 100644 --- a/core/translate.h +++ b/core/translate.h @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2011-2021 Google, Inc. All rights reserved. + * Copyright (c) 2011-2022 Google, Inc. All rights reserved. * Copyright (c) 2000-2010 VMware, Inc. All rights reserved. * **********************************************************/ @@ -53,6 +53,7 @@ enum { */ TRANSLATE_IDENTICAL = 0x0001, /* otherwise contiguous */ TRANSLATE_OUR_MANGLING = 0x0002, /* added by our own mangling (PR 267260) */ + TRANSLATE_CLEAN_CALL = 0x0004, /* Added by our own mangling. */ }; /* no typedef b/c we need ushort not int */ /* Translation table entry (case 3559). diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt index a723df6c405..2c38ffb2322 100644 --- a/suite/tests/CMakeLists.txt +++ b/suite/tests/CMakeLists.txt @@ -2949,7 +2949,7 @@ link_with_pthread(api.startstop) tobuild_api(api.detach api/detach.c "" "" OFF OFF OFF) link_with_pthread(api.detach) if (LINUX AND X64) # Will take extra work to port to 32-bit. - tobuild_api(api.detach_state api/detach_state.c "" "" OFF OFF OFF) + tobuild_api(api.detach_state api/detach_state.c "" "" OFF ON OFF) target_include_directories(api.detach_state PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/api) link_with_pthread(api.detach_state) diff --git a/suite/tests/api/detach_state.c b/suite/tests/api/detach_state.c index 2badd8c38e4..74a985488c2 100644 --- a/suite/tests/api/detach_state.c +++ b/suite/tests/api/detach_state.c @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2018-2021 Google, Inc. All rights reserved. + * Copyright (c) 2018-2022 Google, Inc. All rights reserved. * **********************************************************/ /* @@ -382,7 +382,7 @@ check_xsp(ptr_uint_t *xsp) # endif void -make_writable(ptr_uint_t pc) +make_mem_writable(ptr_uint_t pc) { protect_mem((void *)pc, 1024, ALLOW_READ | ALLOW_WRITE | ALLOW_EXEC); } @@ -410,6 +410,109 @@ test_thread_func(void (*asm_func)(void)) sideline_ready_for_detach = false; } +/*************************************************************************** + * Client code + */ + +static void *load_from; + +static dr_emit_flags_t +event_bb(void *drcontext, void *tag, instrlist_t *bb, bool for_trace, bool translating) +{ +# ifdef X86 + /* Test i#5786 where a tool-inserted pc-relative load that doesn't reach is + * mangled by DR and the resulting DR-mangling with no translation triggers + * DR's (fragile) clean call identication and incorrectly tries to restore + * state from (garbage) on the dstack. + */ + instr_t *first = instrlist_first(bb); + dr_save_reg(drcontext, bb, first, DR_REG_XAX, SPILL_SLOT_1); + instrlist_meta_preinsert(bb, first, + XINST_CREATE_load(drcontext, opnd_create_reg(DR_REG_XAX), + OPND_CREATE_ABSMEM(load_from, OPSZ_PTR))); + dr_restore_reg(drcontext, bb, first, DR_REG_XAX, SPILL_SLOT_1); + /* XXX i#4232: If we do not give translations, detach can fail as there are so + * many no-translation instrs here a thread can end up never landing on a + * full-state-translatable PC. Thus, we set the translation, and provide a + * restore-state event. i#4232 covers are more automated/convenient way to solve + * this. + */ + for (instr_t *inst = instrlist_first(bb); inst != first; inst = instr_get_next(inst)) + instr_set_translation(inst, instr_get_app_pc(first)); +# endif + /* Test both by alternating and assuming we hit both at the detach points + * in the various tests. + */ + if ((ptr_uint_t)tag % 2 == 0) + return DR_EMIT_DEFAULT; + else + return DR_EMIT_STORE_TRANSLATIONS; +} + +static bool +event_restore(void *drcontext, bool restore_memory, dr_restore_state_info_t *info) +{ + /* Because we set the translation (to avoid detach timeouts) we need to restore + * our register too. + */ + if (info->fragment_info.cache_start_pc == NULL || + /* At the first instr should require no translation. */ + info->raw_mcontext->pc <= info->fragment_info.cache_start_pc) + return true; + instr_t inst; + instr_init(drcontext, &inst); + byte *pc = info->fragment_info.cache_start_pc; + while (pc < info->raw_mcontext->pc) { + instr_reset(drcontext, &inst); + pc = decode(drcontext, pc, &inst); + bool tls; + uint offs; + bool spill; + reg_id_t reg; + if (instr_is_reg_spill_or_restore(drcontext, &inst, &tls, &spill, ®, &offs) && + tls && !spill && reg == DR_REG_XAX) { + /* The target point is past our restore. */ + instr_free(drcontext, &inst); + return true; + } + } + instr_free(drcontext, &inst); + reg_set_value(DR_REG_XAX, info->mcontext, dr_read_saved_reg(drcontext, SPILL_SLOT_1)); + return true; +} + +static void +event_exit(void) +{ + dr_custom_free(NULL, 0, load_from, dr_page_size()); + dr_unregister_bb_event(event_bb); + dr_unregister_restore_state_ex_event(event_restore); + dr_unregister_exit_event(event_exit); +} + +DR_EXPORT void +dr_client_main(client_id_t id, int argc, const char *argv[]) +{ + static bool do_once; + if (!do_once) { + print("in dr_client_main\n"); + do_once = true; + } + dr_register_bb_event(event_bb); + dr_register_restore_state_ex_event(event_restore); + dr_register_exit_event(event_exit); + + /* Try to get an address that is not 32-bit-displacement reachable from + * the cache. + */ + load_from = dr_custom_alloc(NULL, 0, dr_page_size(), + DR_MEMPROT_READ | DR_MEMPROT_WRITE, NULL); +} + +/*************************************************************************** + * Top-level + */ + int main(void) { @@ -715,7 +818,7 @@ START_FILE DECL_EXTERN(sideline_exit) DECL_EXTERN(sideline_ready_for_detach) -DECL_EXTERN(make_writable) +DECL_EXTERN(make_mem_writable) DECL_EXTERN(check_gpr_vals) DECL_EXTERN(safe_stack) @@ -974,7 +1077,7 @@ GLOBAL_LABEL(FUNCNAME:) call check_gprs_from_DR_retaddr check_gprs_from_DR_retaddr: pop REG_XAX - CALLC1(GLOBAL_REF(make_writable), REG_XAX) + CALLC1(GLOBAL_REF(make_mem_writable), REG_XAX) /* Put in unique values. */ CALLC0(unique_values_to_registers) /* Signal that we are ready for a detach. */ @@ -1039,7 +1142,7 @@ GLOBAL_LABEL(FUNCNAME:) call check_eflags_from_DR_retaddr check_eflags_from_DR_retaddr: pop REG_XAX - CALLC1(GLOBAL_REF(make_writable), REG_XAX) + CALLC1(GLOBAL_REF(make_mem_writable), REG_XAX) /* Put in a unique value. */ mov REG_XAX, MAKE_HEX_ASM(XFLAGS_BASE()) push REG_XAX @@ -1112,7 +1215,7 @@ GLOBAL_LABEL(FUNCNAME:) call check_xsp_from_DR_retaddr check_xsp_from_DR_retaddr: pop REG_XAX - CALLC1(GLOBAL_REF(make_writable), REG_XAX) + CALLC1(GLOBAL_REF(make_mem_writable), REG_XAX) /* Put in a unique value. */ mov REG_XCX, REG_XSP mov REG_XSP, PTRSZ SYMREF(safe_stack) diff --git a/suite/tests/api/detach_state.expect b/suite/tests/api/detach_state.expect index de639143055..8ffbdf6b657 100644 --- a/suite/tests/api/detach_state.expect +++ b/suite/tests/api/detach_state.expect @@ -1 +1,2 @@ +in dr_client_main all done