Improve filtering for false positive crashes

...by inspecting if the module with the crash has the topmost handler
in the SEH chain.  If that's the case then the exception will very
likely be handled. This covers the case of IsBadReadPtr adn family
without any special code. It also lets us address IsValidInterface,
an internal helper in urlmon.

BUG=64348
TEST=covered by existing unit tests

Review URL: http://codereview.chromium.org/5622006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69066 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 42 insertions, 61 deletions

diff --git a/chrome_frame/crash_reporting/vectored_handler-impl.h b/chrome_frame/crash_reporting/vectored_handler-impl.h
index 175e496..277c34b 100644
--- a/chrome_frame/crash_reporting/vectored_handler-impl.h
+++ b/chrome_frame/crash_reporting/vectored_handler-impl.h
@@ -83,9 +83,10 @@ LONG VectoredHandlerT<E>::Handler(EXCEPTION_POINTERS* exceptionInfo) {
     return ExceptionContinueSearch;
   }
 
-  // Check whether exception address is inbetween
-  // [IsBadReadPtr, IsBadReadPtr + 0xXX]
-  if (api_->ShouldIgnoreException(exceptionInfo)) {
+  // Check whether exception will be handled by the module that cuased it.
+  // This should automatically handle the case of IsBadReadPtr and family.
+  const EXCEPTION_REGISTRATION_RECORD* seh = api_->RtlpGetExceptionList();
+  if (api_->ShouldIgnoreException(exceptionInfo, seh)) {
     return ExceptionContinueSearch;
   }
 
@@ -169,27 +170,31 @@ class Win32VEHTraits {
                                       BackTrace, BackTraceHash);
   }
 
