i#38 attach: Make -skip_syscall the default

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.

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);
 

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 "<block>")
+        set(exe_ops "${exe_ops};-block")
+      endif ()
     else ()
       if ("${runall}" MATCHES "<attach>")
         # For attach we use attachee, which is identical to infloop, aside for writing
@@ -2386,10 +2389,12 @@ 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 "" "" "")
+  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)

suite/tests/client-interface/attach_blocking.expect

@@ -0,0 +1,4 @@
+starting
+thank you for testing attach
+thread init
+done

suite/tests/client-interface/attach_blocking.runall

@@ -0,0 +1 @@
+<attach><block>

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 <signal.h>
 #include <assert.h>
 #include <string.h>
+#include <errno.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <sys/select.h>
 
 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,10 +71,24 @@ 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;
     }
 
+    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.
@@ -80,14 +99,42 @@ 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++;
         if (counter > 15 * 1024 * 1024 * 1024LL) {
             print("hit max iters\n");
             break;
         }
+        if (block) {
+            /* Make a blocking syscall, but not forever to again guard against
+             * a runaway test.
+             */
+            struct timeval timeout;
+            timeout.tv_sec = 60;
+            timeout.tv_usec = 0;
+            fd_set set;
+            FD_ZERO(&set);
+            FD_SET(pipefd[0], &set);
+            int res = select(pipefd[0] + 1, &set, NULL, NULL, &timeout);
+            /* 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)
+                perror("select error");
+
+            /* XXX i#38: We may want a test of an auto-restart syscall as well
+             * once the injector handles that.
+             */
+        }
     }
+
+    if (block) {
+        close(pipefd[0]);
+        close(pipefd[1]);
+    }
+
     return 0;
 }

suite/tests/runall.cmake

@@ -197,22 +197,8 @@ if ("${nudge}" MATCHES "<use-persisted>")
   endif ()
 elseif ("${nudge}" MATCHES "<attach>")
   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 "<attach>"
-    "${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}

tools/drdeploy.c

@@ -302,13 +302,6 @@ const char *options_list_str =
     "       -attach <pid>      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 <pid>      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) {