win: Validate readability of memory ranges added to minidump

R=mark@chromium.org
BUG=crashpad:59

Review URL: https://codereview.chromium.org/1412243005 .

handler/handler.gyp

@@ -91,6 +91,24 @@
             'win/crashy_test_program.cc',
           ],
         },
+        {
+          'target_name': 'self_destroying_program',
+          'type': 'executable',
+          'dependencies': [
+            '../client/client.gyp:crashpad_client',
+            '../compat/compat.gyp:crashpad_compat',
+            '../snapshot/snapshot.gyp:crashpad_snapshot',
+            '../third_party/mini_chromium/mini_chromium.gyp:base',
+            '../tools/tools.gyp:crashpad_tool_support',
+            '../util/util.gyp:crashpad_util',
+          ],
+          'include_dirs': [
+            '..',
+          ],
+          'sources': [
+            'win/self_destroying_test_program.cc',
+          ],
+        },
       ],
     }, {
       'targets': [],

handler/win/self_destroying_test_program.cc

@@ -0,0 +1,97 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <malloc.h>
+#include <stdlib.h>
+#include <windows.h>
+#include <winternl.h>
+
+#include "base/logging.h"
+#include "base/strings/stringprintf.h"
+#include "client/crashpad_client.h"
+#include "snapshot/win/process_reader_win.h"
+#include "tools/tool_support.h"
+
+namespace crashpad {
+namespace {
+
+// We VirtualFree a region in ourselves (the stack) to confirm that the
+// exception reporter captures as much as possible in the minidump and doesn't
+// abort. __debugbreak() immediately after doing so because the process is
+// clearly in a very broken state at this point.
+bool FreeOwnStackAndBreak() {
+  ProcessReaderWin process_reader;
+  if (!process_reader.Initialize(GetCurrentProcess(),
+                                 ProcessSuspensionState::kRunning)) {
+    LOG(ERROR) << "ProcessReaderWin Initialize";
+    return false;
+  }
+
+  const std::vector<ProcessReaderWin::Thread> threads =
+      process_reader.Threads();
+  if (threads.empty()) {
+    LOG(ERROR) << "no threads";
+    return false;
+  }
+
+  // Push the stack up a bit so that hopefully the crash handler can succeed,
+  // but won't be able to read the base of the stack.
+  _alloca(16384);
+
+  // We can't succeed at MEM_RELEASEing this memory, but MEM_DECOMMIT is good
+  // enough to make it inaccessible.
+  if (!VirtualFree(reinterpret_cast<void*>(threads[0].stack_region_address),
+                   100,
+                   MEM_DECOMMIT)) {
+    PLOG(ERROR) << "VirtualFree";
+    return false;
+  }
+
+  // If the VirtualFree() succeeds, we may have already crashed. __debugbreak()
+  // just to be sure.
+  __debugbreak();
+
+  return true;
+}
+
+int SelfDestroyingMain(int argc, char* argv[]) {
+  if (argc != 2) {
+    fprintf(stderr, "Usage: %s <server_pipe_name>\n", argv[0]);
+    return EXIT_FAILURE;
+  }
+
+  CrashpadClient client;
+  if (!client.SetHandler(argv[1])) {
+    LOG(ERROR) << "SetHandler";
+    return EXIT_FAILURE;
+  }
+  if (!client.UseHandler()) {
+    LOG(ERROR) << "UseHandler";
+    return EXIT_FAILURE;
+  }
+
+  if (!FreeOwnStackAndBreak())
+    return EXIT_FAILURE;
+
+  // This will never be reached. On success, we'll have crashed above, or
+  // otherwise returned before here.
+  return EXIT_SUCCESS;
+}
+
+}  // namespace
+}  // namespace crashpad
+
+int wmain(int argc, wchar_t* argv[]) {
+  return crashpad::ToolSupport::Wmain(argc, argv, crashpad::SelfDestroyingMain);
+}

snapshot/win/end_to_end_test.py

@@ -76,8 +76,8 @@ def GetCdbPath():
   return None
 
 
-def GetDumpFromCrashyProgram(out_dir, pipe_name):
+def GetDumpFromProgram(out_dir, pipe_name, executable_name):
-  """Initialize a crash database, run crashpad_handler, run crashy_program
+  """Initialize a crash database, run crashpad_handler, run |executable_name|
   connecting to the crash_handler. Returns the minidump generated by
   crash_handler for further testing.
   """
@@ -97,7 +97,7 @@ def GetDumpFromCrashyProgram(out_dir, pipe_name):
         '--database=' + test_database
     ])
 
-    subprocess.call([os.path.join(out_dir, 'crashy_program.exe'), pipe_name])
+    subprocess.call([os.path.join(out_dir, executable_name), pipe_name])
 
     out = subprocess.check_output([
         os.path.join(out_dir, 'crashpad_database_util.exe'),
@@ -113,6 +113,14 @@ def GetDumpFromCrashyProgram(out_dir, pipe_name):
       handler.kill()
 
 
+def GetDumpFromCrashyProgram(out_dir, pipe_name):
+  return GetDumpFromProgram(out_dir, pipe_name, 'crashy_program.exe')
+
+
+def GetDumpFromSelfDestroyingProgram(out_dir, pipe_name):
+  return GetDumpFromProgram(out_dir, pipe_name, 'self_destroying_program.exe')
+
+
 class CdbRun(object):
   """Run cdb.exe passing it a cdb command and capturing the output.
   `Check()` searches for regex patterns in sequence allowing verification of
@@ -147,7 +155,7 @@ class CdbRun(object):
       sys.exit(1)
 
 
-def RunTests(cdb_path, dump_path, pipe_name):
+def RunTests(cdb_path, dump_path, destroyed_dump_path, pipe_name):
   """Runs various tests in sequence. Runs a new cdb instance on the dump for
   each block of tests to reduce the chances that output from one command is
   confused for output from another.
@@ -192,6 +200,18 @@ def RunTests(cdb_path, dump_path, pipe_name):
   out.Check(r'Event\s+\d+', 'capture some event handles')
   out.Check(r'File\s+\d+', 'capture some file handles')
 
+  out = CdbRun(cdb_path, destroyed_dump_path, '.ecxr;!peb;k 2')
+  out.Check(r'Ldr\.InMemoryOrderModuleList:.*\d+ \. \d+', 'PEB_LDR_DATA saved')
+  out.Check(r'ntdll\.dll', 'ntdll present', re.IGNORECASE)
+
+  # Check that there is no stack trace in the self-destroyed process. Confirm
+  # that the top is where we expect it (that's based only on IP), but subsequent
+  # stack entries will not be available. This confirms that we have a mostly
+  # valid dump, but that the stack was omitted.
+  out.Check(r'self_destroying_program!crashpad::`anonymous namespace\'::'
+                r'FreeOwnStackAndBreak.*\nquit:',
+            'at correct location, no additional stack entries')
+
 def main(args):
   try:
     if len(args) != 1:
@@ -214,11 +234,15 @@ def main(args):
     pipe_name = r'\\.\pipe\end-to-end_%s_%s' % (
         os.getpid(), str(random.getrandbits(64)))
 
-    dump_path = GetDumpFromCrashyProgram(args[0], pipe_name)
+    crashy_dump_path = GetDumpFromCrashyProgram(args[0], pipe_name)
-    if not dump_path:
+    if not crashy_dump_path:
       return 1
 
-    RunTests(cdb_path, dump_path, pipe_name)
+    destroyed_dump_path = GetDumpFromSelfDestroyingProgram(args[0], pipe_name)
+    if not destroyed_dump_path:
+      return 1
+
+    RunTests(cdb_path, crashy_dump_path, destroyed_dump_path, pipe_name)
 
     return 0
   finally:

snapshot/win/process_snapshot_win.cc

@@ -344,19 +344,8 @@ void ProcessSnapshotWin::AddMemorySnapshot(
   if (size == 0)
     return;
 
-  // Ensure that the entire range is readable. TODO(scottmg): Consider
+  if (!process_reader_.GetProcessInfo().LoggingRangeIsFullyReadable(
-  // generalizing this as part of
+          CheckedRange<WinVMAddress, WinVMSize>(address, size))) {
-  // https://code.google.com/p/crashpad/issues/detail?id=59.
-  auto ranges = process_reader_.GetProcessInfo().GetReadableRanges(
-      CheckedRange<WinVMAddress, WinVMSize>(address, size));
-  if (ranges.size() != 1) {
-    LOG(ERROR) << base::StringPrintf(
-        "range at 0x%llx, size 0x%llx fully unreadable", address, size);
-    return;
-  }
-  if (ranges[0].base() != address || ranges[0].size() != size) {
-    LOG(ERROR) << base::StringPrintf(
-        "some of range at 0x%llx, size 0x%llx unreadable", address, size);
     return;
   }
 

snapshot/win/thread_snapshot_win.cc

@@ -41,11 +41,23 @@ bool ThreadSnapshotWin::Initialize(
   INITIALIZATION_STATE_SET_INITIALIZING(initialized_);
 
   thread_ = process_reader_thread;
-  // TODO(scottmg): Ensure these regions are readable
+  if (process_reader->GetProcessInfo().LoggingRangeIsFullyReadable(
-  // https://code.google.com/p/crashpad/issues/detail?id=59
+          CheckedRange<WinVMAddress, WinVMSize>(thread_.stack_region_address,
-  stack_.Initialize(
+                                                thread_.stack_region_size))) {
-      process_reader, thread_.stack_region_address, thread_.stack_region_size);
+    stack_.Initialize(process_reader,
+                      thread_.stack_region_address,
+                      thread_.stack_region_size);
+  } else {
+    stack_.Initialize(process_reader, 0, 0);
+  }
+
+  if (process_reader->GetProcessInfo().LoggingRangeIsFullyReadable(
+          CheckedRange<WinVMAddress, WinVMSize>(thread_.teb_address,
+                                                thread_.teb_size))) {
     teb_.Initialize(process_reader, thread_.teb_address, thread_.teb_size);
+  } else {
+    teb_.Initialize(process_reader, 0, 0);
+  }
 
 #if defined(ARCH_CPU_X86_64)
   if (process_reader->Is64Bit()) {

util/win/process_info.cc

@@ -600,6 +600,26 @@ ProcessInfo::GetReadableRanges(
   return GetReadableRangesOfMemoryMap(range, MemoryInfo());
 }
 
+bool ProcessInfo::LoggingRangeIsFullyReadable(
+    const CheckedRange<WinVMAddress, WinVMSize>& range) const {
+  const auto ranges = GetReadableRanges(range);
+  if (ranges.size() != 1) {
+    LOG(ERROR) << base::StringPrintf(
+        "range at 0x%llx, size 0x%llx fully unreadable",
+        range.base(),
+        range.size());
+    return false;
+  }
+  if (ranges[0].base() != range.base() || ranges[0].size() != range.size()) {
+    LOG(ERROR) << base::StringPrintf(
+        "some of range at 0x%llx, size 0x%llx unreadable",
+        range.base(),
+        range.size());
+    return false;
+  }
+  return true;
+}
+
 const std::vector<ProcessInfo::Handle>& ProcessInfo::Handles() const {
   INITIALIZATION_STATE_DCHECK_VALID(initialized_);
   if (handles_.empty())

util/win/process_info.h

@@ -139,6 +139,16 @@ class ProcessInfo {
   std::vector<CheckedRange<WinVMAddress, WinVMSize>> GetReadableRanges(
       const CheckedRange<WinVMAddress, WinVMSize>& range) const;
 
+  //! \brief Given a range in the target process, determines if the entire range
+  //!     is readable.
+  //!
+  //! \param[in] range The range being inspected.
+  //!
+  //! \return `true` if the range is fully readable, otherwise `false` with a
+  //!     message logged.
+  bool LoggingRangeIsFullyReadable(
+      const CheckedRange<WinVMAddress, WinVMSize>& range) const;
+
   //! \brief Retrieves information about open handles in the target process.
   const std::vector<Handle>& Handles() const;
 

util/win/process_info_test.cc

@@ -623,6 +623,20 @@ TEST(ProcessInfo, Handles) {
   EXPECT_TRUE(found_mapping_handle);
 }
 
+TEST(ProcessInfo, OutOfRangeCheck) {
+  const size_t kAllocationSize = 12345;
+  scoped_ptr<char[]> safe_memory(new char[kAllocationSize]);
+
+  ProcessInfo info;
+  info.Initialize(GetCurrentProcess());
+
+  EXPECT_TRUE(
+      info.LoggingRangeIsFullyReadable(CheckedRange<WinVMAddress, WinVMSize>(
+          reinterpret_cast<WinVMAddress>(safe_memory.get()), kAllocationSize)));
+  EXPECT_FALSE(info.LoggingRangeIsFullyReadable(
+      CheckedRange<WinVMAddress, WinVMSize>(0, 1024)));
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace crashpad