-  static bool ShouldIgnoreException(const EXCEPTION_POINTERS* exceptionInfo) {
+  static bool ShouldIgnoreException(const EXCEPTION_POINTERS* exceptionInfo,
+      const EXCEPTION_REGISTRATION_RECORD* seh_record) {
     const void* address = exceptionInfo->ExceptionRecord->ExceptionAddress;
-    for (int i = 0; i < kIgnoreEntries; i++) {
-      const CodeBlock& code_block = IgnoreExceptions[i];
-      DCHECK(code_block.code) << "Win32VEHTraits::CodeBlocks not initialized!";
-      if ((CodeOffset(code_block.code, code_block.begin_offset) <= address) &&
-          (address < CodeOffset(code_block.code, code_block.end_offset))) {
-        return true;
-      }
-    }
+    const DWORD flags = GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT |
+        GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS;
+    HMODULE crashing_module = GetModuleHandleFromAddress(address);
+    if (!crashing_module)
+      return false;
+
+    HMODULE top_seh_module = NULL;
+    if (EXCEPTION_CHAIN_END != seh_record)
+      top_seh_module = GetModuleHandleFromAddress(seh_record->Handler);
+
+    // check if the crash address belongs in a module that's expecting
+    // and handling it. This should address cases like kernel32!IsBadXXX
+    // and urlmon!IsValidInterface
+    if (crashing_module == top_seh_module)
+      return true;
 
     // We don't want to report exceptions that occur during DLL loading,
     // as those are captured and ignored by the NT loader. If this thread
     // is holding the loader's lock, there's a possiblity that the crash
     // is occurring in a loading DLL, in which case we resolve the
     // crash address to a module and check on the module's status.
-    const DWORD flags = GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT |
-        GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS;
-    HMODULE crashing_module = NULL;
-    if (nt_loader::OwnsLoaderLock() && ::GetModuleHandleEx(
-            flags, reinterpret_cast<LPCWSTR>(address), &crashing_module)) {
+    if (nt_loader::OwnsLoaderLock()) {
       nt_loader::LDR_DATA_TABLE_ENTRY* entry =
           nt_loader::GetLoaderEntry(crashing_module);
 
@@ -213,7 +218,7 @@ class Win32VEHTraits {
   }
 
   static bool CheckForStackOverflow() {
-    MEMORY_BASIC_INFORMATION mi;
+    MEMORY_BASIC_INFORMATION mi = {0};
     const DWORD kPageSize = 0x1000;
     void* stack_top = GetStackTopLimit() - kPageSize;
     ::VirtualQuery(stack_top, &mi, sizeof(mi));
@@ -225,45 +230,23 @@ class Win32VEHTraits {
     return !(mi.Protect & PAGE_GUARD);
   }
 
-  static void InitializeIgnoredBlocks() {
-    // Initialize ignored exception list
-    for (int i = 0; i < kIgnoreEntries; i++) {
-      CodeBlock& code_block = IgnoreExceptions[i];
-      if (!code_block.code) {
-        HMODULE module = GetModuleHandleA(code_block.module);
-        DCHECK(module) << "GetModuleHandle error: " << GetLastError();
-        code_block.code = GetProcAddress(module, code_block.function);
-        DCHECK(code_block.code) << "GetProcAddress error: "<< GetLastError();
-      }
-    }
-  }
-
  private:
   static inline const void* CodeOffset(const void* code, int offset) {
     return reinterpret_cast<const char*>(code) + offset;
   }
 
-  // Block of code to be ignored for exceptions
-  struct CodeBlock {
-    char* module;
-    char* function;
-    int begin_offset;
-    int end_offset;
-    const void* code;
-  };
-
-  static const int kIgnoreEntries = 4;
-  static CodeBlock IgnoreExceptions[kIgnoreEntries];
-};
+  static HMODULE GetModuleHandleFromAddress(const void* p) {
+    HMODULE module_at_address = NULL;
+    MEMORY_BASIC_INFORMATION mi = {0};
+    if (::VirtualQuery(p, &mi, sizeof(mi)) && (mi.Type & MEM_IMAGE)) {
+      module_at_address = reinterpret_cast<HMODULE>(mi.AllocationBase);
+    }
 
-DECLSPEC_SELECTANY Win32VEHTraits::CodeBlock
-Win32VEHTraits::IgnoreExceptions[kIgnoreEntries] = {
-  { "kernel32.dll", "IsBadReadPtr", 0, 100, NULL },
-  { "kernel32.dll", "IsBadWritePtr", 0, 100, NULL },
-  { "kernel32.dll", "IsBadStringPtrA", 0, 100, NULL },
-  { "kernel32.dll", "IsBadStringPtrW", 0, 100, NULL },
+    return module_at_address;
+  }
 };
 
+
 // Use Win32 API; checks for single (current) module. Will call a specified
 // CrashHandlerTraits::DumpHandler when taking a dump.
 class CrashHandlerTraits : public Win32VEHTraits,
@@ -279,7 +262,6 @@ class CrashHandlerTraits : public Win32VEHTraits,
             DumpHandler dump_handler) {
     DCHECK(dump_handler);
     dump_handler_ = dump_handler;
-    Win32VEHTraits::InitializeIgnoredBlocks();
     ModuleOfInterestWithExcludedRegion::SetCurrentModule();
     // Pointers to static (non-extern) functions take the address of the
     // function's first byte, as opposed to an entry in the compiler generated
diff --git a/chrome_frame/crash_reporting/vectored_handler_unittest.cc b/chrome_frame/crash_reporting/vectored_handler_unittest.cc
index 7babde1..616745e 100644
--- a/chrome_frame/crash_reporting/vectored_handler_unittest.cc
+++ b/chrome_frame/crash_reporting/vectored_handler_unittest.cc
@@ -23,9 +23,7 @@ ACTION_P(StackTraceDump, s) {
 namespace {
 class MockApi : public Win32VEHTraits, public ModuleOfInterest {
  public:
-  MockApi() {
-    Win32VEHTraits::InitializeIgnoredBlocks();
-  }
+  MockApi() {}
 
   MOCK_METHOD1(WriteDump, void(const EXCEPTION_POINTERS*));
   MOCK_METHOD0(RtlpGetExceptionList, const EXCEPTION_REGISTRATION_RECORD*());
@@ -60,7 +58,7 @@ TEST(ChromeFrame, ExceptionReport) {
   VectoredHandlerMock veh(&api);
 
   // Start address of our module.
-  char* s = reinterpret_cast<char*>(0x30000000);
+  char* s = reinterpret_cast<char*>(&__ImageBase);
   char *e = s + 0x10000;
   api.SetModule(s, e);
 
@@ -84,6 +82,7 @@ TEST(ChromeFrame, ExceptionReport) {
   // RPC_E_DISCONNECTED (0x80010108) is "The object invoked has disconnected
   // from its clients", shall not be caught since it's a warning only.
   EXPECT_CALL(api, WriteDump(_)).Times(0);
+  EXPECT_CALL(api, RtlpGetExceptionList()).Times(0);
   EXPECT_EQ(ExceptionContinueSearch,
       veh.Handler(&ExceptionInfo(RPC_E_DISCONNECTED, our_code)));
   testing::Mock::VerifyAndClearExpectations(&api);
@@ -114,21 +113,22 @@ TEST(ChromeFrame, ExceptionReport) {
   testing::Mock::VerifyAndClearExpectations(&api);
 
   // Exception, in IsBadStringPtrA, we are on the stack.
-  api.SetSEH(no_seh);
+  char* is_bad_ptr = reinterpret_cast<char*>(GetProcAddress(
+      GetModuleHandleA("kernel32.dll"), "IsBadStringPtrA"));
+  SEHChain kernel32_seh(is_bad_ptr, is_bad_ptr + 0x100, NULL);
+  api.SetSEH(kernel32_seh);
   api.SetStack(on_stack);
   EXPECT_CALL(api, WriteDump(_)).Times(0);
-  char* ignore_address = reinterpret_cast<char*>(GetProcAddress(
-      GetModuleHandleA("kernel32.dll"), "IsBadStringPtrA")) + 10;
   EXPECT_EQ(ExceptionContinueSearch,
-      veh.Handler(&ExceptionInfo(STATUS_ACCESS_VIOLATION, ignore_address)));
+      veh.Handler(&ExceptionInfo(STATUS_ACCESS_VIOLATION, is_bad_ptr)));
   testing::Mock::VerifyAndClearExpectations(&api);
 
   // Exception, in IsBadStringPtrA, we are not on the stack.
-  api.SetSEH(no_seh);
+  api.SetSEH(kernel32_seh);
   api.SetStack(not_on_stack);
   EXPECT_CALL(api, WriteDump(_)).Times(0);
   EXPECT_EQ(ExceptionContinueSearch,
-      veh.Handler(&ExceptionInfo(STATUS_ACCESS_VIOLATION, ignore_address)));
+      veh.Handler(&ExceptionInfo(STATUS_ACCESS_VIOLATION, is_bad_ptr)));
   testing::Mock::VerifyAndClearExpectations(&api);
 
   // Exception in a loading module, we are on the stack.
diff --git a/chrome_frame/crash_reporting/veh_test.cc b/chrome_frame/crash_reporting/veh_test.cc
index bfdb788..ec868ef 100644
--- a/chrome_frame/crash_reporting/veh_test.cc
+++ b/chrome_frame/crash_reporting/veh_test.cc
@@ -53,7 +53,6 @@ class MockApi : public Win32VEHTraits,
                 public ModuleOfInterestWithExcludedRegion {
  public:
   MockApi() {
-    Win32VEHTraits::InitializeIgnoredBlocks();
     ModuleOfInterestWithExcludedRegion::SetModule(&ModuleStart, &ModuleEnd);
     ModuleOfInterestWithExcludedRegion::SetExcludedRegion(&Undetectable,
         &UndetectableEnd);