Improve filtering for false positive crashes...

chrome_frame/crash_reporting/vectored_handler-impl.h

Index: chrome_frame/crash_reporting/vectored_handler-impl.h
===================================================================
--- chrome_frame/crash_reporting/vectored_handler-impl.h	(revision 68282)
+++ chrome_frame/crash_reporting/vectored_handler-impl.h	(working copy)
@@ -83,9 +83,10 @@
     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,32 @@
                                       BackTrace, BackTraceHash);
   }
 
-  static bool ShouldIgnoreException(const EXCEPTION_POINTERS* exceptionInfo) {
+  static bool ShouldIgnoreException(const EXCEPTION_POINTERS* exceptionInfo,

[stoyan, 2010/12/09 09:58:23]

Hm.. if a module had installed SEH filter it does not mean it will handle
everything in it.

[amit, 2010/12/09 17:58:59]

True, there's a chance that the module's SEH filter will not handle the
exception. I guess it boils down to the following cases:
1. Crashing module is npchrome_frame.dll. If we have SEH installed, we will do
the right thing in the handler
2. Crashing module is x.dll and npchrome_frame.dll is not on stack. It's is very
unlikely that this is our fault.
3. Crashing module is x.dll and npchrome_frame.dll is on the stack. If x.dll is
has the top SEH filter, then it follows the expected exception pattern (very
often like urlmon!IsValidInterface). From the crashes, I haven't seen anything
indicating npchrome_frame.dll is at fault in this kind of scenario. Also, in
areas where we expect to have issues, we have SEH filters installed. 

So my sense is this fix will allow us to suppress a category of false positives
without too much risk of hiding true crashes.

+      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 = NULL;
+    ::GetModuleHandleEx(flags, reinterpret_cast<LPCWSTR>(address),

[stoyan, 2010/12/09 09:58:23]

Beware - IIRC GetModuleHandle obtains loader lock.

[amit, 2010/12/09 17:58:59]

Good point, changed it to use VirtualQuery

+                        &crashing_module);
+    HMODULE top_seh_module = NULL;
+    if (EXCEPTION_CHAIN_END != seh_record) {
+      ::GetModuleHandleEx(flags, reinterpret_cast<LPCWSTR>(seh_record->Handler),

[stoyan, 2010/12/09 09:58:23]

if crashing_module == NULL you don't have to call this, because of the check in
the next line.
I suspect GetModuleHandle installs SEH too, because the address passed can be
bogus i.e. seh_record->Handler could be anything; it may create some interesting
cases.

[amit, 2010/12/09 17:58:59]

Yupp, added the check for crashing_module and changed to VirtualQuery

+                          &top_seh_module);
     }
 
+    // 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 && (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 (crashing_module && nt_loader::OwnsLoaderLock()) {
       nt_loader::LDR_DATA_TABLE_ENTRY* entry =
           nt_loader::GetLoaderEntry(crashing_module);
 
@@ -225,44 +231,12 @@
     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];
 };
 
-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 },
-};
 
 // Use Win32 API; checks for single (current) module. Will call a specified
 // CrashHandlerTraits::DumpHandler when taking a dump.
@@ -279,7 +253,6 @@
             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