Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement attach for both windows and linux #3328

Closed
wants to merge 2 commits into from
Closed

Implement attach for both windows and linux #3328

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2141465
Implement attach for both windows and linux
summershrimp Dec 25, 2018
c36cac9
Fix formatting.
summershrimp Dec 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,9 +278,9 @@ endif (WIN32)

if (WIN32)
if (DEBUG)
set(WIN32_C_LIB libcmtd)
set(WIN32_C_LIB libcmtd Psapi)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This psapi dependence should not be added here since this is linked into the core DR library. We do not want the core DR library to have extra dependences: it will fail to function with early injection and will hit transparency issues in general. Please see the transparency slides in the DR tutorial and http://dynamorio.org/docs/transparency.html#sec_trans_resource. Please only link this with non-core targets like drinjectlib.

else (DEBUG)
set(WIN32_C_LIB libcmt)
set(WIN32_C_LIB libcmt Psapi)
endif (DEBUG)

set(NOLIBC_DLL_ENTRY /entry:DllMain)
Expand Down
40 changes: 40 additions & 0 deletions core/lib/dr_inject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,22 @@ DR_EXPORT
int
dr_inject_process_create(const char *app_name, const char **app_cmdline, void **data);

#ifdef WINDOWS
DR_EXPORT
/**
* Attach to a existing process.
summershrimp marked this conversation as resolved.
Show resolved Hide resolved
*
* \param[in] pid PID for process to attach.
*
* \param[out] data An opaque pointer that should be passed to
* subsequent dr_inject_* routines to refer to
* this process.
* \return Returns 0 on success. On failure, returns a system error code.`
*/
int
dr_inject_process_attach(process_id_t pid, void **data);
#endif

#ifdef UNIX

DR_EXPORT
Expand Down Expand Up @@ -138,6 +154,30 @@ DR_EXPORT
int
dr_inject_prepare_to_exec(const char *app_name, const char **app_cmdline, void **data);

DR_EXPORT
/**
* Prepare to ptrace(ATTACH) the provided process. Use
* dr_inject_process_inject() to perform the ptrace(ATTACH) under DR.
summershrimp marked this conversation as resolved.
Show resolved Hide resolved
*
* \note Only available on Linux.
*
* \param[in] pid The pid for the target executable. The caller
* must ensure this data is valid until the
* inject data is disposed.
summershrimp marked this conversation as resolved.
Show resolved Hide resolved
*
* \param[in] app_name The path to the target executable. The caller
* must ensure this data is valid until the
* inject data is disposed.
*
* \param[out] data An opaque pointer that should be passed to
* subsequent dr_inject_* routines to refer to
* this process.
* \return Always return true because we just prepare dr_inject_info_t for
* future attach.
summershrimp marked this conversation as resolved.
Show resolved Hide resolved
*/
int
dr_inject_prepare_to_attach(process_id_t pid, const char *app_name, void **data);

DR_EXPORT
/**
* Use the ptrace system call to inject into the targetted process. Must be
Expand Down
34 changes: 34 additions & 0 deletions core/unix/injector.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,20 @@ dr_inject_prepare_to_exec(const char *exe, const char **argv, void **data OUT)
return errcode;
}

DR_EXPORT
int
dr_inject_prepare_to_attach(process_id_t pid, const char *appname, void **data OUT)
{
dr_inject_info_t *info = create_inject_info(appname, NULL);
int errcode = 0;
*data = info;
info->pid = pid;
info->pipe_fd = 0; /* No pipe. */
info->exec_self = false;
info->method = INJECT_PTRACE;
return errcode;
}

DR_EXPORT
bool
dr_inject_prepare_to_ptrace(void *data)
Expand Down Expand Up @@ -1372,6 +1386,20 @@ detach_and_exec_gdb(process_id_t pid, const char *library_path)
ASSERT(false && "failed to exec gdb?");
}

/* singlestep traced process
*/
bool
ptrace_singlestep(process_id_t pid)
{
if (our_ptrace(PTRACE_SINGLESTEP, pid, NULL, NULL) < 0)
return false;

if (!wait_until_signal(pid, SIGTRAP))
return false;

return true;
}

bool
inject_ptrace(dr_inject_info_t *info, const char *library_path)
{
Expand Down Expand Up @@ -1406,6 +1434,12 @@ inject_ptrace(dr_inject_info_t *info, const char *library_path)
return false;
if (!continue_until_break(info->pid))
return false;
} else {
/* We are attached to target process, singlestep to make sure not returning from
* blocked syscall.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please elaborate in the comment: why does it matter if the app was at a "blocked" syscall or just at any syscall (e.g., inside SYS_getpid)? Why do we need to single-step in such cases? Will the kernel auto-adjust the PC for us for an auto-restart syscall?

*/
if (!ptrace_singlestep(info->pid))
return false;
}

