Skip to content

GitLab

T
tangled
Commit cf244d18 authored by tonyg's avatar tonyg Committed by Commit bot
Browse files
Options

Revert of Improve the ScopedHandle verifier. (patchset #1 of... 

Revert of Improve the ScopedHandle verifier. (patchset #1 of https://codereview.chromium.org/506013004/)

Reason for revert:
All windows perf bots (official builds) crashing.

http://build.chromium.org/p/chromium.perf/builders/Win%20Builder/builds/73062/steps/generate_telemetry_profiles/logs/stdio

	ChildEBP RetAddr
	0022f7c0 6a3ffa19 chrome_69c90000!base::debug::BreakDebugger+0x10
	0022f830 6a400622 chrome_69c90000!CheckIsChromeSxSProcess+0x26
	0022f834 6a400d52 chrome_69c90000!InstallUtil::IsChromeSxSProcess+0x16
	0022f83c 6a400c29 chrome_69c90000!BrowserDistribution::GetSpecificDistribution+0x3b
	0022f844 6a3fe166 chrome_69c90000!BrowserDistribution::GetDistribution+0x7
	0022f930 6a3fe11d chrome_69c90000!`anonymous namespace'::GetChromeChannelInternal+0x2c
	0022f948 6a092c6c chrome_69c90000!GoogleUpdateSettings::GetChromeChannel+0x21
	0022f9b4 6a08f70f chrome_69c90000!chrome::VersionInfo::GetChannel+0x60
	0022f9b8 6a08f65a chrome_69c90000!`anonymous namespace'::UseHooks+0x5
	0022f9dc 6a08eb5a chrome_69c90000!InstallCloseHandleHooks+0x15
	0022fa20 00fb7623 chrome_69c90000!ChromeMain+0x3e
	0022fab0 00fb7026 chrome!MainDllLoader::Launch+0x15f
	0022faf4 00fd936a chrome!wWinMain+0x5a
	0022fb40 766e338a chrome!__tmainCRTStartup+0xfd
	WARNING: Stack unwind information not available. Following frames may be wrong.
	0022fb4c 76f99f72 kernel32!BaseThreadInitThunk+0x12
	0022fb8c 76f99f45 ntdll!RtlInitializeExceptionChain+0x63
	0022fba4 00000000 ntdll!RtlInitializeExceptionChain+0x36

Original issue's description:
> Improve the ScopedHandle verifier.
>
> 1. Automate the selection of the proper channel to enable the verifier.
>    Now the code is enabled at runtime.
>
> 2. Switch to a hash_map to track handles.
>
> 3. Intercept CloseHandle to detect the code that is closing handles owned
>    by ScopedHandles. The initial implementation only covers chrome.exe/dll,
>    but the plan is to extend that in the future to all modules loaded in the
>    process.
>
> BUG=362176
> R=cpu@chromium.org
> R=sky@chromium.org
>
> See https://codereview.chromium.org/490043002/ for the actual review.
>
> TBR=cpu@chromium.org
> TBR=sky@chromium.org
>
> Committed: https://chromium.googlesource.com/chromium/src/+/c928d0383db43f2f4baf8f9b24ed7454bf7eda64

TBR=cpu@chromium.org,sky@chromium.org,rvargas@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=362176

Review URL: https://codereview.chromium.org/507893002

Cr-Commit-Position: refs/heads/master@{#292047}
parent f712d106
Hide whitespace changes
Inline Side-by-side
Showing with 31 additions and 260 deletions
base/win/scoped_handle.cc
View file @ cf244d18
......@@ -4,12 +4,11 @@
#include "base/win/scoped_handle.h"
#include "base/containers/hash_tables.h"
#include <map>
#include "base/debug/alias.h"
#include "base/hash.h"
#include "base/lazy_instance.h"
#include "base/logging.h"
#include "base/synchronization/lock_impl.h"
#include "base/synchronization/lock.h"
#include "base/win/windows_version.h"
namespace {
......@@ -20,86 +19,23 @@ struct Info {
const void* pc2;
DWORD thread_id;
};
typedef base::hash_map<HANDLE, Info> HandleMap;
typedef std::map<HANDLE, Info> HandleMap;
typedef base::internal::LockImpl NativeLock;
base::LazyInstance<HandleMap>::Leaky g_handle_map = LAZY_INSTANCE_INITIALIZER;
base::LazyInstance<NativeLock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
bool g_closing = false;
bool g_verifier_enabled = false;
bool CloseHandleWrapper(HANDLE handle) {
if (!::CloseHandle(handle))
CHECK(false);
return true;
}
// Simple automatic locking using a native critical section so it supports
// recursive locking.
class AutoNativeLock {
public:
explicit AutoNativeLock(NativeLock& lock) : lock_(lock) {
lock_.Lock();
}
~AutoNativeLock() {
lock_.Unlock();
}
private:
NativeLock& lock_;
DISALLOW_COPY_AND_ASSIGN(AutoNativeLock);
};
inline size_t HashHandle(const HANDLE& handle) {
char buffer[sizeof(handle)];
memcpy(buffer, &handle, sizeof(handle));
return base::Hash(buffer, sizeof(buffer));
}
base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
} // namespace
namespace BASE_HASH_NAMESPACE {
#if defined(COMPILER_MSVC)
inline size_t hash_value(const HANDLE& handle) {
return HashHandle(handle);
}
#elif defined (COMPILER_GCC)
template <>
struct hash<HANDLE> {
size_t operator()(const HANDLE& handle) const {
return HashHandle(handle);
}
};
#endif
} // BASE_HASH_NAMESPACE
namespace base {
namespace win {
// Static.
bool HandleTraits::CloseHandle(HANDLE handle) {
if (!g_verifier_enabled)
return CloseHandleWrapper(handle);
AutoNativeLock lock(g_lock.Get());
g_closing = true;
CloseHandleWrapper(handle);
g_closing = false;
return true;
}
// Static.
void VerifierTraits::StartTracking(HANDLE handle, const void* owner,
const void* pc1, const void* pc2) {
if (!g_verifier_enabled)
return;
// Grab the thread id before the lock.
DWORD thread_id = GetCurrentThreadId();
AutoNativeLock lock(g_lock.Get());
AutoLock lock(g_lock.Get());
Info handle_info = { owner, pc1, pc2, thread_id };
std::pair<HANDLE, Info> item(handle, handle_info);
......@@ -114,10 +50,7 @@ void VerifierTraits::StartTracking(HANDLE handle, const void* owner,
// Static.
void VerifierTraits::StopTracking(HANDLE handle, const void* owner,
const void* pc1, const void* pc2) {
if (!g_verifier_enabled)
return;
AutoNativeLock lock(g_lock.Get());
AutoLock lock(g_lock.Get());
HandleMap::iterator i = g_handle_map.Get().find(handle);
if (i == g_handle_map.Get().end())
CHECK(false);
......@@ -131,23 +64,5 @@ void VerifierTraits::StopTracking(HANDLE handle, const void* owner,
g_handle_map.Get().erase(i);
}
void EnableHandleVerifier() {
g_verifier_enabled = true;
}
void OnHandleBeingClosed(HANDLE handle) {
AutoNativeLock lock(g_lock.Get());
if (g_closing)
return;
HandleMap::iterator i = g_handle_map.Get().find(handle);
if (i == g_handle_map.Get().end())
return;
Info other = i->second;
debug::Alias(&other);
CHECK(false);
}
} // namespace win
} // namespace base
base/win/scoped_handle.h
View file @ cf244d18
......@@ -101,7 +101,9 @@ class GenericScopedHandle {
Verifier::StopTracking(handle_, this, BASE_WIN_GET_CALLER,
tracked_objects::GetProgramCounter());
Traits::CloseHandle(handle_);
if (!Traits::CloseHandle(handle_))
CHECK(false);
handle_ = Traits::NullHandle();
}
}
......@@ -118,7 +120,9 @@ class HandleTraits {
typedef HANDLE Handle;
// Closes the handle.
static bool BASE_EXPORT CloseHandle(HANDLE handle);
static bool CloseHandle(HANDLE handle) {
return ::CloseHandle(handle) != FALSE;
}
// Returns true if the handle value is valid.
static bool IsHandleValid(HANDLE handle) {
......@@ -164,17 +168,6 @@ class BASE_EXPORT VerifierTraits {
typedef GenericScopedHandle<HandleTraits, VerifierTraits> ScopedHandle;
// This function should be called by the embedder to enable the use of
// VerifierTraits at runtime. It has no effect if DummyVerifierTraits is used
// for ScopedHandle.
void BASE_EXPORT EnableHandleVerifier();
// This should be called whenever the OS is closing a handle, if extended
// verification of improper handle closing is desired. If |handle| is being
// tracked by the handle verifier and ScopedHandle is not the one closing it,
// a CHECK is generated.
void BASE_EXPORT OnHandleBeingClosed(HANDLE handle);
} // namespace win
} // namespace base
......
chrome/BUILD.gn
View file @ cf244d18
......@@ -102,8 +102,6 @@ shared_library("main_dll") {
"app/chrome_main.cc",
"app/chrome_main_delegate.cc",
"app/chrome_main_delegate.h",
"app/close_handle_hook_win.cc",
"app/close_handle_hook_win.h",
"app/delay_load_hook_win.cc",
"app/delay_load_hook_win.h",
"//base/win/dllmain.cc",
......
chrome/app/chrome_main.cc
View file @ cf244d18
......@@ -9,7 +9,6 @@
#if defined(OS_WIN)
#include "base/debug/dump_without_crashing.h"
#include "base/win/win_util.h"
#include "chrome/app/close_handle_hook_win.h"
#include "chrome/common/chrome_constants.h"
#define DLLEXPORT __declspec(dllexport)
......@@ -42,8 +41,6 @@ int ChromeMain(int argc, const char** argv) {
params.instance = instance;
params.sandbox_info = sandbox_info;
InstallCloseHandleHooks();
// SetDumpWithoutCrashingFunction must be passed the DumpProcess function
// from the EXE and not from the DLL in order for DumpWithoutCrashing to
// function correctly.
......@@ -60,7 +57,6 @@ int ChromeMain(int argc, const char** argv) {
int rv = content::ContentMain(params);
#if defined(OS_WIN)
RemoveCloseHandleHooks();
base::win::SetShouldCrashOnProcessDetach(false);
#endif
......
chrome/app/close_handle_hook_win.cc deleted 100644 → 0
View file @ f712d106
// Copyright 2014 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.
#include "chrome/app/close_handle_hook_win.h"
#include <Windows.h>
#include <vector>
#include "base/files/file_path.h"
#include "base/lazy_instance.h"
#include "base/strings/string16.h"
#include "base/win/iat_patch_function.h"
#include "base/win/scoped_handle.h"
#include "chrome/common/chrome_version_info.h"
namespace {
typedef BOOL (WINAPI* CloseHandleType) (HANDLE handle);
CloseHandleType g_close_function = NULL;
// The entry point for CloseHandle interception. This function notifies the
// verifier about the handle that is being closed, and calls the original
// function.
BOOL WINAPI CloseHandleHook(HANDLE handle) {
base::win::OnHandleBeingClosed(handle);
return g_close_function(handle);
}
// Keeps track of all the hooks needed to intercept CloseHandle.
class CloseHandleHooks {
public:
CloseHandleHooks() {}
~CloseHandleHooks() {}
void AddIATPatch(const base::string16& module);
void Unpatch();
private:
std::vector<base::win::IATPatchFunction*> hooks_;
DISALLOW_COPY_AND_ASSIGN(CloseHandleHooks);
};
base::LazyInstance<CloseHandleHooks> g_hooks = LAZY_INSTANCE_INITIALIZER;
void CloseHandleHooks::AddIATPatch(const base::string16& module) {
if (module.empty())
return;
base::win::IATPatchFunction* patch = new base::win::IATPatchFunction;
patch->Patch(module.c_str(), "kernel32.dll", "CloseHandle", CloseHandleHook);
hooks_.push_back(patch);
if (!g_close_function) {
// Things are probably messed up if each intercepted function points to
// a different place, but we need only one function to call.
g_close_function =
reinterpret_cast<CloseHandleType>(patch->original_function());
}
}
void CloseHandleHooks::Unpatch() {
for (std::vector<base::win::IATPatchFunction*>::iterator it = hooks_.begin();
it != hooks_.end(); ++it) {
(*it)->Unpatch();
}
}
bool UseHooks() {
chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
if (channel == chrome::VersionInfo::CHANNEL_CANARY ||
channel == chrome::VersionInfo::CHANNEL_DEV) {
return true;
}
return false;
}
base::string16 GetModuleName(HMODULE module) {
base::string16 name;
if (!module)
return name;
wchar_t buffer[MAX_PATH];
int rv = GetModuleFileName(module, buffer, MAX_PATH);
if (rv == MAX_PATH)
return name;
buffer[MAX_PATH - 1] = L'\0';
name.assign(buffer);
base::FilePath path(name);
return path.BaseName().AsUTF16Unsafe();
}
HMODULE GetChromeDLLModule() {
HMODULE module;
if (!GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS |
GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT,
reinterpret_cast<wchar_t*>(&GetChromeDLLModule),
&module)) {
return NULL;
}
return module;
}
} // namespace
void InstallCloseHandleHooks() {
if (!UseHooks())
return;
base::win::EnableHandleVerifier();
CloseHandleHooks* hooks = g_hooks.Pointer();
hooks->AddIATPatch(L"chrome.exe");
hooks->AddIATPatch(GetModuleName(GetChromeDLLModule()));
}
void RemoveCloseHandleHooks() {
g_hooks.Get().Unpatch();
}
chrome/app/close_handle_hook_win.h deleted 100644 → 0
View file @ f712d106
// Copyright 2014 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_APP_CLOSE_HANDLE_HOOK_WIN_H_
#define CHROME_APP_CLOSE_HANDLE_HOOK_WIN_H_
// Installs the hooks required to debug use of improper handles.
void InstallCloseHandleHooks();
// Removes the hooks installed by InstallCloseHandleHooks().
void RemoveCloseHandleHooks();
#endif // CHROME_APP_CLOSE_HANDLE_HOOK_WIN_H_
chrome/chrome_dll.gypi
View file @ cf244d18
......@@ -77,20 +77,6 @@
'variables': {
'enable_wexit_time_destructors': 1,
},
'sources': [
'app/chrome_command_ids.h',
'app/chrome_dll_resource.h',
'app/chrome_main.cc',
'app/chrome_main_delegate.cc',
'app/chrome_main_delegate.h',
'app/chrome_main_mac.mm',
'app/chrome_main_mac.h',
'app/close_handle_hook_win.cc',
'app/close_handle_hook_win.h',
'app/delay_load_hook_win.cc',
'app/delay_load_hook_win.h',
'../base/win/dllmain.cc',
],
'dependencies': [
'<@(chromium_browser_dependencies)',
'../content/content.gyp:content_app_browser',
......@@ -131,9 +117,17 @@
'../ui/views/views.gyp:views',
],
'sources': [
'app/chrome_command_ids.h',
'app/chrome_dll.rc',
'app/chrome_dll_resource.h',
'app/chrome_main.cc',
'app/chrome_main_delegate.cc',
'app/chrome_main_delegate.h',
'app/delay_load_hook_win.cc',
'app/delay_load_hook_win.h',
'<(SHARED_INTERMEDIATE_DIR)/chrome_version/chrome_dll_version.rc',
'../base/win/dllmain.cc',
# Cursors.
'<(SHARED_INTERMEDIATE_DIR)/ui/resources/ui_unscaled_resources.rc',
......@@ -278,6 +272,15 @@
# sets -order_file.
'ORDER_FILE': 'app/framework.order',
},
'sources': [
'app/chrome_command_ids.h',
'app/chrome_dll_resource.h',
'app/chrome_main.cc',
'app/chrome_main_delegate.cc',
'app/chrome_main_delegate.h',
'app/chrome_main_mac.mm',
'app/chrome_main_mac.h',
],
'dependencies': [
'../pdf/pdf.gyp:pdf',
],
......@@ -347,8 +350,6 @@
'app/chrome_main.cc',
'app/chrome_main_delegate.cc',
'app/chrome_main_delegate.h',
'app/close_handle_hook_win.cc',
'app/close_handle_hook_win.h',
],
'conditions': [
['OS=="win"', {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment