i#5054: Fix attach crash and other problems (#5230)

Fixes a number of issues with Linux attach:

+ Set xdi to zero for x86 _start relocation of libdynamorio.

+ Implement remote memset for .bss zeroing in elf_loader_map_phdrs(),
  fixing a crash in some builds such as Ubuntu20 release build.

+ Don't kill target if attach fails.

+ Fix crash if no pid passed.

+ Adds a useful error message on failure to look at ptrace permissions.

+ Adds a warning to use -skip_syscall if attach hangs.

+ Adds a test by porting the Windows client.attach test to Linux.
  Disables the mprotect syscall due to weird failures which need to be
  examined.
  Further tests of blocking syscalls and -skip_syscall are needed.

Re-enables the attach help message for drrun and the deployment docs.

Tested release build on Ubuntu20 where the .bss crash reproduced every
run and is now gone.

Tested "ctest --repeat-until-fail 100 -V -R client.attach" on Ubuntu20
and on a Debian-ish system: no failures.

Issue: #38, #5054
Fixes #5054

.github/workflows/ci-x86.yml

@@ -92,6 +92,7 @@ jobs:
         sudo rsync -av ../extract/usr/lib/i386-linux-gnu/ /usr/lib/i386-linux-gnu/
         sudo rsync -av ../extract/lib/i386-linux-gnu/ /usr/lib/i386-linux-gnu/
         sudo rsync -av ../extract/usr/include/i386-linux-gnu/ /usr/include/
+        echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope
 
     # Use a newer cmake to avoid 32-bit toolchain problems (i#4830).
     - name: Setup newer cmake
@@ -152,6 +153,7 @@ jobs:
         sudo apt update
         sudo apt-get -y install doxygen vera++ zlib1g-dev libsnappy-dev \
           g++-multilib libunwind-dev
+        echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope
 
     # Use a newer cmake to avoid 32-bit toolchain problems (i#4830).
     - name: Setup newer cmake

api/docs/deployment.dox

@@ -315,14 +315,21 @@ under DynamoRIO with the bbsize sample client:
 % bin32/drrun -c samples/bin32/libbbsize.so -- ls
 \endcode
 
-\if DISABLED_UNTIL_BUG_5054_IS_FIXED
 Alternatively, you can first run the target, and then use \c drrun
 to attach to it, but please note that attaching is an experimental
-feature and is not yet as well-supported as launching a new process:
+feature and is not yet as well-supported as launching a new process.
+In particular, if the application is in the middle of a blocking syscall,
+DynamoRIO will wait for that to finish.  To instead force interruption of
+the syscall, additionally pass -skip_syscall.
 \code
 % bin32/drrun -attach <target_pid> -c samples/bin32/libbbsize.so
 \endcode
-\endif
+
+This attach feature requires ptrace capabilities, which can be enabled
+with this command:
+\code
+% echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope
+\endcode
 
 Run \c drrun with no options to get a list of the options and
 environment variable shortcuts it supports.  To disable following across

api/docs/release.dox

@@ -245,7 +245,7 @@ Further non-compatibility-affecting changes include:
  - Added drmodtrack_lookup_segment().
  - Added a new drrun option \p -attach for attaching to a running process.
    This is currently an experimental option and is not yet as well-supported
-   as launching a new process.
+   as launching a new process.  It is only supported on x86 at this time.
  - Added \ref page_drcallstack Extension for walking application callstacks, with
    an initial Linux-only implementation.
  - Added new #dr_cleancall_save_t flags which are required for proper interaction

core/drlibc/drlibc_module_elf.c

