Handle failures of copying process data from a core file.

When LinuxCoreDumper fails to copy process data from a core file, it
fills the return buffer with a repeated sequence of a special marker.
However, MinidumpWriter doesn't know about that and may incorrectly
interpret the data. In many cases, MinidumpWriter simply copies the
gibberish data to the minidump, which isn't too bad. However, the
gibberish data may cause MinidumpWriter to behave badly in some other
cases. For example, when MinidumpWriter tries to iterate through the
linked list of all loaded DSOs via the r_map field of a r_debug struct,
if the linked list is filed with the special marker, the code keeps
iterating through the same address.

This CL addresses the issue by having LinuxCoreDumper::CopyFromProcess()
returns a Boolean value to indicate if the expected data is found from
the core file. MinidumpWriter can then decide how to handle that.

BUG=chromium:453484
TEST=Run core2md with the test data attached to chromium:453484.
R=mark@chromium.org

Review URL: https://breakpad.appspot.com/4724002

git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@1420 4c0a9323-5329-0410-9bdc-e9ce6186880e

6 files changed, 31 insertions, 16 deletions

diff --git a/src/client/linux/minidump_writer/linux_core_dumper.cc b/src/client/linux/minidump_writer/linux_core_dumper.cc
index 3eab44db..b8d90af6 100644
--- a/src/client/linux/minidump_writer/linux_core_dumper.cc
+++ b/src/client/linux/minidump_writer/linux_core_dumper.cc
@@ -74,7 +74,7 @@ bool LinuxCoreDumper::BuildProcPath(char* path, pid_t pid,
   return true;
 }
 
-void LinuxCoreDumper::CopyFromProcess(void* dest, pid_t child,
+bool LinuxCoreDumper::CopyFromProcess(void* dest, pid_t child,
                                       const void* src, size_t length) {
   ElfCoreDump::Addr virtual_address = reinterpret_cast<ElfCoreDump::Addr>(src);
   // TODO(benchan): Investigate whether the data to be copied could span
@@ -84,7 +84,9 @@ void LinuxCoreDumper::CopyFromProcess(void* dest, pid_t child,
     // If the data segment is not found in the core dump, fill the result
     // with marker characters.
     memset(dest, 0xab, length);
+    return false;
   }
+  return true;
 }
 
 bool LinuxCoreDumper::GetThreadInfoByIndex(size_t index, ThreadInfo* info) {
diff --git a/src/client/linux/minidump_writer/linux_core_dumper.h b/src/client/linux/minidump_writer/linux_core_dumper.h
index edb9e738..8537896e 100644
--- a/src/client/linux/minidump_writer/linux_core_dumper.h
+++ b/src/client/linux/minidump_writer/linux_core_dumper.h
@@ -68,8 +68,9 @@ class LinuxCoreDumper : public LinuxDumper {
   // Copies content of |length| bytes from a given process |child|,
   // starting from |src|, into |dest|. This method extracts the content
   // the core dump and fills |dest| with a sequence of marker bytes
-  // if the expected data is not found in the core dump.
-  virtual void CopyFromProcess(void* dest, pid_t child, const void* src,
+  // if the expected data is not found in the core dump. Returns true if
+  // the expected data is found in the core dump.
+  virtual bool CopyFromProcess(void* dest, pid_t child, const void* src,
                                size_t length);
 
   // Implements LinuxDumper::GetThreadInfoByIndex().
diff --git a/src/client/linux/minidump_writer/linux_dumper.h b/src/client/linux/minidump_writer/linux_dumper.h
index 40678e9c..d4310d7e 100644
--- a/src/client/linux/minidump_writer/linux_dumper.h
+++ b/src/client/linux/minidump_writer/linux_dumper.h
@@ -100,8 +100,8 @@ class LinuxDumper {
   PageAllocator* allocator() { return &allocator_; }
 
   // Copy content of |length| bytes from a given process |child|,
-  // starting from |src|, into |dest|.
-  virtual void CopyFromProcess(void* dest, pid_t child, const void* src,
+  // starting from |src|, into |dest|. Returns true on success.
+  virtual bool CopyFromProcess(void* dest, pid_t child, const void* src,
                                size_t length) = 0;
 
   // Builds a proc path for a certain pid for a node (/proc/<pid>/<node>).
diff --git a/src/client/linux/minidump_writer/linux_ptrace_dumper.cc b/src/client/linux/minidump_writer/linux_ptrace_dumper.cc
index 95f12942..7a2cfb9f 100644
--- a/src/client/linux/minidump_writer/linux_ptrace_dumper.cc
+++ b/src/client/linux/minidump_writer/linux_ptrace_dumper.cc
@@ -130,7 +130,7 @@ bool LinuxPtraceDumper::BuildProcPath(char* path, pid_t pid,
   return true;
 }
 
-void LinuxPtraceDumper::CopyFromProcess(void* dest, pid_t child,
+bool LinuxPtraceDumper::CopyFromProcess(void* dest, pid_t child,
                                         const void* src, size_t length) {
   unsigned long tmp = 55;
   size_t done = 0;
@@ -146,6 +146,7 @@ void LinuxPtraceDumper::CopyFromProcess(void* dest, pid_t child,
     my_memcpy(local + done, &tmp, l);
     done += l;
   }
+  return true;
 }
 
 // Read thread info from /proc/$pid/status.
diff --git a/src/client/linux/minidump_writer/linux_ptrace_dumper.h b/src/client/linux/minidump_writer/linux_ptrace_dumper.h
index 1e9bcfdf..2ce834b0 100644
--- a/src/client/linux/minidump_writer/linux_ptrace_dumper.h
+++ b/src/client/linux/minidump_writer/linux_ptrace_dumper.h
@@ -55,8 +55,8 @@ class LinuxPtraceDumper : public LinuxDumper {
   // Implements LinuxDumper::CopyFromProcess().
   // Copies content of |length| bytes from a given process |child|,
   // starting from |src|, into |dest|. This method uses ptrace to extract
-  // the content from the target process.
-  virtual void CopyFromProcess(void* dest, pid_t child, const void* src,
+  // the content from the target process. Always returns true.
+  virtual bool CopyFromProcess(void* dest, pid_t child, const void* src,
                                size_t length);
 
   // Implements LinuxDumper::GetThreadInfoByIndex().
diff --git a/src/client/linux/minidump_writer/minidump_writer.cc b/src/client/linux/minidump_writer/minidump_writer.cc
index 91610c03..edc7e47d 100644
--- a/src/client/linux/minidump_writer/minidump_writer.cc
+++ b/src/client/linux/minidump_writer/minidump_writer.cc
@@ -658,7 +658,9 @@ class MinidumpWriter {
     ElfW(Addr) dyn_addr = 0;
     for (; phnum >= 0; phnum--, phdr++) {
       ElfW(Phdr) ph;
-      dumper_->CopyFromProcess(&ph, GetCrashThread(), phdr, sizeof(ph));
+      if (!dumper_->CopyFromProcess(&ph, GetCrashThread(), phdr, sizeof(ph)))
+        return false;
+
       // Adjust base address with the virtual address of the PT_LOAD segment
       // corresponding to offset 0
       if (ph.p_type == PT_LOAD && ph.p_offset == 0) {
@@ -679,11 +681,14 @@ class MinidumpWriter {
     struct r_debug* r_debug = NULL;
     uint32_t dynamic_length = 0;
 
-    for (int i = 0;;) {
+    for (int i = 0; ; ++i) {
       ElfW(Dyn) dyn;
       dynamic_length += sizeof(dyn);
-      dumper_->CopyFromProcess(&dyn, GetCrashThread(), dynamic+i++,
-                               sizeof(dyn));
+      if (!dumper_->CopyFromProcess(&dyn, GetCrashThread(), dynamic + i,
+                                    sizeof(dyn))) {
+        return false;
+      }
+
       if (dyn.d_tag == DT_DEBUG) {
         r_debug = reinterpret_cast<struct r_debug*>(dyn.d_un.d_ptr);
         continue;
@@ -703,11 +708,15 @@ class MinidumpWriter {
     // Count the number of loaded DSOs
     int dso_count = 0;
     struct r_debug debug_entry;
-    dumper_->CopyFromProcess(&debug_entry, GetCrashThread(), r_debug,
-                             sizeof(debug_entry));
+    if (!dumper_->CopyFromProcess(&debug_entry, GetCrashThread(), r_debug,
+                                  sizeof(debug_entry))) {
+      return false;
+    }
     for (struct link_map* ptr = debug_entry.r_map; ptr; ) {
       struct link_map map;
-      dumper_->CopyFromProcess(&map, GetCrashThread(), ptr, sizeof(map));
+      if (!dumper_->CopyFromProcess(&map, GetCrashThread(), ptr, sizeof(map)))
+        return false;
+
       ptr = map.l_next;
       dso_count++;
     }
@@ -725,7 +734,9 @@ class MinidumpWriter {
       // Iterate over DSOs and write their information to mini dump
       for (struct link_map* ptr = debug_entry.r_map; ptr; ) {
         struct link_map map;
-        dumper_->CopyFromProcess(&map, GetCrashThread(), ptr, sizeof(map));
+        if (!dumper_->CopyFromProcess(&map, GetCrashThread(), ptr, sizeof(map)))
+          return  false;
+
         ptr = map.l_next;
         char filename[257] = { 0 };
         if (map.l_name) {