Improve filtering for false positive crashes...

chrome_frame/crash_reporting/vectored_handler-impl.h

 // Copyright (c) 2009 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.
 
 #ifndef CHROME_FRAME_CRASH_REPORTING_VECTORED_HANDLER_IMPL_H_
 #define CHROME_FRAME_CRASH_REPORTING_VECTORED_HANDLER_IMPL_H_
 
 #include "base/logging.h"
 #include "chrome_frame/crash_reporting/vectored_handler.h"
 #include "chrome_frame/crash_reporting/nt_loader.h"
@@ skipping 65 matching lines @@
   exceptions_seen_++;
 
   // If the exception code is STATUS_STACK_OVERFLOW then proceed as usual -
   // we want to report it. Otherwise check whether guard page in stack is gone -
   // i.e. stack overflow has been already observed and most probably we are
   // seeing the follow-up STATUS_ACCESS_VIOLATION(s). See bug 32441.
   if (exceptionCode != STATUS_STACK_OVERFLOW && api_->CheckForStackOverflow()) {
     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;
   }
 
   const DWORD exceptionFlags = exceptionInfo->ExceptionRecord->ExceptionFlags;
   // I don't think VEH is called on unwind. Just to be safe.
   if (IS_DISPATCHING(exceptionFlags)) {
     if (ModuleHasInstalledSEHFilter())
       return ExceptionContinueSearch;
 
     if (api_->IsOurModule(exceptionInfo->ExceptionRecord->ExceptionAddress)) {
@@ skipping 63 matching lines @@
   EXCEPTION_REGISTRATION_RECORD* RtlpGetExceptionList() {
     return InternalRtlpGetExceptionList();
   }
 
   static inline WORD RtlCaptureStackBackTrace(DWORD FramesToSkip,
       DWORD FramesToCapture, void** BackTrace, DWORD* BackTraceHash) {
     return ::RtlCaptureStackBackTrace(FramesToSkip, FramesToCapture,
                                       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);
 
       // If:
       //  1. we found the entry in question, and
       //  2. the entry is a DLL (has the IMAGE_DLL flag set), and
       //  3. the DLL has a non-null entrypoint, and
       //  4. the loader has not tagged it with the process attached called flag
       // we conclude that the crash is most likely during the loading of the
       // module and bail on reporting the crash to avoid false positives
@@ skipping 15 matching lines @@
     void* stack_top = GetStackTopLimit() - kPageSize;
     ::VirtualQuery(stack_top, &mi, sizeof(mi));
     // The above call may result in moving the top of the stack.
     // Check once more.
     void* stack_top2 = GetStackTopLimit() - kPageSize;
     if (stack_top2 != stack_top)
       ::VirtualQuery(stack_top2, &mi, sizeof(mi));
     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.
 class CrashHandlerTraits : public Win32VEHTraits,
                            public ModuleOfInterestWithExcludedRegion {
  public:
 
   typedef bool (*DumpHandler)(EXCEPTION_POINTERS* p);
 
   CrashHandlerTraits() : dump_handler_(NULL) {}
 
   // Note that breakpad_lock must be held when this is called.
   void Init(const void* veh_segment_start, const void* veh_segment_end,
             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
     // JMP table. In release builds /OPT:REF wipes away the JMP table, but debug
     // builds are not so lucky.
     ModuleOfInterestWithExcludedRegion::SetExcludedRegion(veh_segment_start,
                                                           veh_segment_end);
   }
 
   void Shutdown() {
   }
 
   inline bool WriteDump(EXCEPTION_POINTERS* p) {
     if (dump_handler_) {
       return dump_handler_(p);
     } else {
       return false;
     }
   }
 
  private:
   DumpHandler dump_handler_;
 };
 
 #endif  // CHROME_FRAME_CRASH_REPORTING_VECTORED_HANDLER_IMPL_H_