Skip to content

Commit

Permalink
i#2666 dyn cxt: Fix early context free (#4722)
Browse files Browse the repository at this point in the history 
Fixes a bug in PR #4703 where for post-CreateUserProcess handling it
frees the heap buffer for a CONTEXT before the code is finished using
it.

Also updates the CONTEXT heap buffers from PR #4703 to use
thread-private heap where possible, to avoid locks.

Issue: #2666
  • Loading branch information
@derekbruening
derekbruening authored Feb 6, 2021
1 parent 9be64f0 commit c05ee23
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 23 deletions.
16 changes: 8 additions & 8 deletions core/win32/os.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2503,7 +2503,7 @@ os_take_over_thread(dcontext_t *dcontext, HANDLE hthread, thread_id_t tid, bool
bool success = true;
DWORD cxt_flags = CONTEXT_DR_STATE;
size_t bufsz = nt_get_context_size(cxt_flags);
char *buf = (char *)global_heap_alloc(bufsz HEAPACCT(ACCT_THREAD_MGT));
char *buf = (char *)heap_alloc(dcontext, bufsz HEAPACCT(ACCT_THREAD_MGT));
CONTEXT *cxt = nt_initialize_context(buf, bufsz, cxt_flags);
ASSERT(tid == thread_id_from_handle(hthread));
if ((suspended || nt_thread_suspend(hthread, NULL)) &&
Expand Down Expand Up @@ -2531,7 +2531,7 @@ os_take_over_thread(dcontext_t *dcontext, HANDLE hthread, thread_id_t tid, bool
if (is_in_dynamo_dll((app_pc)cxt->CXT_XIP) ||
new_thread_is_waiting_for_dr_init(tid, (app_pc)cxt->CXT_XIP)) {
LOG(GLOBAL, LOG_THREADS, 1, "\tthread " TIDFMT " is already waiting\n", tid);
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
return true; /* it's waiting for us to take it over */
}
/* Avoid double-takeover.
Expand Down Expand Up @@ -2568,7 +2568,7 @@ os_take_over_thread(dcontext_t *dcontext, HANDLE hthread, thread_id_t tid, bool
*/
ASSERT_CURIOSITY(false && "thread takeover context reverted!");
} else {
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
return true;
}
} else {
Expand Down Expand Up @@ -2614,7 +2614,7 @@ os_take_over_thread(dcontext_t *dcontext, HANDLE hthread, thread_id_t tid, bool
LOG(GLOBAL, LOG_THREADS, 1, "\tfailed to suspend/query thread " TIDFMT "\n", tid);
success = false;
}
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
return success;
}

Expand Down Expand Up @@ -5159,14 +5159,14 @@ thread_get_mcontext(thread_record_t *tr, priv_mcontext_t *mc)
{
DWORD cxt_flags = CONTEXT_DR_STATE;
size_t bufsz = nt_get_context_size(cxt_flags);
char *buf = (char *)global_heap_alloc(bufsz HEAPACCT(ACCT_THREAD_MGT));
char *buf = (char *)heap_alloc(tr->dcontext, bufsz HEAPACCT(ACCT_THREAD_MGT));
CONTEXT *cxt = nt_initialize_context(buf, bufsz, cxt_flags);
bool res = false;
if (thread_get_context(tr, cxt)) {
context_to_mcontext(mc, cxt);
res = true;
}
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(tr->dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
return res;
}

Expand All @@ -5175,15 +5175,15 @@ thread_set_mcontext(thread_record_t *tr, priv_mcontext_t *mc)
{
DWORD cxt_flags = CONTEXT_DR_STATE;
size_t bufsz = nt_get_context_size(cxt_flags);
char *buf = (char *)global_heap_alloc(bufsz HEAPACCT(ACCT_THREAD_MGT));
char *buf = (char *)heap_alloc(tr->dcontext, bufsz HEAPACCT(ACCT_THREAD_MGT));
CONTEXT *cxt = nt_initialize_context(buf, bufsz, cxt_flags);
/* i#1033: get the context from the dst thread to make sure
* segments are correctly set.
*/
thread_get_context(tr, cxt);
mcontext_to_context(cxt, mc, false /* !set_cur_seg */);
bool res = thread_set_context(tr, cxt);
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(tr->dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
return res;
}

Expand Down
31 changes: 16 additions & 15 deletions core/win32/syscall.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1722,7 +1722,8 @@ add_dr_env_vars(dcontext_t *dcontext, HANDLE phandle, uint64 env_ptr, bool peb_i
* first thread). Retrieves context from thread handle.
*/
static bool
not_first_thread_in_new_process(HANDLE process_handle, HANDLE thread_handle)
not_first_thread_in_new_process(dcontext_t *dcontext, HANDLE process_handle,
HANDLE thread_handle)
{
#ifndef X64
bool peb_is_32 = is_32bit_process(process_handle);
Expand All @@ -1739,12 +1740,12 @@ not_first_thread_in_new_process(HANDLE process_handle, HANDLE thread_handle)
#endif
DWORD cxt_flags = CONTEXT_DR_STATE;
size_t bufsz = nt_get_context_size(cxt_flags);
char *buf = (char *)global_heap_alloc(bufsz HEAPACCT(ACCT_THREAD_MGT));
char *buf = (char *)heap_alloc(dcontext, bufsz HEAPACCT(ACCT_THREAD_MGT));
CONTEXT *cxt = nt_initialize_context(buf, bufsz, cxt_flags);
bool res = false;
if (NT_SUCCESS(nt_get_context(thread_handle, cxt)))
res = !is_first_thread_in_new_process(process_handle, cxt);
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
return res;
}

Expand Down Expand Up @@ -1853,7 +1854,7 @@ presys_ResumeThread(dcontext_t *dcontext, reg_t *param_base)
"Not injecting so not setting DR env vars in pid=" PIFX "\n", pid);
return;
}
if (not_first_thread_in_new_process(process_handle, thread_handle)) {
if (not_first_thread_in_new_process(dcontext, process_handle, thread_handle)) {
LOG(THREAD, LOG_SYSCALLS, 1,
"Not first thread so not setting DR env vars in pid=" PIFX "\n", pid);
return;
Expand Down Expand Up @@ -2091,7 +2092,7 @@ presys_SetContextThread(dcontext_t *dcontext, reg_t *param_base)
*/
DWORD cxt_flags = CONTEXT_DR_STATE;
size_t bufsz = nt_get_context_size(cxt_flags);
char *buf = (char *)global_heap_alloc(bufsz HEAPACCT(ACCT_THREAD_MGT));
char *buf = (char *)heap_alloc(dcontext, bufsz HEAPACCT(ACCT_THREAD_MGT));
CONTEXT *alt_cxt = nt_initialize_context(buf, bufsz, cxt_flags);
STATS_INC(num_app_setcontext_no_control);
if (thread_get_context(tr, alt_cxt) &&
Expand Down Expand Up @@ -2119,7 +2120,7 @@ presys_SetContextThread(dcontext_t *dcontext, reg_t *param_base)
intercept = false;
ASSERT_NOT_REACHED();
}
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
}
if (intercept) {
/* modify the being-set cxt so that we retain control */
Expand Down Expand Up @@ -3269,7 +3270,7 @@ postsys_CreateUserProcess(dcontext_t *dcontext, reg_t *param_base, bool success)
}
DWORD cxt_flags = CONTEXT_DR_STATE;
size_t bufsz = nt_get_context_size(cxt_flags);
char *buf = (char *)global_heap_alloc(bufsz HEAPACCT(ACCT_THREAD_MGT));
char *buf = (char *)heap_alloc(dcontext, bufsz HEAPACCT(ACCT_THREAD_MGT));
CONTEXT *context;
CONTEXT *cxt = NULL;
int res;
Expand Down Expand Up @@ -3310,10 +3311,9 @@ postsys_CreateUserProcess(dcontext_t *dcontext, reg_t *param_base, bool success)
ASSERT(cxt != NULL || DYNAMO_OPTION(early_inject)); /* Else, exited above. */
/* Do the actual injection. */
if (!maybe_inject_into_process(dcontext, proc_handle, thread_handle, cxt)) {
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
return;
}
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
propagate_options_via_env_vars(dcontext, proc_handle, thread_handle);
if (cxt != NULL) {
/* injection routine is assuming doesn't have to install cxt */
Expand All @@ -3328,6 +3328,7 @@ postsys_CreateUserProcess(dcontext_t *dcontext, reg_t *param_base, bool success)
"Failed to set context of child thread");
}
}
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
}

