[Android] libheap_profiler/heap_dump: use ptrace instead of SIGSTOP.

tools/android/heap_profiler/heap_dump.c

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // The client dump tool for libheap_profiler. It attaches to a process (given
 // its pid) and dumps all the libheap_profiler tracking information in JSON.
-// The target process is frozen (SIGSTOP) while dumping, unless the -n opt.
-// is passed (in which case the caller is responsible for un/freezing).
 // The JSON output looks like this:
 // {
 //   "total_allocated": 908748493,   # Total bytes allocated and not freed.
 //   "num_allocs":      37542,       # Number of allocations.
 //   "num_stacks":      3723,        # Number of allocation call-sites.
 //   "allocs":                       # Optional. Printed only with the -x arg.
 //   {
 //     "beef1234": {"l": 17, "f": 1, "s": "1a"},
 //      ^            ^        ^       ^ Index of the corresponding entry in the
 //      |            |        |         next "stacks" section. Essentially a ref
 //      |            |        |         to the call site that created the alloc.
 //      |            |        |
 //      |            |        +-------> Flags (last arg of heap_profiler_alloc).
 //      |            +----------------> Length of the Alloc.
 //      +-----------------------------> Start address of the Alloc (hex).
 //   },
 //   "stacks":
 //   {
 //      "1a": {"l": 17, "f": [1074792772, 1100849864, 1100850688, ...]},
 //       ^      ^        ^
 //       |      |        +-----> Stack frames (absolute virtual addresses).
 //       |      +--------------> Bytes allocated and not freed by the call site.
 //       +---------------------> Index of the entry (as for "allocs" xref).
 //                               Indexes are hex and might not be monotonic.
 
 #include <fcntl.h>
 #include <inttypes.h>
-#include <signal.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <time.h>
 #include <unistd.h>