@@ -376,7 +376,8 @@ elf_loader_read_headers(elf_loader_t *elf, const char *filename)
 app_pc
 elf_loader_map_phdrs(elf_loader_t *elf, bool fixed, map_fn_t map_func,
                      unmap_fn_t unmap_func, prot_fn_t prot_func,
-                     check_bounds_fn_t check_bounds_func, modload_flags_t flags)
+                     check_bounds_fn_t check_bounds_func, memset_fn_t memset_func,
+                     modload_flags_t flags)
 {
     app_pc lib_base, lib_end, last_end;
     ELF_HEADER_TYPE *elf_hdr = elf->ehdr;
@@ -500,16 +501,11 @@ elf_loader_map_phdrs(elf_loader_t *elf, bool fixed, map_fn_t map_func,
                     /* fill zeros at extend size */
                     file_end = (app_pc)prog_hdr->p_vaddr + prog_hdr->p_filesz;
                     if (seg_end > file_end + delta) {
-                        if (!TEST(MODLOAD_SEPARATE_PROCESS, flags)) {
-                            memset(file_end + delta, 0, seg_end - (file_end + delta));
-                        } else {
-                            /* FIXME i#37: use a remote memset to zero out this gap or
-                             * fix it up in the child.  There is typically one RW
-                             * PT_LOAD segment for .data and .bss.  If .data ends and
-                             * .bss starts before filesz bytes, we need to zero the .bss
-                             * bytes manually.
-                             */
-                        }
+                        /* There is typically one RW PT_LOAD segment for .data and
+                         * .bss.  If .data ends and .bss starts before filesz bytes,
+                         * we need to zero the .bss bytes manually.
+                         */
+                        (*memset_func)(file_end + delta, 0, seg_end - (file_end + delta));
                     }
                 }
             }

core/unix/injector.c

@@ -875,6 +875,7 @@ enum { MAX_SHELL_CODE = 4096 };
 #        define USER_REGS_TYPE user_regs_struct
 #        define REG_PC_FIELD IF_X64_ELSE(rip, eip)
 #        define REG_SP_FIELD IF_X64_ELSE(rsp, esp)
+#        define REG_DI_FIELD IF_X64_ELSE(rdi, edi)
 #        define REG_RETVAL_FIELD IF_X64_ELSE(rax, eax)
 #    elif defined(DR_HOST_ARM)
 /* On AArch32, glibc uses user_regs instead of user_regs_struct.
@@ -1372,6 +1373,53 @@ injectee_prot(byte *addr, size_t size, uint prot /*MEMPROT_*/)
     return true;
 }
 
+static void *
+injectee_memset(void *dst, int val, size_t size)
+{
+    ptr_int_t *dst_addr = dst;
+    ptr_int_t src_val;
+    long r;
+    if (!ALIGNED(dst_addr, sizeof(ptr_int_t))) {
+        dst_addr = (ptr_int_t *)ALIGN_BACKWARD(dst_addr, sizeof(ptr_int_t));
+        r = our_ptrace(PTRACE_PEEKDATA, injector_info->pid, dst_addr, &src_val);
+        if (r < 0)
+            return NULL;
+        /* Set the top bytes to val. */
+        uint offs = (byte *)dst - (byte *)dst_addr;
+        byte *dst_byte = (byte *)&src_val;
+        dst_byte += offs;
+        for (int i = 0; i < sizeof(ptr_int_t) - offs; i++, dst_byte++)
+            *dst_byte = val;
+        r = our_ptrace(PTRACE_POKEDATA, injector_info->pid, dst_addr, (void *)src_val);
+        if (r < 0)
+            return NULL;
+        dst_addr++;
+    }
+    src_val = 0;
+    for (uint i = 0; i < sizeof(ptr_int_t); i++)
+        src_val |= val << 8 * i;
+    for (; dst_addr + 1 < (ptr_int_t *)((byte *)dst + size); dst_addr++) {
+        r = our_ptrace(PTRACE_POKEDATA, injector_info->pid, dst_addr, (void *)src_val);
+        if (r < 0)
+            return NULL;
+    }
+    if (dst_addr + 1 > (ptr_int_t *)((byte *)dst + size)) {
+        r = our_ptrace(PTRACE_PEEKDATA, injector_info->pid, dst_addr, &src_val);
+        if (r < 0)
+            return NULL;
+        /* Set the bottom bytes to val. */
+        int offs = (byte *)(dst_addr + 1) - ((byte *)dst + size);
+        byte *dst_byte = (byte *)&src_val;
+        for (int i = 0; i < sizeof(ptr_int_t) - offs; i++, dst_byte++)
+            *dst_byte = val;
+        r = our_ptrace(PTRACE_POKEDATA, injector_info->pid, dst_addr, (void *)src_val);
+        if (r < 0)
+            return NULL;
+        dst_addr++;
+    }
+    return dst;
+}
+
 /* Convert a user_regs_struct used by the ptrace API into DR's priv_mcontext_t
  * struct.
  */