/* Open libdynamorio.so as readonly in the child. */
Expand Down
63 changes: 60 additions & 3 deletions core/win32/injector.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
#include "../globals.h"
#define WIN32_LEAN_AND_MEAN
#include <windows.h>
#include <Psapi.h>
#include <commdlg.h>
#include <imagehlp.h>
#include <stdio.h>
Expand Down Expand Up @@ -235,6 +236,7 @@ tchar_to_char(const TCHAR *wstr, OUT char *buf, size_t buflen /*# elements*/)
typedef struct _dr_inject_info_t {
PROCESS_INFORMATION pi;
bool using_debugger_injection;
bool attached;
TCHAR wimage_name[MAXIMUM_PATH];
/* We need something to point at for dr_inject_get_image_name so we just
* keep a utf8 buffer as well.
Expand Down Expand Up @@ -754,6 +756,56 @@ append_app_arg_and_space(char *buf, size_t bufsz, size_t *sofar, const char *arg
}
}

DYNAMORIO_EXPORT
int
dr_inject_process_attach(process_id_t pid, void **data OUT)
{
dr_inject_info_t *info = HeapAlloc(GetProcessHeap(), 0, sizeof(*info));
memset(info, 0, sizeof(*info));
int errcode = ERROR_SUCCESS;
if (DebugActiveProcess((DWORD)pid)) {
info->using_debugger_injection = false;
info->attached = true;
DEBUG_EVENT dbgevt = { 0 };
for (;;) {
dbgevt.dwProcessId = (DWORD)pid;
WaitForDebugEvent(&dbgevt, INFINITE);
ContinueDebugEvent(dbgevt.dwProcessId, dbgevt.dwThreadId, DBG_CONTINUE);

if (dbgevt.dwDebugEventCode == CREATE_PROCESS_DEBUG_EVENT) {
break;
}
}
char szExePath[MAX_PATH];
summershrimp marked this conversation as resolved.
Show resolved Hide resolved
char *pExeName = NULL;
summershrimp marked this conversation as resolved.
Show resolved Hide resolved
GetModuleFileNameExA(dbgevt.u.CreateProcessInfo.hProcess, NULL, szExePath,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use the ExW version to properly handle non-ascii chars

MAX_PATH);
pExeName = strrchr(szExePath, '\\');
if (pExeName == NULL) {
return ERROR_INVALID_PARAMETER;
}

strcpy(info->image_name, pExeName + 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strcpy is not allowed: use strncpy and NULL_TERMINATE_BUFFER

char_to_tchar(info->image_name, info->wimage_name,
BUFFER_SIZE_ELEMENTS(info->wimage_name));

info->pi.dwProcessId = dbgevt.dwProcessId;
info->pi.dwThreadId = dbgevt.dwThreadId;

DuplicateHandle(GetCurrentProcess(), dbgevt.u.CreateProcessInfo.hProcess,
GetCurrentProcess(), &info->pi.hProcess, 0, FALSE,
DUPLICATE_SAME_ACCESS);

DuplicateHandle(GetCurrentProcess(), dbgevt.u.CreateProcessInfo.hThread,
GetCurrentProcess(), &info->pi.hThread, 0, FALSE,
DUPLICATE_SAME_ACCESS);
} else {
errcode = GetLastError();
}
*data = info;
return errcode;
}

/* Returns 0 on success.
* On failure, returns a Windows API error code.
*/
Expand Down Expand Up @@ -937,9 +989,14 @@ bool
dr_inject_process_run(void *data)
{
dr_inject_info_t *info = (dr_inject_info_t *)data;
/* resume the suspended app process so its main thread can run */
ResumeThread(info->pi.hThread);
close_handle(info->pi.hThread);
if (info->attached) {
/* detach the debugger */
DebugActiveProcessStop(info->pi.dwProcessId);
} else {
/* resume the suspended app process so its main thread can run */
ResumeThread(info->pi.hThread);
close_handle(info->pi.hThread);
}

return true;
}
Expand Down
46 changes: 34 additions & 12 deletions tools/drdeploy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,10 +286,10 @@ const char *options_list_str =
" -early Requests early injection (the default).\n"
" -late Requests late injection.\n"
# endif
" -attach <pid> Attach to the process with the given pid. Pass 0\n"
" for pid to launch and inject into a new process.\n"
" -logdir <dir> Logfiles will be stored in this directory.\n"
# endif
" -attach <pid> Attach to the process with the given pid. Pass 0\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The big problem with attach on Windows is that if we attach when a thread is inside a Windows callback we will lose control when it goes back into the kernel, since there is no way to know where it will resume. We have some syscall hooks to try and regain control but there will at least be a window where that thread is native. We should document this: maybe label Windows attach support as "experimental" or describe this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the main documentation ("page_deploy" in intro.dox) should talk about attaching as a supported feature?

It should be listed in the changelog in release.dox for sure as a major feature.

" for pid to launch and inject into a new process.\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why anyone would pass 0? If you don't want to attach to an existing process you just wouldn't pass -attach. I would remove that sentence and make 0 an invalid argument.

" -use_dll <dll> Inject given dll instead of configured DR dll.\n"
" -force Inject regardless of configuration.\n"
" -exit0 Return a 0 exit code instead of the app's exit code.\n"
Expand Down Expand Up @@ -1035,6 +1035,7 @@ _tmain(int argc, TCHAR *targv[])
bool use_ptrace = false;
bool kill_group = false;
# endif
process_id_t attach_pid = 0;
char *app_name = NULL;
char full_app_name[MAXIMUM_PATH];
const char **app_argv;
Expand Down Expand Up @@ -1170,23 +1171,25 @@ _tmain(int argc, TCHAR *targv[])
} else if (strcmp(argv[i], "-no_wait") == 0) {
limit = -1;
continue;
}
# ifdef UNIX
else if (strcmp(argv[i], "-use_ptrace") == 0) {
/* Undocumented option for using ptrace on a fresh process. */
use_ptrace = true;
continue;
} else if (strcmp(argv[i], "-attach") == 0) {
const char *pid_str = argv[++i];
process_id_t pid = strtoul(pid_str, NULL, 10);
if (pid == ULONG_MAX)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make 0 invalid as well

usage(false, "-attach expects an integer pid");
if (pid != 0)
usage(false, "attaching to running processes is not yet implemented");
attach_pid = pid;
limit = -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is your reasoning for not allowing a time limit on attaching? Maybe it would be less commonly used but is there a reason to disallow it?

# ifdef UNIX
use_ptrace = true;
# endif
/* FIXME: use pid below to attach. */
continue;
}
# ifdef UNIX
else if (strcmp(argv[i], "-use_ptrace") == 0) {
/* Undocumented option for using ptrace on a fresh process. */
use_ptrace = true;
continue;
}
# ifndef MACOS /* XXX i#1285: private loader NYI on MacOS */
else if (strcmp(argv[i], "-early") == 0) {
/* Now the default: left here just for back-compat */
Expand Down Expand Up @@ -1442,7 +1445,20 @@ _tmain(int argc, TCHAR *targv[])
/* Support no app if the tool has its own frontend, under the assumption
* it may have post-processing or other features.
*/
if (i < argc || native_tool[0] == '\0') {
# ifdef UNIX
if (attach_pid != 0) {
char *exe = malloc(PATH_MAX);
char *exe_str = malloc(PATH_MAX);
ssize_t size;
sprintf(exe_str, "/proc/%u/exe", attach_pid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sprintf is not allowed: use _snprintf and NULL_TERMINATE_BUFFER

size = readlink(exe_str, exe, PATH_MAX);
exe[size] = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the return value of readlink: this will overflow when size==-1!

app_name = strdup(exe);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory leak: never freed.

free(exe);
free(exe_str);
}
# endif /* UNIX */
if (attach_pid == 0 && (i < argc || native_tool[0] == '\0')) {
# endif
if (i >= argc)
usage(false, "%s", "no app specified");
Expand Down Expand Up @@ -1644,8 +1660,14 @@ _tmain(int argc, TCHAR *targv[])
if (limit == 0 && !use_ptrace && !kill_group) {
info("will exec %s", app_name);
errcode = dr_inject_prepare_to_exec(app_name, app_argv, &inject_data);
} else if (attach_pid != 0) {
errcode = dr_inject_prepare_to_attach(attach_pid, app_name, &inject_data);
} else
# endif /* UNIX */
# elif defined(WINDOWS)
if (attach_pid != 0) {
errcode = dr_inject_process_attach(attach_pid, &inject_data);
} else
# endif /* WINDOWS */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to write some simple tests of these new attach features? It will take a little more work than adding a regular test because you'll need to launch something in the background. See client.nudge_test and how it has a .runall file and uses run_in_bg to run the infloop app in the background.

If you'd prefer to make adding a test a separate CL that would be fine.

{
errcode = dr_inject_process_create(app_name, app_argv, &inject_data);
info("created child with pid %d for %s", dr_inject_get_process_id(inject_data),
Expand Down