diff options
author | siggi@chromium.org <siggi@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-03-12 00:27:58 +0000 |
---|---|---|
committer | siggi@chromium.org <siggi@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-03-12 00:27:58 +0000 |
commit | ba31e56dc1ef1f29ddd890df9943dffac7853477 (patch) | |
tree | d8cc480a25c56064f54b2a11edb47a91fe01d8eb /chrome_frame/crash_reporting | |
parent | 98bea6510a3c12ce40ddc2190dadb32d726414bf (diff) | |
download | chromium_src-ba31e56dc1ef1f29ddd890df9943dffac7853477.zip chromium_src-ba31e56dc1ef1f29ddd890df9943dffac7853477.tar.gz chromium_src-ba31e56dc1ef1f29ddd890df9943dffac7853477.tar.bz2 |
Heuristically avoid reporting crashes during DLL loading.
Cleanup white space as requested by Robert in CL 882001.
BUG=31980
TEST=Unittests in this change.
Review URL: http://codereview.chromium.org/872004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@41367 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome_frame/crash_reporting')
-rw-r--r-- | chrome_frame/crash_reporting/crash_dll.h | 1 | ||||
-rw-r--r-- | chrome_frame/crash_reporting/nt_loader.h | 8 | ||||
-rw-r--r-- | chrome_frame/crash_reporting/nt_loader_unittest.cc | 41 | ||||
-rw-r--r-- | chrome_frame/crash_reporting/vectored_handler-impl.h | 33 | ||||
-rw-r--r-- | chrome_frame/crash_reporting/vectored_handler_unittest.cc | 52 |
5 files changed, 117 insertions, 18 deletions
diff --git a/chrome_frame/crash_reporting/crash_dll.h b/chrome_frame/crash_reporting/crash_dll.h index ee4fafd..ca38e15 100644 --- a/chrome_frame/crash_reporting/crash_dll.h +++ b/chrome_frame/crash_reporting/crash_dll.h @@ -6,6 +6,7 @@ // Set either of these environment variables to // crash at load or unload time, respectively. +static const wchar_t* kCrashDllName = L"crash_dll.dll"; static const wchar_t* kCrashOnLoadMode = L"CRASH_DLL_CRASH_ON_LOAD"; static const wchar_t* kCrashOnUnloadMode = L"CRASH_DLL_CRASH_ON_UNLOAD"; static const DWORD kCrashAddress = 0x42; diff --git a/chrome_frame/crash_reporting/nt_loader.h b/chrome_frame/crash_reporting/nt_loader.h index fc4efb1..c751215 100644 --- a/chrome_frame/crash_reporting/nt_loader.h +++ b/chrome_frame/crash_reporting/nt_loader.h @@ -128,13 +128,13 @@ typedef struct _LDR_DATA_TABLE_ENTRY { union { LIST_ENTRY HashLinks; // 0x03c struct { - void* SectionPointer; // 0x03c - ULONG CheckSum; // 0x040 + void* SectionPointer; // 0x03c + ULONG CheckSum; // 0x040 }; }; union { - ULONG TimeDateStamp; // 0x044 - void* LoadedImports; // 0x044 + ULONG TimeDateStamp; // 0x044 + void* LoadedImports; // 0x044 }; void *EntryPointActivationContext; // 0x048 void* PatchInformation; // 0x04c diff --git a/chrome_frame/crash_reporting/nt_loader_unittest.cc b/chrome_frame/crash_reporting/nt_loader_unittest.cc index 67392bd..45546f6 100644 --- a/chrome_frame/crash_reporting/nt_loader_unittest.cc +++ b/chrome_frame/crash_reporting/nt_loader_unittest.cc @@ -7,6 +7,7 @@ #include <winnt.h> #include <base/at_exit.h> #include <base/scoped_handle.h> +#include <base/string_util.h> #include <base/sys_info.h> #include <base/thread.h> #include "chrome_frame/crash_reporting/crash_dll.h" @@ -116,8 +117,6 @@ TEST(NtLoader, OwnsLoaderLock) { } TEST(NtLoader, GetLoaderEntry) { - ScopedEnterCriticalSection lock(GetLoaderLock()); - // Get all modules in the current process. ScopedHandle snap(::CreateToolhelp32Snapshot(TH32CS_SNAPMODULE, ::GetCurrentProcessId())); @@ -128,14 +127,44 @@ TEST(NtLoader, GetLoaderEntry) { MODULEENTRY32 module = { sizeof(module) }; ASSERT_TRUE(::Module32First(snap.Get(), &module)); do { + ScopedEnterCriticalSection lock(GetLoaderLock()); + nt_loader::LDR_DATA_TABLE_ENTRY* entry = nt_loader::GetLoaderEntry(module.hModule); ASSERT_TRUE(entry != NULL); - ASSERT_EQ(module.hModule, reinterpret_cast<HMODULE>(entry->DllBase)); - ASSERT_STREQ(module.szModule, + EXPECT_EQ(module.hModule, reinterpret_cast<HMODULE>(entry->DllBase)); + EXPECT_STREQ(module.szModule, FromUnicodeString(entry->BaseDllName).c_str()); - ASSERT_STREQ(module.szExePath, + EXPECT_STREQ(module.szExePath, FromUnicodeString(entry->FullDllName).c_str()); + + ULONG flags = entry->Flags; + + // All entries should have this flag set. + EXPECT_TRUE(flags & LDRP_ENTRY_PROCESSED); + + if (0 == (flags & LDRP_IMAGE_DLL)) { + // TODO(siggi): write a test to assert this holds true for loading + // non-DLL, e.g. exe image files. + // Dlls have the LDRP_IMAGE_DLL flag set, any module that doesn't + // have that flag has to be the main executable. + EXPECT_TRUE(module.hModule == ::GetModuleHandle(NULL)); + } else { + // Since we're not currently loading any modules, all loaded + // modules should either have the LDRP_PROCESS_ATTACH_CALLED, + // or a NULL entrypoint. + if (entry->EntryPoint == NULL) { + EXPECT_FALSE(flags & LDRP_PROCESS_ATTACH_CALLED); + } else { + // Shimeng.dll is an exception to the above, it's loaded + // in a special way, see e.g. http://www.alex-ionescu.com/?p=41 + // for details. + bool is_shimeng = LowerCaseEqualsASCII( + FromUnicodeString(entry->BaseDllName), "shimeng.dll"); + + EXPECT_TRUE(is_shimeng || (flags & LDRP_PROCESS_ATTACH_CALLED)); + } + } } while(::Module32Next(snap.Get(), &module)); } @@ -199,8 +228,6 @@ class NtLoaderTest: public testing::Test { NtLoaderTest* NtLoaderTest::current_ = NULL; DWORD NtLoaderTest::main_thread_ = ::GetCurrentThreadId(); -const wchar_t kCrashDllName[] = L"crash_dll.dll"; - } // namespace static int exceptions_handled = 0; diff --git a/chrome_frame/crash_reporting/vectored_handler-impl.h b/chrome_frame/crash_reporting/vectored_handler-impl.h index 9c0e97d..641522f 100644 --- a/chrome_frame/crash_reporting/vectored_handler-impl.h +++ b/chrome_frame/crash_reporting/vectored_handler-impl.h @@ -5,6 +5,7 @@ #ifndef CHROME_FRAME_CRASH_REPORTING_VECTORED_HANDLER_IMPL_H_ #define CHROME_FRAME_CRASH_REPORTING_VECTORED_HANDLER_IMPL_H_ #include "chrome_frame/crash_reporting/vectored_handler.h" +#include "chrome_frame/crash_reporting/nt_loader.h" #if defined(_M_IX86) #ifndef EXCEPTION_CHAIN_END @@ -98,7 +99,7 @@ LONG VectoredHandlerT<E>::Handler(EXCEPTION_POINTERS* exceptionInfo) { } // See whether our module is somewhere in the call stack. - void* back_trace[api_->max_back_trace] = {0}; + void* back_trace[E::max_back_trace] = {0}; DWORD captured = api_->RtlCaptureStackBackTrace(0, api_->max_back_trace, &back_trace[0], NULL); for (DWORD i = 0; i < captured; ++i) { @@ -139,6 +140,7 @@ namespace { ret } } + __declspec(naked) static char* GetStackTopLimit() { __asm { @@ -176,6 +178,35 @@ class Win32VEHTraits { } } + // 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)) { + 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 + // during crashes that occur during module loads, such as e.g. when + // the hook manager attempts to load buggy window hook DLLs. + if (entry && + (entry->Flags & LDRP_IMAGE_DLL) != 0 && + (entry->EntryPoint != NULL) && + (entry->Flags & LDRP_PROCESS_ATTACH_CALLED) == 0) + return true; + } + return false; } diff --git a/chrome_frame/crash_reporting/vectored_handler_unittest.cc b/chrome_frame/crash_reporting/vectored_handler_unittest.cc index d7fc4ac..d17847b 100644 --- a/chrome_frame/crash_reporting/vectored_handler_unittest.cc +++ b/chrome_frame/crash_reporting/vectored_handler_unittest.cc @@ -5,6 +5,8 @@ #include <atlbase.h> #include "base/logging.h" +#include "chrome_frame/crash_reporting/crash_dll.h" +#include "chrome_frame/crash_reporting/nt_loader.h" #include "chrome_frame/crash_reporting/vectored_handler-impl.h" #include "chrome_frame/crash_reporting/veh_test.h" #include "testing/gmock/include/gmock/gmock.h" @@ -45,8 +47,12 @@ class MockApi : public Win32VEHTraits, public ModuleOfInterest { }; }; // namespace - typedef VectoredHandlerT<MockApi> VectoredHandlerMock; +static VectoredHandlerMock* g_mock_veh = NULL; +static LONG WINAPI VEH(EXCEPTION_POINTERS* exptrs) { + return g_mock_veh->Handler(exptrs); +} + TEST(ChromeFrame, ExceptionReport) { MockApi api; VectoredHandlerMock veh(&api); @@ -122,6 +128,45 @@ TEST(ChromeFrame, ExceptionReport) { EXPECT_EQ(ExceptionContinueSearch, veh.Handler(&ExceptionInfo(STATUS_ACCESS_VIOLATION, ignore_address))); testing::Mock::VerifyAndClearExpectations(&api); + + // Exception in a loading module, we are on the stack. + // Make sure we don't report it. + api.SetSEH(no_seh); + api.SetStack(on_stack); + EXPECT_CALL(api, WriteDump(_)).Times(0); + + g_mock_veh = &veh; + void* id = ::AddVectoredExceptionHandler(FALSE, VEH); + + EXPECT_TRUE(::SetEnvironmentVariable(kCrashOnLoadMode, L"1")); + long exceptions_seen = veh.get_exceptions_seen(); + HMODULE module = ::LoadLibrary(kCrashDllName); + EXPECT_EQ(NULL, module); + + testing::Mock::VerifyAndClearExpectations(&api); + EXPECT_EQ(exceptions_seen + 1, veh.get_exceptions_seen()); + EXPECT_TRUE(::SetEnvironmentVariable(kCrashOnLoadMode, NULL)); + + // Exception in an unloading module, we are on the stack/ + // Make sure we report it. + EXPECT_TRUE(::SetEnvironmentVariable(kCrashOnUnloadMode, L"1")); + + module = ::LoadLibrary(kCrashDllName); + + api.SetSEH(no_seh); + api.SetStack(on_stack); + EXPECT_CALL(api, WriteDump(_)).Times(1); + EXPECT_TRUE(module != NULL); + exceptions_seen = veh.get_exceptions_seen(); + + // Crash on unloading. + ::FreeLibrary(module); + + EXPECT_EQ(exceptions_seen + 1, veh.get_exceptions_seen()); + testing::Mock::VerifyAndClearExpectations(&api); + + ::RemoveVectoredExceptionHandler(id); + g_mock_veh = NULL; } MATCHER_P(ExceptionCodeIs, code, "") { @@ -150,11 +195,6 @@ DWORD WINAPI CrashingThread(PVOID tmp) { return 0; } -static VectoredHandlerMock* g_mock_veh = NULL; -static LONG WINAPI VEH(EXCEPTION_POINTERS* exptrs) { - return g_mock_veh->Handler(exptrs); -} - TEST(ChromeFrame, TrickyStackOverflow) { MockApi api; VectoredHandlerMock veh(&api); |