@@ -1599,9 +1647,9 @@ inject_ptrace(dr_inject_info_t *info, const char *library_path)
     injector_info = info;
     injector_dr_fd = loader.fd;
     injectee_dr_fd = dr_fd;
-    injected_base = elf_loader_map_phdrs(&loader, true /*fixed*/, injectee_map_file,
-                                         injectee_unmap, injectee_prot, NULL,
-                                         MODLOAD_SEPARATE_PROCESS /*!reachable*/);
+    injected_base = elf_loader_map_phdrs(
+        &loader, true /*fixed*/, injectee_map_file, injectee_unmap, injectee_prot, NULL,
+        injectee_memset, MODLOAD_SEPARATE_PROCESS /*!reachable*/);
     if (injected_base == NULL) {
         if (verbose)
             fprintf(stderr, "Unable to mmap libdynamorio.so in injectee\n");
@@ -1669,6 +1717,11 @@ inject_ptrace(dr_inject_info_t *info, const char *library_path)
 #        error "depends on arch stack growth direction"
 #    endif
 
+#    ifdef X86
+    /* _start for x86 assumes xdi starts out 0.  Otherwise relocation is skipped. */
+    regs.REG_DI_FIELD = 0;
+#    endif
+
     regs.REG_PC_FIELD = (ptr_int_t)injected_dr_start;
     our_ptrace_setregs(info->pid, &regs);
 

core/unix/loader.c

@@ -522,9 +522,9 @@ privload_map_and_relocate(const char *filename, size_t *size OUT, modload_flags_
         }
         return NULL;
     }
-    base =
-        elf_loader_map_phdrs(&loader, false /* fixed */, map_func, unmap_func, prot_func,
-                             privload_check_new_map_bounds, privload_map_flags(flags));
+    base = elf_loader_map_phdrs(&loader, false /* fixed */, map_func, unmap_func,
+                                prot_func, privload_check_new_map_bounds, memset,
+                                privload_map_flags(flags));
     if (base != NULL) {
         if (size != NULL)
             *size = loader.image_size;
@@ -1897,7 +1897,7 @@ reload_dynamorio(void **init_sp, app_pc conflict_start, app_pc conflict_end)
     /* Now load the 2nd libdynamorio.so */
     dr_map = elf_loader_map_phdrs(&dr_ld, false /*!fixed*/, os_map_file, os_unmap_file,
                                   os_set_protection, privload_check_new_map_bounds,
-                                  privload_map_flags(0 /*!reachable*/));
+                                  memset, privload_map_flags(0 /*!reachable*/));
     ASSERT(dr_map != NULL);
     ASSERT(is_elf_so_header(dr_map, 0));
 
@@ -2054,7 +2054,7 @@ privload_early_inject(void **sp, byte *old_libdr_base, size_t old_libdr_size)
                                    true,
                                    /* ensure there's space for the brk */
                                    map_exe_file_and_brk, os_unmap_file, os_set_protection,
