From d1e49bd221d8584b77566da52ffc293203360c3d Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Fri, 16 Oct 2015 14:55:14 -0700 Subject: [PATCH] Fix CRITICAL_SECTION test I thought I had confirmed that this still allocated and ignored the flag on older OSs, but I must have not had the PLOG active yet? I'm not sure what I did. (I might try to blame VMware as it has an annoying habit of caching old binaries when you use it's "Shared Folders" feature to point at the dev machine's build dir.) I confirmed that it does work on Win8 and Win10 but doesn't on Win XP and Win 7. R=mark@chromium.org BUG=crashpad:52 Review URL: https://codereview.chromium.org/1405243002 . --- client/crashpad_client_win.cc | 21 +---- handler/handler.gyp | 1 + handler/win/crashy_test_program.cc | 20 ++--- snapshot/win/end_to_end_test.py | 5 +- util/util.gyp | 2 + util/util_test.gyp | 1 + util/win/critical_section_with_debug_info.cc | 76 +++++++++++++++++++ util/win/critical_section_with_debug_info.h | 34 +++++++++ .../critical_section_with_debug_info_test.cc | 34 +++++++++ util/win/registration_protocol_win.h | 8 +- 10 files changed, 164 insertions(+), 38 deletions(-) create mode 100644 util/win/critical_section_with_debug_info.cc create mode 100644 util/win/critical_section_with_debug_info.h create mode 100644 util/win/critical_section_with_debug_info_test.cc diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc index 7a1e547a..3b3eaa6a 100644 --- a/client/crashpad_client_win.cc +++ b/client/crashpad_client_win.cc @@ -23,6 +23,7 @@ #include "base/strings/utf_string_conversions.h" #include "base/synchronization/lock.h" #include "util/file/file_io.h" +#include "util/win/critical_section_with_debug_info.h" #include "util/win/registration_protocol_win.h" #include "util/win/scoped_handle.h" @@ -101,18 +102,6 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) { return EXCEPTION_CONTINUE_SEARCH; } -BOOL CrashpadInitializeCriticalSectionEx( - CRITICAL_SECTION* critical_section, - DWORD spin_count, - DWORD flags) { - static decltype(InitializeCriticalSectionEx)* initialize_critical_section_ex = - reinterpret_cast(GetProcAddress( - LoadLibrary(L"kernel32.dll"), "InitializeCriticalSectionEx")); - if (!initialize_critical_section_ex) - return false; - return initialize_critical_section_ex(critical_section, spin_count, flags); -} - } // namespace namespace crashpad { @@ -156,14 +145,10 @@ bool CrashpadClient::SetHandler(const std::string& ipc_port) { // debugger would instead inspect ntdll!RtlCriticalSectionList to get the head // of the list. But that is not an exported symbol, so on an arbitrary client // machine, we don't have a way of getting that pointer. - if (CrashpadInitializeCriticalSectionEx( - &g_critical_section_with_debug_info, - 0, - RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO)) { + if (InitializeCriticalSectionWithDebugInfoIfPossible( + &g_critical_section_with_debug_info)) { message.registration.critical_section_address = reinterpret_cast(&g_critical_section_with_debug_info); - } else { - PLOG(ERROR) << "InitializeCriticalSectionEx"; } ServerToClientMessage response = {0}; diff --git a/handler/handler.gyp b/handler/handler.gyp index 8f8324c0..28bc9e35 100644 --- a/handler/handler.gyp +++ b/handler/handler.gyp @@ -82,6 +82,7 @@ '../client/client.gyp:crashpad_client', '../third_party/mini_chromium/mini_chromium.gyp:base', '../tools/tools.gyp:crashpad_tool_support', + '../util/util.gyp:crashpad_util', ], 'include_dirs': [ '..', diff --git a/handler/win/crashy_test_program.cc b/handler/win/crashy_test_program.cc index 0694a88b..4057df89 100644 --- a/handler/win/crashy_test_program.cc +++ b/handler/win/crashy_test_program.cc @@ -24,6 +24,7 @@ #include "base/logging.h" #include "client/crashpad_client.h" #include "tools/tool_support.h" +#include "util/win/critical_section_with_debug_info.h" namespace crashpad { namespace { @@ -77,18 +78,6 @@ void AllocateMemoryOfVariousProtections() { } } -BOOL CrashpadInitializeCriticalSectionEx( - CRITICAL_SECTION* critical_section, - DWORD spin_count, - DWORD flags) { - static decltype(InitializeCriticalSectionEx)* initialize_critical_section_ex = - reinterpret_cast(GetProcAddress( - LoadLibrary(L"kernel32.dll"), "InitializeCriticalSectionEx")); - if (!initialize_critical_section_ex) - return false; - return initialize_critical_section_ex(critical_section, spin_count, flags); -} - void SomeCrashyFunction() { // SetLastError and NTSTATUS so that we have something to view in !gle in // windbg. RtlNtStatusToDosError() stores STATUS_NO_SUCH_FILE into the @@ -117,9 +106,10 @@ int CrashyMain(int argc, char* argv[]) { AllocateMemoryOfVariousProtections(); - CrashpadInitializeCriticalSectionEx( - &g_test_critical_section, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO); - EnterCriticalSection(&g_test_critical_section); + if (InitializeCriticalSectionWithDebugInfoIfPossible( + &g_test_critical_section)) { + EnterCriticalSection(&g_test_critical_section); + } SomeCrashyFunction(); diff --git a/snapshot/win/end_to_end_test.py b/snapshot/win/end_to_end_test.py index 75dafde9..a09edd46 100644 --- a/snapshot/win/end_to_end_test.py +++ b/snapshot/win/end_to_end_test.py @@ -15,6 +15,7 @@ # limitations under the License. import os +import platform import random import re import subprocess @@ -182,7 +183,9 @@ def RunTests(cdb_path, dump_path, pipe_name): out = CdbRun(cdb_path, dump_path, '!locks') out.Check(r'CritSec crashy_program!crashpad::`anonymous namespace\'::' r'g_test_critical_section', 'lock was captured') - out.Check(r'\*\*\* Locked', 'lock debug info was captured, and is locked') + if float(platform.win32_ver()[0]) != 7: + # We can't allocate CRITICAL_SECTIONs with .DebugInfo on Win 7. + out.Check(r'\*\*\* Locked', 'lock debug info was captured, and is locked') def main(args): diff --git a/util/util.gyp b/util/util.gyp index 02e3b02c..26ed3d29 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -152,6 +152,8 @@ 'win/capture_context.asm', 'win/capture_context.h', 'win/checked_win_address_range.h', + 'win/critical_section_with_debug_info.cc', + 'win/critical_section_with_debug_info.h', 'win/exception_handler_server.cc', 'win/exception_handler_server.h', 'win/module_version.cc', diff --git a/util/util_test.gyp b/util/util_test.gyp index dc9ed60a..7de1ffcb 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -80,6 +80,7 @@ 'thread/thread_log_messages_test.cc', 'thread/thread_test.cc', 'win/capture_context_test.cc', + 'win/critical_section_with_debug_info_test.cc', 'win/exception_handler_server_test.cc', 'win/process_info_test.cc', 'win/scoped_process_suspend_test.cc', diff --git a/util/win/critical_section_with_debug_info.cc b/util/win/critical_section_with_debug_info.cc new file mode 100644 index 00000000..91387ae9 --- /dev/null +++ b/util/win/critical_section_with_debug_info.cc @@ -0,0 +1,76 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/win/critical_section_with_debug_info.h" + +#include "base/logging.h" + +namespace crashpad { + +namespace { + +BOOL CrashpadInitializeCriticalSectionEx( + CRITICAL_SECTION* critical_section, + DWORD spin_count, + DWORD flags) { + static decltype(InitializeCriticalSectionEx)* initialize_critical_section_ex = + reinterpret_cast(GetProcAddress( + LoadLibrary(L"kernel32.dll"), "InitializeCriticalSectionEx")); + if (!initialize_critical_section_ex) { + PLOG(ERROR) << "GetProcAddress"; + return false; + } + bool ret = + initialize_critical_section_ex(critical_section, spin_count, flags); + if (!ret) { + PLOG(ERROR) << "InitializeCriticalSectionEx"; + return false; + } + return true; +} + +} // namespace + +bool InitializeCriticalSectionWithDebugInfoIfPossible( + CRITICAL_SECTION* critical_section) { + // On XP and Vista, a plain initialization causes the CRITICAL_SECTION to be + // allocated with .DebugInfo. On 8 and above, we can pass an additional flag + // to InitializeCriticalSectionEx() to force the .DebugInfo on. Before Win 8, + // that flag causes InitializeCriticalSectionEx() to fail. So, for XP, Vista, + // and 7 we use InitializeCriticalSection(), and for 8 and above, + // InitializeCriticalSectionEx() with the additional flag. + // + // TODO(scottmg): Try to find a solution for Win 7. It's unclear how to force + // it on for Win 7, however the Loader Lock does have .DebugInfo so there may + // be a way to do it. The comments in winnt.h imply that perhaps it's passed + // to InitializeCriticalSectionAndSpinCount() as the top bits of the spin + // count, but that doesn't appear to work. For now, we initialize a valid + // CRITICAL_SECTION, but without .DebugInfo. + + const DWORD version = GetVersion(); + const DWORD major_version = LOBYTE(LOWORD(version)); + const DWORD minor_version = HIBYTE(LOWORD(version)); + const bool win7_or_lower = + major_version < 6 || (major_version == 6 && minor_version <= 1); + + if (win7_or_lower) { + InitializeCriticalSection(critical_section); + return true; + } + + return CrashpadInitializeCriticalSectionEx( + critical_section, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO); +} + +} // namespace crashpad diff --git a/util/win/critical_section_with_debug_info.h b/util/win/critical_section_with_debug_info.h new file mode 100644 index 00000000..12716857 --- /dev/null +++ b/util/win/critical_section_with_debug_info.h @@ -0,0 +1,34 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_UTIL_WIN_CRITICAL_SECTION_WITH_DEBUG_INFO_H_ +#define CRASHPAD_UTIL_WIN_CRITICAL_SECTION_WITH_DEBUG_INFO_H_ + +#include + +namespace crashpad { + +//! \brief Equivalent to `InitializeCritialSection()`, but attempts to allocate +//! with a valid `.DebugInfo` field on versions of Windows where it's +//! possible to do so. +//! +//! \return `true` on success, or `false` on failure with a message logged. +//! Success means that the critical section was successfully initialized, +//! but it does not necessarily have a valid `.DebugInfo` field. +bool InitializeCriticalSectionWithDebugInfoIfPossible( + CRITICAL_SECTION* critical_section); + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_WIN_CRITICAL_SECTION_WITH_DEBUG_INFO_H_ diff --git a/util/win/critical_section_with_debug_info_test.cc b/util/win/critical_section_with_debug_info_test.cc new file mode 100644 index 00000000..dcc59655 --- /dev/null +++ b/util/win/critical_section_with_debug_info_test.cc @@ -0,0 +1,34 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/win/critical_section_with_debug_info.h" + +#include "gtest/gtest.h" + +namespace crashpad { +namespace test { +namespace { + +TEST(CriticalSectionWithDebugInfo, CriticalSectionWithDebugInfo) { + CRITICAL_SECTION critical_section; + ASSERT_TRUE( + InitializeCriticalSectionWithDebugInfoIfPossible(&critical_section)); + EnterCriticalSection(&critical_section); + LeaveCriticalSection(&critical_section); + DeleteCriticalSection(&critical_section); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/win/registration_protocol_win.h b/util/win/registration_protocol_win.h index 9a7bd296..a691931d 100644 --- a/util/win/registration_protocol_win.h +++ b/util/win/registration_protocol_win.h @@ -61,10 +61,10 @@ struct RegistrationRequest { //! \brief The address, in the client process's address space, of a //! `CRITICAL_SECTION` allocated with a valid .DebugInfo field. This can - //! be accomplished by using the - //! `RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO` flag to - //! `InitializeCriticalSectionEx()`. This value can be `0`, however then - //! limited lock data will be available in minidumps. + //! be accomplished by using + //! InitializeCriticalSectionWithDebugInfoIfPossible() or equivalent. This + //! value can be `0`, however then limited lock data will be available in + //! minidumps. WinVMAddress critical_section_address; };