From 141f405ec011830984706e38024c4c0b36f28fdf Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Wed, 13 Apr 2022 22:51:57 -0400 Subject: [PATCH 1/4] i#38 attach: Make -skip_syscall the default Makes interrupting a blocking syscall on attach the default behavior. Removes the -skip_syscall drrun/drinject parameter. Adds a test by adding an optional blocking syscall to infloop. Issue: #38 --- api/docs/release.dox | 3 +- suite/tests/CMakeLists.txt | 8 +++-- .../client-interface/attach_blocking.expect | 6 ++++ .../client-interface/attach_blocking.runall | 1 + suite/tests/linux/infloop.c | 32 +++++++++++++++++-- suite/tests/runall.cmake | 16 +--------- tools/drdeploy.c | 21 ++---------- 7 files changed, 47 insertions(+), 40 deletions(-) create mode 100644 suite/tests/client-interface/attach_blocking.expect create mode 100644 suite/tests/client-interface/attach_blocking.runall diff --git a/api/docs/release.dox b/api/docs/release.dox index 320852f7568..3dd9c9fa88c 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -126,7 +126,8 @@ clients. The changes between version \DR_VERSION and 9.0.1 include the following compatibility changes: - - Nothing yet (placeholder). + - Eliminated the -skip_syscall option to drrun and drinject, which is now always + on by default. Further non-compatibility-affecting changes include: - Added AArchXX support for attaching to a running process. diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt index ef16062c639..125150b97b7 100644 --- a/suite/tests/CMakeLists.txt +++ b/suite/tests/CMakeLists.txt @@ -1317,6 +1317,9 @@ function(torun test key source native standalone_dr dr_ops exe_ops added_out pas # is failing). set(exe_ops "${exe_ops};-v;-attach") endif () + if ("${runall}" MATCHES "") + set(exe_ops "${exe_ops};-block") + endif () else () if ("${runall}" MATCHES "") # For attach we use attachee, which is identical to infloop, aside for writing @@ -2386,10 +2389,9 @@ if (ANNOTATIONS AND NOT ARM) endif (ANNOTATIONS AND NOT ARM) if (NOT ANDROID) # TODO i#38: Port test to Android. - # TODO i#38: Add targeted Linux tests of attaching during a blocking syscall. - # That seems to happen with the infloop test on ARM already (we have to add - # -skip_syscall in runall.cmake). tobuild_ci(client.attach_test client-interface/attach_test.runall "" "" "") + torunonly_ci(client.attach_blocking linux.infloop client.attach_test.dll + client-interface/attach_blocking.runall "" "" "") endif () if (UNIX) diff --git a/suite/tests/client-interface/attach_blocking.expect b/suite/tests/client-interface/attach_blocking.expect new file mode 100644 index 00000000000..5409336c2c5 --- /dev/null +++ b/suite/tests/client-interface/attach_blocking.expect @@ -0,0 +1,6 @@ +starting +thank you for testing attach +thread init +select error: Interrupted system call +blocking syscall returned -1 errno=4 +done diff --git a/suite/tests/client-interface/attach_blocking.runall b/suite/tests/client-interface/attach_blocking.runall new file mode 100644 index 00000000000..9f9d9dd2f42 --- /dev/null +++ b/suite/tests/client-interface/attach_blocking.runall @@ -0,0 +1 @@ + diff --git a/suite/tests/linux/infloop.c b/suite/tests/linux/infloop.c index 4009fda34fc..c55071526cf 100644 --- a/suite/tests/linux/infloop.c +++ b/suite/tests/linux/infloop.c @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2011-2012 Google, Inc. All rights reserved. + * Copyright (c) 2011-2022 Google, Inc. All rights reserved. * Copyright (c) 2009-2010 VMware, Inc. All rights reserved. * **********************************************************/ @@ -39,6 +39,11 @@ #include #include #include +#include +#include +#include +#include +#include static void signal_handler(int sig) @@ -54,7 +59,7 @@ main(int argc, const char *argv[]) { int arg_offs = 1; long long counter = 0; - bool for_attach = false; + bool for_attach = false, block = false; while (arg_offs < argc && argv[arg_offs][0] == '-') { if (strcmp(argv[arg_offs], "-v") == 0) { /* enough verbosity to satisfy runall.cmake: needs an initial and a @@ -66,6 +71,9 @@ main(int argc, const char *argv[]) } else if (strcmp(argv[arg_offs], "-attach") == 0) { for_attach = true; arg_offs++; + } else if (strcmp(argv[arg_offs], "-block") == 0) { + block = true; + arg_offs++; } else return 1; } @@ -80,7 +88,7 @@ main(int argc, const char *argv[]) */ protect_mem((void *)signal_handler, 1, ALLOW_READ | ALLOW_EXEC); } - /* don't spin forever to avoid hosing machines if test harness somehow + /* Don't spin forever to avoid hosing machines if test harness somehow * fails to kill. 15 billion syscalls takes ~ 1 minute. */ counter++; @@ -88,6 +96,24 @@ main(int argc, const char *argv[]) print("hit max iters\n"); break; } + if (block) { + /* Make a blocking syscall, but not forever to again guard against + * a runaway test. + */ + int res; + struct timeval timeout; + timeout.tv_sec = 60; + timeout.tv_usec = 0; + fd_set set; + FD_ZERO(&set); + FD_SET(0, &set); + res = select(0 + 1, &set, NULL, NULL, &timeout); + if (res == -1) + perror("select error"); + else if (res == 0) + print("select timeout\n"); + print("blocking syscall returned %d errno=%d\n", res, errno); + } } return 0; } diff --git a/suite/tests/runall.cmake b/suite/tests/runall.cmake index 10210d09316..7863aebeadf 100644 --- a/suite/tests/runall.cmake +++ b/suite/tests/runall.cmake @@ -197,22 +197,8 @@ if ("${nudge}" MATCHES "") endif () elseif ("${nudge}" MATCHES "") set(nudge_cmd run_in_bg) - - set(extra_ops "") - if (UNIX) - # We're not yet explicitly testing -skip_syscall b/c it's considered experimental, - # but we seem to need it on ARM where infloop is in a blocking syscall. - # Not sure why it's different vs other arches. - # XXX i#38: Add additional explicit tests of blocking syscalls, and then turn - # -skip_syscall on by default all the time. - execute_process(COMMAND uname -m OUTPUT_VARIABLE HOST_ARCH) - if ("${HOST_ARCH}" MATCHES "^arm") - set(extra_ops "@-skip_syscall") - endif () - endif () - string(REGEX REPLACE "" - "${toolbindir}/drrun@-attach@${pid}${extra_ops}@-takeover_sleep@-takeovers@1000" + "${toolbindir}/drrun@-attach@${pid}@-takeover_sleep@-takeovers@1000" nudge "${nudge}") string(REGEX REPLACE "@" ";" nudge "${nudge}") execute_process(COMMAND "${toolbindir}/${nudge_cmd}" ${nudge} diff --git a/tools/drdeploy.c b/tools/drdeploy.c index c9c5c5521ff..3670f65e18a 100644 --- a/tools/drdeploy.c +++ b/tools/drdeploy.c @@ -302,13 +302,6 @@ const char *options_list_str = " -attach Attach to the process with the given pid.\n" " Attaching is an experimental feature and is not yet\n" " as well-supported as launching a new process.\n" - " When attaching to a process in the middle of a blocking\n" - " system call, DynamoRIO will wait until it returns.\n" - " Use -skip_syscall to force interruption.\n" - " -skip_syscall (Experimental)\n" - " Only works with -attach.\n" - " Attaching to a process will force blocking system calls\n" - " to fail with EINTR.\n" # endif # ifdef WINDOWS " -attach Attach to the process with the given pid.\n" @@ -1192,7 +1185,6 @@ _tmain(int argc, TCHAR *targv[]) time_t start_time, end_time; # else bool use_ptrace = false; - bool wait_syscall = true; bool kill_group = false; # endif process_id_t attach_pid = 0; @@ -1379,12 +1371,6 @@ _tmain(int argc, TCHAR *targv[]) "-sleep_between_takeovers"); continue; } -# ifdef UNIX - else if (strcmp(argv[i], "-skip_syscall") == 0) { - wait_syscall = false; - continue; - } -# endif # ifdef UNIX else if (strcmp(argv[i], "-use_ptrace") == 0) { /* Undocumented option for using ptrace on a fresh process. */ @@ -1886,8 +1872,9 @@ _tmain(int argc, TCHAR *targv[]) 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, wait_syscall, &inject_data); + /* We always try to avoid hanging on a blocked syscall. */ + errcode = dr_inject_prepare_to_attach(attach_pid, app_name, + /*wait_syscall=*/false, &inject_data); } else # elif defined(WINDOWS) if (attach_pid != 0) { @@ -1978,8 +1965,6 @@ _tmain(int argc, TCHAR *targv[]) goto error; } else { info("using ptrace to inject"); - if (wait_syscall) - warn("using experimental attach feature; if it hangs, try -skip_syscall"); } } if (kill_group) { From eddaa913911c551f3b89e95ac613e6aa8913aca5 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Wed, 13 Apr 2022 23:43:05 -0400 Subject: [PATCH 2/4] Blocking test is UNIX-only; switch from stdin to a pipe to avoid not blocking in the suite --- suite/tests/CMakeLists.txt | 7 +++++-- suite/tests/linux/infloop.c | 22 +++++++++++++++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt index 125150b97b7..8ca26d47fd5 100644 --- a/suite/tests/CMakeLists.txt +++ b/suite/tests/CMakeLists.txt @@ -2390,8 +2390,11 @@ endif (ANNOTATIONS AND NOT ARM) if (NOT ANDROID) # TODO i#38: Port test to Android. tobuild_ci(client.attach_test client-interface/attach_test.runall "" "" "") - torunonly_ci(client.attach_blocking linux.infloop client.attach_test.dll - client-interface/attach_blocking.runall "" "" "") + if (UNIX) + # Test attaching during a blocking syscall. + torunonly_ci(client.attach_blocking linux.infloop client.attach_test.dll + client-interface/attach_blocking.runall "" "" "") + endif () endif () if (UNIX) diff --git a/suite/tests/linux/infloop.c b/suite/tests/linux/infloop.c index c55071526cf..0cecd93f95f 100644 --- a/suite/tests/linux/infloop.c +++ b/suite/tests/linux/infloop.c @@ -78,6 +78,17 @@ main(int argc, const char *argv[]) return 1; } + int pipefd[2]; + if (block) { + /* Create something we can block reading. Stdin is too risky: in the + * test suite it has data. + */ + if (pipe(pipefd) == -1) { + perror("pipe"); + return 1; + } + } + while (1) { /* XXX i#38: We're seeing mprotect fail strangely on attach right before * DR takes over. For now we avoid it in that test. @@ -100,14 +111,13 @@ main(int argc, const char *argv[]) /* Make a blocking syscall, but not forever to again guard against * a runaway test. */ - int res; struct timeval timeout; timeout.tv_sec = 60; timeout.tv_usec = 0; fd_set set; FD_ZERO(&set); - FD_SET(0, &set); - res = select(0 + 1, &set, NULL, NULL, &timeout); + FD_SET(pipefd[0], &set); + int res = select(pipefd[0] + 1, &set, NULL, NULL, &timeout); if (res == -1) perror("select error"); else if (res == 0) @@ -115,5 +125,11 @@ main(int argc, const char *argv[]) print("blocking syscall returned %d errno=%d\n", res, errno); } } + + if (block) { + close(pipefd[0]); + close(pipefd[1]); + } + return 0; } From d6e7b29b348687b0b6da2b2496dd4687f8f008bb Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Thu, 14 Apr 2022 19:16:39 -0400 Subject: [PATCH 3/4] Consider EINTR to be racy as to whether it happens; add diagnostics --- core/unix/injector.c | 30 +++++++++++++++---- .../client-interface/attach_blocking.expect | 2 -- suite/tests/linux/infloop.c | 15 +++++++--- 3 files changed, 36 insertions(+), 11 deletions(-) diff --git a/core/unix/injector.c b/core/unix/injector.c index 86175315711..9b4b3b3cfd4 100644 --- a/core/unix/injector.c +++ b/core/unix/injector.c @@ -146,7 +146,7 @@ typedef struct _dr_inject_info_t { int exitcode; bool no_emulate_brk; /* is -no_emulate_brk in the option string? */ - bool wait_syscall; /* valid iff -attach, handlle blocking syscalls */ + bool wait_syscall; /* valid iff -attach: handle blocking syscalls */ #ifdef MACOS bool spawn_32bit; @@ -1816,11 +1816,21 @@ inject_ptrace(dr_inject_info_t *info, const char *library_path) */ if (!info->wait_syscall) { if (is_prev_bytes_syscall(info->pid, (app_pc)regs.REG_PC_FIELD, app_mode)) { - /* prev bytes might can match by accident, so check return value */ + /* Prev bytes might match by accident, so check return value too. */ + /* XXX i#38: If we interrupt an auto-restart syscall, we want to shift + * the app takeover PC back and restore the syscall number: but it's not + * easy to find the number. (On some AArch64 kernels, the kernel does + * this for us, for both auto-restart and interruptible.) + */ if (regs.REG_RETVAL_FIELD == -ERESTARTSYS || regs.REG_RETVAL_FIELD == -ERESTARTNOINTR || - regs.REG_RETVAL_FIELD == -ERESTARTNOHAND) + regs.REG_RETVAL_FIELD == -ERESTARTNOHAND) { + if (verbose) { + fprintf(stderr, "Post-syscall: changing %ld to -EINTR\n", + (long int)regs.REG_RETVAL_FIELD); + } regs.REG_RETVAL_FIELD = -EINTR; + } } } @@ -1881,11 +1891,21 @@ inject_ptrace(dr_inject_info_t *info, const char *library_path) do { /* Continue or deliver pending signal from status. */ r = our_ptrace(PTRACE_CONT, info->pid, NULL, (void *)(ptr_int_t)signal); - if (r < 0) + if (r < 0) { + if (verbose) + perror("PTRACE_CONT failed"); return false; + } r = waitpid(info->pid, &status, 0); - if (r < 0 || !WIFSTOPPED(status)) + if (r < 0 || !WIFSTOPPED(status)) { + if (verbose) { + if (r < 0) + perror("waitpid failed"); + else + fprintf(stderr, "bad status 0x%x\n", status); + } return false; + } signal = WSTOPSIG(status); } while (signal == SIGSEGV || signal == SIGILL); diff --git a/suite/tests/client-interface/attach_blocking.expect b/suite/tests/client-interface/attach_blocking.expect index 5409336c2c5..fe3e4f72bc3 100644 --- a/suite/tests/client-interface/attach_blocking.expect +++ b/suite/tests/client-interface/attach_blocking.expect @@ -1,6 +1,4 @@ starting thank you for testing attach thread init -select error: Interrupted system call -blocking syscall returned -1 errno=4 done diff --git a/suite/tests/linux/infloop.c b/suite/tests/linux/infloop.c index 0cecd93f95f..e6b4ad39b5d 100644 --- a/suite/tests/linux/infloop.c +++ b/suite/tests/linux/infloop.c @@ -118,11 +118,18 @@ main(int argc, const char *argv[]) FD_ZERO(&set); FD_SET(pipefd[0], &set); int res = select(pipefd[0] + 1, &set, NULL, NULL, &timeout); - if (res == -1) + /* Don't print on EINTR nor on a timeout as both can happen depending + * on the timing of the attach. + */ + if (res == -1 && errno == EINTR) { + /* What we expect for some kernels: our attach interrupted the + * syscall, which is not an auto-restart syscall. + */ + } else if (res == -1) perror("select error"); - else if (res == 0) - print("select timeout\n"); - print("blocking syscall returned %d errno=%d\n", res, errno); + /* XXX i#38: We may want a test of an auto-restart syscall as well + * once the injector handles that. + */ } } From 362f1c5105b64041cf33d4e7a5c488cf8abcd3c5 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Fri, 15 Apr 2022 16:24:43 -0400 Subject: [PATCH 4/4] Simplify infloop errno check --- suite/tests/linux/infloop.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/suite/tests/linux/infloop.c b/suite/tests/linux/infloop.c index e6b4ad39b5d..d85c8a75b01 100644 --- a/suite/tests/linux/infloop.c +++ b/suite/tests/linux/infloop.c @@ -118,15 +118,13 @@ main(int argc, const char *argv[]) FD_ZERO(&set); FD_SET(pipefd[0], &set); int res = select(pipefd[0] + 1, &set, NULL, NULL, &timeout); - /* Don't print on EINTR nor on a timeout as both can happen depending - * on the timing of the attach. + /* For some kernels: our attach interrupts the syscall (which is not an + * auto-restart syscall) and returns EINTR. Don't print on EINTR nor on a + * timeout as both can happen depending on the timing of the attach. */ - if (res == -1 && errno == EINTR) { - /* What we expect for some kernels: our attach interrupted the - * syscall, which is not an auto-restart syscall. - */ - } else if (res == -1) + if (res == -1 && errno != EINTR) perror("select error"); + /* XXX i#38: We may want a test of an auto-restart syscall as well * once the injector handles that. */