-                                   privload_check_new_map_bounds,
+                                   privload_check_new_map_bounds, memset,
                                    privload_map_flags(MODLOAD_IS_APP /*!reachable*/));
     apicheck(exe_map != NULL,
              "Failed to load application.  "
@@ -2114,7 +2114,7 @@ privload_early_inject(void **sp, byte *old_libdr_base, size_t old_libdr_size)
         apicheck(success, "Failed to read ELF interpreter headers.");
         interp_map = elf_loader_map_phdrs(
             &interp_ld, false /* fixed */, os_map_file, os_unmap_file, os_set_protection,
-            privload_check_new_map_bounds,
+            privload_check_new_map_bounds, memset,
             privload_map_flags(MODLOAD_IS_APP /*!reachable*/));
         apicheck(interp_map != NULL && is_elf_so_header(interp_map, 0),
                  "Failed to map ELF interpreter.");

core/unix/module_elf.h

@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2011-2020 Google, Inc.  All rights reserved.
+ * Copyright (c) 2011-2021 Google, Inc.  All rights reserved.
  * Copyright (c) 2008-2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -268,6 +268,7 @@ typedef byte *(*map_fn_t)(file_t f, size_t *size INOUT, uint64 offs, app_pc addr
 typedef bool (*unmap_fn_t)(byte *map, size_t size);
 typedef bool (*prot_fn_t)(byte *map, size_t size, uint prot /*MEMPROT_*/);
 typedef void (*check_bounds_fn_t)(elf_loader_t *elf, byte *start, byte *end);
+typedef void *(*memset_fn_t)(void *dst, int val, size_t size);
 
 /* Initialized an ELF loader for use with the given file. */
 bool
@@ -308,7 +309,8 @@ elf_loader_map_file(elf_loader_t *elf, bool reachable);
 app_pc
 elf_loader_map_phdrs(elf_loader_t *elf, bool fixed, map_fn_t map_func,
                      unmap_fn_t unmap_func, prot_fn_t prot_func,
-                     check_bounds_fn_t check_bounds_func, modload_flags_t flags);
+                     check_bounds_fn_t check_bounds_func, memset_fn_t memset_func,
+                     modload_flags_t flags);
 
 /* Iterate program headers of a mapped ELF image and find the string that
  * PT_INTERP points to.  Typically this comes early in the file and is always

suite/tests/CMakeLists.txt

@@ -1307,6 +1307,11 @@ function(torun test key source native standalone_dr dr_ops exe_ops added_out)
     if (UNIX)
       # We always run infloop.
       get_target_path_for_execution(app_path linux.infloop "${location_suffix}")
+      if ("${runall}" MATCHES "<attach>")
+        # Add a starting message with -v, and avoid mprotect for now (i#38: mprotect
+        # is failing).
+        set(exe_ops "${exe_ops};-v;-attach")
+      endif ()
     else ()
       if ("${runall}" MATCHES "<attach>")
         # For attach we use attachee, which is identical to infloop, aside for writing
@@ -2369,6 +2374,14 @@ if (ANNOTATIONS AND NOT ARM)
       client-interface/vg-annot.c "truncate@2" "" "")
   endif (UNIX OR NOT X64)
 endif (ANNOTATIONS AND NOT ARM)
+
+if (NOT ANDROID) # TODO i#38: Port test to Android.
+  if (X86) # TODO i#38,i#1550: Port injector.c attach gencode to AArchXX.
+    # TODO i#38: Add targeted Linux tests of attaching during a blocking syscall.
+    tobuild_ci(client.attach_test client-interface/attach_test.runall "" "" "")
+  endif ()
+endif ()
+
 if (UNIX)
   if (NOT ARM) # FIXME i#1551: fix bugs on ARM
     if (AARCH64)
@@ -2409,7 +2422,6 @@ if (UNIX)
     use_DynamoRIO_extension(client.emulation_api_simple.dll drmgr)
   endif ()
 else (UNIX)
-  tobuild_ci(client.attach_test client-interface/attach_test.runall "" "" "")
   tobuild_ci(client.events client-interface/events.c
     "" "" "${events_appdll_path}")
   tobuild_ci(client.events_cpp client-interface/events_cpp.cpp

suite/tests/client-interface/attach_test.dll.c

@@ -1,4 +1,5 @@
 /* **********************************************************
+ * Copyright (c) 2021 Google, Inc.  All rights reserved.
  * Copyright (c) 2008-2010 VMware, Inc.  All rights reserved.
  * **********************************************************/
 
@@ -32,27 +33,41 @@
 
 #include "dr_api.h"
 #include "client_tools.h"
-#include "Windows.h"
+#ifdef WINDOWS
+#    include <windows.h>
+#endif
 
 static thread_id_t injection_tid;
 static bool first_thread = true;
 
 static void
 dr_exit(void)
 {
+#ifdef WINDOWS
     dr_fprintf(STDERR, "done\n");
+#else
+    /* The app prints 'done' for us. */
+#endif
 }
 
 static void
 dr_thread_init(void *drcontext)
 {
     thread_id_t tid = dr_get_thread_id(drcontext);
+#ifdef WINDOWS
+    /* On Windows there is an additional thread used for attach injection.
+     * XXX i#725: We should remove it or hide it, and not rely on it here.
+     */
     if (tid != injection_tid && first_thread) {
         first_thread = false;
         dr_fprintf(STDERR, "thread init\n");
     }
+#else
+    dr_fprintf(STDERR, "thread init\n");
+#endif
 }
 
+#ifdef WINDOWS
 static bool
 dr_exception_event(void *drcontext, dr_exception_t *excpt)
 {
@@ -71,6 +86,7 @@ dr_exception_event(void *drcontext, dr_exception_t *excpt)
 
     return true;
 }
+#endif
 
 DR_EXPORT
 void
@@ -80,6 +96,8 @@ dr_init(client_id_t id)
     injection_tid = dr_get_thread_id(drcontext);
     dr_register_exit_event(dr_exit);
     dr_register_thread_init_event(dr_thread_init);
+#ifdef WINDOWS
     dr_register_exception_event(dr_exception_event);
+#endif
     dr_fprintf(STDERR, "thank you for testing attach\n");
 }