/* NtGetContextThread */
Expand All @@ -3350,7 +3351,7 @@ postsys_GetContextThread(dcontext_t *dcontext, reg_t *param_base, bool success)

DWORD cxt_flags = CONTEXT_DR_STATE;
size_t bufsz = nt_get_context_size(cxt_flags);
char *buf = (char *)global_heap_alloc(bufsz HEAPACCT(ACCT_THREAD_MGT));
char *buf = (char *)heap_alloc(dcontext, bufsz HEAPACCT(ACCT_THREAD_MGT));

/* FIXME : we are going to read/write the context argument which is
* potentially unsafe, since success it must have been readable when
Expand Down Expand Up @@ -3413,7 +3414,7 @@ postsys_GetContextThread(dcontext_t *dcontext, reg_t *param_base, bool success)
* we don't translate other than the pc anyway.
*/
d_r_mutex_unlock(&thread_initexit_lock);
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
return;
}
xlate_cxt = alt_cxt;
Expand Down Expand Up @@ -3483,7 +3484,7 @@ postsys_GetContextThread(dcontext_t *dcontext, reg_t *param_base, bool success)
SELF_PROTECT_LOCAL(trec->dcontext, READONLY);
}
d_r_mutex_unlock(&thread_initexit_lock);
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
}

/* NtSuspendThread */
Expand Down Expand Up @@ -3523,7 +3524,7 @@ postsys_SuspendThread(dcontext_t *dcontext, reg_t *param_base, bool success)
if (!mutex_testlock(&all_threads_lock)) {
DWORD cxt_flags = CONTEXT_DR_STATE;
size_t bufsz = nt_get_context_size(cxt_flags);
char *buf = (char *)global_heap_alloc(bufsz HEAPACCT(ACCT_THREAD_MGT));
char *buf = (char *)heap_alloc(dcontext, bufsz HEAPACCT(ACCT_THREAD_MGT));
CONTEXT *cxt = nt_initialize_context(buf, bufsz, cxt_flags);
thread_record_t *tr;
/* know thread isn't holding any of the locks we will need */
Expand All @@ -3547,12 +3548,12 @@ postsys_SuspendThread(dcontext_t *dcontext, reg_t *param_base, bool success)
LOG(THREAD, LOG_SYNCH, 2,
"SuspendThread suspended thread " TIDFMT " at good place\n", tid);
SELF_PROTECT_LOCAL(tr->dcontext, READONLY);
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
return;
}
SELF_PROTECT_LOCAL(tr->dcontext, READONLY);
}
global_heap_free(buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
heap_free(dcontext, buf, bufsz HEAPACCT(ACCT_THREAD_MGT));
} else {
LOG(THREAD, LOG_SYNCH, 2,
"SuspendThread couldn't get all_threads_lock to test if thread " TIDFMT
Expand Down

0 comments on commit c05ee23

Please sign in to comment.