+#include <sys/ptrace.h>
 #include <sys/stat.h>
 
 #include "tools/android/heap_profiler/heap_profiler.h"
 
 
 static void lseek_abs(int fd, size_t off);
 static void read_proc_cmdline(char* cmdline, int size);
 
 static int pid;
 
 
 static int dump_process_heap(
     int mem_fd,
     FILE* fmaps,
     bool dump_also_allocs,
+    bool pedantic,  // Enable pedantic consistency checks on memory counters.
     char* comment) {
   HeapStats stats;
   time_t tm;
   char cmdline[512];
 
   tm = time(NULL);
   read_proc_cmdline(cmdline, sizeof(cmdline));
 
   // Look for the mmap which contains the HeapStats in the target process vmem.
   // On Linux/Android, the libheap_profiler mmaps explicitly /dev/zero. The
   // region furthermore starts with a magic marker to disambiguate.
   bool stats_mmap_found = false;
   for (;;) {
     char line[1024];
     if (fgets(line, sizeof(line), fmaps) == NULL)
       break;
 
     uintptr_t start;
     uintptr_t end;
     char map_file[32];
     int ret = sscanf(line, "%"SCNxPTR"-%"SCNxPTR" rw-p %*s %*s %*s %31s",
                      &start, &end, map_file);
     const size_t size = end - start + 1;
     if (ret != 3 || strcmp(map_file, "/dev/zero") != 0 || size < sizeof(stats))
       continue;
 
     // The mmap looks promising. Let's check for the magic marker.
     lseek_abs(mem_fd, start);
-    if (read(mem_fd, &stats, sizeof(stats)) < sizeof(stats))
-      continue;
+    ssize_t rsize = read(mem_fd, &stats, sizeof(stats));
+
+    if (rsize == -1) {
+      perror("read");
+      return -1;
+    }
+
+    if (rsize < sizeof(stats))
+      continue;  // A false positive.

[pasko, 2014/09/10 12:09:18]

this could be a short read, please check for EINTR and repeat the read, just
like we do in base/posix/eintr_wrapper.h

Please do that for all read() in this file.

The term "false positive" in the comment is a bit too generic. Can we just say
"This mapping has a too narrow of a margin to contain HeapStats", or something
like that? I would be OK with removing the comment whatsoever, because the code
speaks for itself.

[Primiano Tucci (use gerrit), 2014/09/10 13:17:35]

Very hard this will happen (The Android framework is oblivious of the existance
of this process, as it is a shell process, and I don't see who could send a
signal here).
But adding the wrapper should be pretty straightforward.


I would be OK with removing the comment whatsoever, because the code
Me too. Removing the comment.

 
     if (stats.magic_start == HEAP_PROFILER_MAGIC_MARKER) {
       stats_mmap_found = true;
       break;
     }
   }
 
   if (!stats_mmap_found) {
     fprintf(stderr, "Could not find the HeapStats area. "
                     "It looks like libheap_profiler is not loaded.\n");
@@ skipping 45 matching lines @@
       ++dbg_counted_allocs;
 
       if (prepend_trailing_comma)
         printf(",");
       prepend_trailing_comma = true;
       printf("\"%"PRIxPTR"\": {\"l\": %zu, \"f\": %"PRIu32", \"s\": \"%zx\"}",
              alloc.start, alloc_size, alloc.flags, stack_idx);
     }
     printf("},\n");
 
-    if (dbg_counted_allocs != stats.num_allocs) {
+    if (pedantic && dbg_counted_allocs != stats.num_allocs) {
       fprintf(stderr,
               "ERROR: inconsistent alloc count (%"PRIu32" vs %"PRIu32").\n",
               dbg_counted_allocs, stats.num_allocs);
       return -1;
     }
 
-    if (dbg_counted_total_alloc_bytes != stats.total_alloc_bytes) {
+    if (pedantic && dbg_counted_total_alloc_bytes != stats.total_alloc_bytes) {
       fprintf(stderr, "ERROR: inconsistent alloc totals (%zu vs %zu).\n",
               dbg_counted_total_alloc_bytes, stats.total_alloc_bytes);
       return -1;
     }
   }
 
   // Dump the distinct stack traces.
   printf("  \"stacks\": {");
   prepend_trailing_comma = false;
   dbg_counted_total_alloc_bytes = 0;
@@ skipping 23 matching lines @@
       ++n;
       if (n == HEAP_PROFILER_MAX_DEPTH || st.frames[n] == 0)
         break;
       else
         printf(",");
     }
     printf("]}");
   }
   printf("}\n}\n");
 
-  if (dbg_counted_total_alloc_bytes != stats.total_alloc_bytes) {
+  if (pedantic && dbg_counted_total_alloc_bytes != stats.total_alloc_bytes) {
     fprintf(stderr, "ERROR: inconsistent stacks totals (%zu vs %zu).\n",
             dbg_counted_total_alloc_bytes, stats.total_alloc_bytes);
     return -1;
   }
 
   fflush(stdout);
   return 0;
 }
 
