Skip to content

Commit

Permalink
i#5786: Add precise clean call mangling identification
Browse files Browse the repository at this point in the history 
Adds new labels delimiting clean call sequences.
Converts into a translation record flag when storing translations.

Uses the new labels and flag to precisely identify clean call
mangling, replacing the previous scheme which incorrectly thought
mangled tool pc-relative was a clean call, resulting in incorrect
translations and crashes.

Adds a test case to api.detach_state by adding a client (by converting
it to use static DR) which inserts a pc-relative load.  This
reproduces the crash on detach, and is fixed with this fix.
The added instrumentation caused periodic detach failures which were
solved by setting the translation and adding a restore-state event:
i#4232 covers trying to improve the situation.

Issue: #5786, #4232
Fixes #5786
  • Loading branch information
@derekbruening
derekbruening committed Dec 15, 2022
1 parent 614a3c9 commit 9ac3485
Show file tree
Hide file tree
Showing 7 changed files with 165 additions and 27 deletions.
7 changes: 7 additions & 0 deletions core/arch/mangle_shared.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions core/lib/globals_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

/**
Expand Down
57 changes: 38 additions & 19 deletions core/translate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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++;
}

Expand Down Expand Up @@ -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)
Expand All @@ -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 */
Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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 */
Expand All @@ -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" : "");
Expand Down Expand Up @@ -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++;
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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 */
Expand All @@ -1866,23 +1883,25 @@ 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++;
}
}
}
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++;
}
Expand Down
3 changes: 2 additions & 1 deletion core/translate.h
View file
Original file line number Diff line number Diff line change
@@ -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.
* **********************************************************/

Expand Down Expand Up @@ -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).
Expand Down
2 changes: 1 addition & 1 deletion suite/tests/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 9ac3485

Please sign in to comment.