From 43813b76ce14f94a8475c39738d41dc4682faeb5 Mon Sep 17 00:00:00 2001 From: "amit@chromium.org" Date: Fri, 15 Jan 2010 20:59:13 +0000 Subject: Suppress crash dump generation due to IsBadXXX functions. IsBad[Read/Write]Ptr etc functions cause access violations which we catch and take crash dump if we are on stack. Adding code to suppress these false positives. BUG=none TEST=added new tests cases to ChromeFrame.ExceptionReport Review URL: http://codereview.chromium.org/536073 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@36403 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome_frame/crash_reporting/crash_report.cc | 47 ++--------- chrome_frame/crash_reporting/crash_report.h | 90 ++++++++++++++++++++++ chrome_frame/crash_reporting/crash_reporting.gyp | 1 + .../crash_reporting/vectored_handler-impl.h | 5 +- chrome_frame/crash_reporting/vectored_handler.h | 4 + .../crash_reporting/vectored_handler_unittest.cc | 24 +++++- 6 files changed, 127 insertions(+), 44 deletions(-) (limited to 'chrome_frame/crash_reporting') diff --git a/chrome_frame/crash_reporting/crash_report.cc b/chrome_frame/crash_reporting/crash_report.cc index 43d602d..2a1b88b 100644 --- a/chrome_frame/crash_reporting/crash_report.cc +++ b/chrome_frame/crash_reporting/crash_report.cc @@ -7,57 +7,20 @@ #include "chrome_frame/crash_reporting/crash_report.h" #include "base/basictypes.h" -#include "base/logging.h" #include "breakpad/src/client/windows/handler/exception_handler.h" #include "chrome_frame/crash_reporting/vectored_handler.h" -#include "chrome_frame/crash_reporting/vectored_handler-impl.h" -namespace { // TODO(joshia): factor out common code with chrome used for crash reporting const wchar_t kGoogleUpdatePipeName[] = L"\\\\.\\pipe\\GoogleCrashServices\\"; - google_breakpad::ExceptionHandler* g_breakpad = NULL; -__declspec(naked) -static EXCEPTION_REGISTRATION_RECORD* InternalRtlpGetExceptionList() { - __asm { - mov eax, fs:0 - ret - } -} -} // end of namespace - -// Class which methods simply forwards to Win32 API and uses breakpad to write -// a minidump. Used as template (external interface) of VectoredHandlerT. -class Win32VEHTraits : public VEHTraitsBase { - public: - static inline void* Register(PVECTORED_EXCEPTION_HANDLER func, - const void* module_start, const void* module_end) { - VEHTraitsBase::SetModule(module_start, module_end); - return ::AddVectoredExceptionHandler(1, func); - } - - static inline ULONG Unregister(void* handle) { - return ::RemoveVectoredExceptionHandler(handle); - } - - static inline bool WriteDump(EXCEPTION_POINTERS* p) { - return g_breakpad->WriteMinidumpForException(p); - } - - static inline EXCEPTION_REGISTRATION_RECORD* RtlpGetExceptionList() { - return InternalRtlpGetExceptionList(); - } - - static inline WORD RtlCaptureStackBackTrace(DWORD FramesToSkip, - DWORD FramesToCapture, void** BackTrace, DWORD* BackTraceHash) { - return ::RtlCaptureStackBackTrace(FramesToSkip, FramesToCapture, - BackTrace, BackTraceHash); - } +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 }, }; -extern "C" IMAGE_DOS_HEADER __ImageBase; - std::wstring GetCrashServerPipeName(const std::wstring& user_sid) { std::wstring pipe_name = kGoogleUpdatePipeName; pipe_name += user_sid; diff --git a/chrome_frame/crash_reporting/crash_report.h b/chrome_frame/crash_reporting/crash_report.h index e98fd05..829dce2 100644 --- a/chrome_frame/crash_reporting/crash_report.h +++ b/chrome_frame/crash_reporting/crash_report.h @@ -9,7 +9,12 @@ #include +#include "base/logging.h" #include "breakpad/src/client/windows/handler/exception_handler.h" +#include "chrome_frame/crash_reporting/vectored_handler-impl.h" + +extern google_breakpad::ExceptionHandler* g_breakpad; +extern "C" IMAGE_DOS_HEADER __ImageBase; bool InitializeVectoredCrashReporting( bool full_dump, @@ -25,4 +30,89 @@ bool InitializeVectoredCrashReportingWithPipeName( bool ShutdownVectoredCrashReporting(); +namespace { +__declspec(naked) +static EXCEPTION_REGISTRATION_RECORD* InternalRtlpGetExceptionList() { + __asm { + mov eax, fs:0 + ret + } +} +} // end of namespace + +// Class which methods simply forwards to Win32 API and uses breakpad to write +// a minidump. Used as template (external interface) of VectoredHandlerT. +class Win32VEHTraits : public VEHTraitsBase { + public: + static inline void* Register(PVECTORED_EXCEPTION_HANDLER func, + const void* module_start, const void* module_end) { + InitializeIgnoredBlocks(); + VEHTraitsBase::SetModule(module_start, module_end); + return ::AddVectoredExceptionHandler(1, func); + } + + static inline ULONG Unregister(void* handle) { + return ::RemoveVectoredExceptionHandler(handle); + } + + static inline bool WriteDump(EXCEPTION_POINTERS* p) { + return g_breakpad->WriteMinidumpForException(p); + } + + static inline 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) { + const void* address = exceptionInfo->ExceptionRecord->ExceptionAddress; + for (int i = 0; i < kIgnoreEntries; i++) { + const CodeBlock& code_block = IgnoreExceptions[i]; + if (code_block.code && + (CodeOffset(code_block.code, code_block.begin_offset) <= address) && + (address < CodeOffset(code_block.code, code_block.end_offset))) { + return true; + } + } + + return false; + } + + static inline 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(); + } + } + } + + static inline const void* CodeOffset(const void* code, int offset) { + return reinterpret_cast(code) + offset; + } + + private: + // 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]; +}; + + #endif // CHROME_FRAME_CRASH_REPORTING_CRASH_REPORT_H_ diff --git a/chrome_frame/crash_reporting/crash_reporting.gyp b/chrome_frame/crash_reporting/crash_reporting.gyp index 7d75184..6cd712c 100644 --- a/chrome_frame/crash_reporting/crash_reporting.gyp +++ b/chrome_frame/crash_reporting/crash_reporting.gyp @@ -42,6 +42,7 @@ '../../testing/gmock.gyp:gmock', '../../testing/gtest.gyp:gtest', '../../testing/gtest.gyp:gtestmain', + '../../breakpad/breakpad.gyp:breakpad_handler', ], }, ], diff --git a/chrome_frame/crash_reporting/vectored_handler-impl.h b/chrome_frame/crash_reporting/vectored_handler-impl.h index f4ae117..c6d7d8e 100644 --- a/chrome_frame/crash_reporting/vectored_handler-impl.h +++ b/chrome_frame/crash_reporting/vectored_handler-impl.h @@ -65,8 +65,11 @@ LONG WINAPI VectoredHandlerT::VectoredHandler( ++VectoredHandlerT::g_exceptions_seen; - // TODO(stoyan): Check whether exception address is inbetween + // Check whether exception address is inbetween // [IsBadReadPtr, IsBadReadPtr + 0xXX] + if (E::ShouldIgnoreException(exceptionInfo)) { + return ExceptionContinueSearch; + } const DWORD exceptionFlags = exceptionInfo->ExceptionRecord->ExceptionFlags; // I don't think VEH is called on unwind. Just to be safe. diff --git a/chrome_frame/crash_reporting/vectored_handler.h b/chrome_frame/crash_reporting/vectored_handler.h index f3e08b0..7447bba 100644 --- a/chrome_frame/crash_reporting/vectored_handler.h +++ b/chrome_frame/crash_reporting/vectored_handler.h @@ -77,6 +77,10 @@ class VEHTraitsBase { g_module_start = module_start; g_module_end = module_end; } + + static bool ShouldIgnoreException(const EXCEPTION_POINTERS* exceptionInfo) { + return false; + } }; DECLSPEC_SELECTANY const void* VEHTraitsBase::g_module_start; diff --git a/chrome_frame/crash_reporting/vectored_handler_unittest.cc b/chrome_frame/crash_reporting/vectored_handler_unittest.cc index 52c730a..0b87f04 100644 --- a/chrome_frame/crash_reporting/vectored_handler_unittest.cc +++ b/chrome_frame/crash_reporting/vectored_handler_unittest.cc @@ -6,11 +6,12 @@ #include "base/logging.h" #include "chrome_frame/crash_reporting/vectored_handler-impl.h" +#include "chrome_frame/crash_reporting/crash_report.h" #include "gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" // Class that mocks external call from VectoredHandlerT for testing purposes. -class EMock : public VEHTraitsBase { +class EMock : public Win32VEHTraits { public: static inline bool WriteDump(EXCEPTION_POINTERS* p) { g_dump_made = true; @@ -20,6 +21,7 @@ class EMock : public VEHTraitsBase { static inline void* Register(PVECTORED_EXCEPTION_HANDLER func, const void* module_start, const void* module_end) { + InitializeIgnoredBlocks(); VEHTraitsBase::SetModule(module_start, module_end); // Return some arbitrary number, expecting to get the same on Unregister() return reinterpret_cast(4); @@ -188,5 +190,25 @@ TEST(ChromeFrame, ExceptionReport) { EXPECT_TRUE(EMock::g_dump_made); EMock::g_dump_made = false; + // Exception, in IsBadStringPtrA, we are on the stack. + char* ignore_address = reinterpret_cast(GetProcAddress( + GetModuleHandleA("kernel32.dll"), "IsBadStringPtrA")) + 10; + ex.Set(STATUS_ACCESS_VIOLATION, ignore_address + 10, 0); + EMock::SetNoSEHFilter(); + EMock::SetOnStack(); + EXPECT_EQ(ExceptionContinueSearch, VectoredHandlerMock::VectoredHandler(&ex)); + EXPECT_EQ(5, VectoredHandlerMock::g_exceptions_seen); + EXPECT_FALSE(EMock::g_dump_made); + EMock::g_dump_made = false; + + // Exception, in IsBadStringPtrA, we are not in stack. + ex.Set(STATUS_ACCESS_VIOLATION, ignore_address + 10, 0); + EMock::SetNoSEHFilter(); + EMock::SetNotOnStack(); + EXPECT_EQ(ExceptionContinueSearch, VectoredHandlerMock::VectoredHandler(&ex)); + EXPECT_EQ(6, VectoredHandlerMock::g_exceptions_seen); + EXPECT_FALSE(EMock::g_dump_made); + EMock::g_dump_made = false; + VectoredHandlerMock::Unregister(); } -- cgit v1.1