-// If the dump is interrupted, resume the target process before exiting.
-static void exit_handler() {
-  kill(pid, SIGCONT);
-  waitpid(pid, NULL, 0);
-  exit(-1);
-}
-
-static bool freeze_process() {
-  if (kill(pid, SIGSTOP) != 0) {
-    fprintf(stderr, "Could not freeze the target process.\n");
-    perror("kill");
-    return false;
-  }
-
-  signal(SIGPIPE, exit_handler);
-  signal(SIGINT, exit_handler);
-  return true;
-}
-
 // Unfortunately lseek takes a *signed* offset, which is unsuitable for large
 // files like /proc/X/mem on 64-bit.
 static void lseek_abs(int fd, size_t off) {
 #define OFF_T_MAX ((off_t) ~(((uint64_t) 1) << (8 * sizeof(off_t) - 1)))
   if (off <= OFF_T_MAX) {
     lseek(fd, (off_t) off, SEEK_SET);
     return;
   }
   lseek(fd, (off_t) OFF_T_MAX, SEEK_SET);
   lseek(fd, (off_t) (off - OFF_T_MAX), SEEK_CUR);
@@ skipping 37 matching lines @@
     perror("read");
     length = 0;
   }
   close(cmdline_fd);
   cmdline[length] = '\0';
 }
 
 int main(int argc, char** argv) {
   char c;
   int ret = 0;
-  bool should_freeze_process = true;
   bool dump_also_allocs = false;
+  bool pedantic = true;
   char comment[1024] = { '\0' };
 
-  while (((c = getopt(argc, argv, "nxc:")) & 0x80) == 0) {
+  while (((c = getopt(argc, argv, "xnc:")) & 0x80) == 0) {

[pasko, 2014/09/10 12:09:18]

Please provide a short human-readable description of these individual options. I
think having them in this file's comments is OK, but I would prefer a --help
printout of course.

[Primiano Tucci (use gerrit), 2014/09/10 13:17:35]

OK. I'll expand the usage string.

    switch (c) {
-      case 'n':
-        should_freeze_process = false;
-        break;
       case 'x':
         dump_also_allocs = true;
         break;
+      case 'n':
+        pedantic = false;
+        break;
       case 'c':
         strlcpy(comment, optarg, sizeof(comment));
         break;
      }
   }
 
   if (optind >= argc) {
     printf("Usage: %s [-n] [-x] [-c comment] pid\n", argv[0]);
     return -1;
   }
 
   pid = atoi(argv[optind]);
 
-  if (should_freeze_process && !freeze_process())
+  if (ptrace(PTRACE_ATTACH, pid, NULL, NULL) == -1) {

[pasko, 2014/09/10 12:09:18]

ptrace interface is tricky and I am not a good enough expert here. I think it's
a good idea to ask eugenis@ to have a quick look at this, since I remember him
playing with ptrace extensively.

Will PTRACE_ATTACH continue to be allowed on L+? Some distributions require
PR_SET_PTRACER followed by PTRACE_TRACEME.

[Primiano Tucci (use gerrit), 2014/09/10 13:17:35]

Very hard to tell. However PTRACE_ATTACH is the way gdb --attach works, and I
hope Android will keep supporting gdb :)

Some distributions require
That is different and must be done by the traced process (child), not by the
tracer.

In general, AFAICT, as long as the tracer is root everything should be fine.
Adding euginins@ to doublecheck btw.

+    perror("ptrace");
     return -1;
+  }
 
   // Wait for the process to actually freeze.
   waitpid(pid, NULL, 0);
 
   int mem_fd = open_proc_mem_fd();
   if (mem_fd < 0)
     ret = -1;
 
   FILE* fmaps = open_proc_maps();
   if (fmaps == NULL)
     ret = -1;
 
   if (ret == 0)
-    ret = dump_process_heap(mem_fd, fmaps, dump_also_allocs, comment);
+    ret = dump_process_heap(mem_fd, fmaps, dump_also_allocs, pedantic, comment);
 
-  if (should_freeze_process)
-    kill(pid, SIGCONT);
+  ptrace(PTRACE_DETACH, pid, NULL, NULL);

[pasko, 2014/09/10 12:09:18]

ptrace manual page says:
"""
While being traced, the tracee will stop each time a signal is delivered, even
if the signal is being ignored.
"""

It seems the code as it is now ignores the signal:
"""
After signal-delivery-stop is observed by the tracer, the tracer
should restart the tracee with the call:
    ptrace(PTRACE_restart, pid, 0, sig)
If sig is 0, then a signal is not delivered.
"""

[pasko, 2014/09/10 12:45:53]

please disregard this comment, the quote from above should apply to the signals
that occurred while the process runs and is being traced. We have a simpler
case, the program is either traced or running.

(I am not 100% sure that I read the manual correctly though)

[Primiano Tucci (use gerrit), 2014/09/10 13:17:35]

That is for the case of fork + TRACEME. In the case of PTRACE_ATTACH, the tracee
is already stopped, so it's very hard it will be stopped more than that. :)
Which signal? The signal eventually goes to the tracee, not to this process.
Again, this is for the case of the tracee being alive and the tracer wanting to
be notified when the tracee gets a signal / exception. This is not the case
here.

 
   // Cleanup.
   fflush(stdout);
   close(mem_fd);
   fclose(fmaps);
   return ret;
 }