suite/tests/client-interface/attach_test.template

@@ -1,4 +1,8 @@
+#ifdef WINDOWS
 starting attachee
+#else
+starting
+#endif
 thank you for testing attach
 thread init
 #ifdef WINDOWS

suite/tests/linux/infloop.c

@@ -54,6 +54,7 @@ main(int argc, const char *argv[])
 {
     int arg_offs = 1;
     long long counter = 0;
+    bool for_attach = 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
@@ -62,15 +63,23 @@ main(int argc, const char *argv[])
             intercept_signal(SIGTERM, (handler_3_t)signal_handler, false);
             print("starting\n");
             arg_offs++;
+        } else if (strcmp(argv[arg_offs], "-attach") == 0) {
+            for_attach = true;
+            arg_offs++;
         } else
             return 1;
     }
 
     while (1) {
-        /* workaround for PR 213040 and i#1087: prevent loop from being coarse
-         * by using a non-ignorable system call
+        /* XXX i#38: We're seeing mprotect fail strangely on attach right before
+         * DR takes over.  For now we avoid it in that test.
          */
-        protect_mem((void *)signal_handler, 1, ALLOW_READ | ALLOW_EXEC);
+        if (!for_attach) {
+            /* workaround for PR 213040 and i#1087: prevent loop from being coarse
+             * by using a non-ignorable system call
+             */
+            protect_mem((void *)signal_handler, 1, ALLOW_READ | ALLOW_EXEC);
+        }
         /* don't spin forever to avoid hosing machines if test harness somehow
          * fails to kill.  15 billion syscalls takes ~ 1 minute.
          */