Reland "Enable support for handle verifier on 64-bit (arm64/x86-64)"
This is a reland of commit f310096d18d921892867e2930e603a7fd32a3596
The previous CL failed to take into account that on official
builds the CHECK() message is empty to save space.
Therefore, the tests that validate which exact failure is being
hit were incorrectly failing.
This CL adds a handler for this in PS2. Otherwise, the CL
remains identical to before.
Original change's description:
> Enable support for handle verifier on 64-bit (arm64/x86-64)
>
> The EAT entry is only 32-bit so on 64-bit there is not enough
> space to overwrite the entry in kernel32.dll.
>
> On 64-bit therefore, the ModuleWatcher is used to watch for
> module loads and when they happen, the IAT is patched on-demand.
>
> For non-browser process on 64-bit, attempt to patch IAT of
> all loaded modules, but do not watch for new ones. This loses
> some coverage on 64-bit for these processes but it's better
> than nothing, and mostly all modules will have loaded by the
> point the IAT is patched anyway.
>
> This CL also adds to the current death tests by validating
> that each death is caused by the right failure. It also makes
> explicit what was already true: which is that these hooks
> are never removed.
>
> BUG=1104358
>
> Cq-Include-Trybots: luci.chromium.try:win7-rel,win-asan
> Change-Id: I8adc5164cd08899264c7f6e34668b43b4e8c4876
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3628394
> Commit-Queue: Will Harris <wfh@chromium.org>
> Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Greg Thompson <grt@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1005515}
Bug: 1104358
Change-Id: I520de39beb5784a02e61a4b4f12f2c76086f11de
Cq-Include-Trybots: luci.chromium.try:win7-rel,win-asan
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3657593
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1006039}
diff --git a/base/BUILD.gn b/base/BUILD.gn
index 1c3b41c..e5a179d 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -1019,11 +1019,11 @@
if (is_win) {
sources += [
- "debug/close_handle_hook_win.cc",
- "debug/close_handle_hook_win.h",
"debug/debugger_win.cc",
"debug/gdi_debug_util_win.cc",
"debug/gdi_debug_util_win.h",
+ "debug/handle_hooks_win.cc",
+ "debug/handle_hooks_win.h",
"debug/invalid_access_win.cc",
"debug/invalid_access_win.h",
"debug/stack_trace_win.cc",
diff --git a/base/debug/close_handle_hook_win.h b/base/debug/close_handle_hook_win.h
deleted file mode 100644
index c775d75..0000000
--- a/base/debug/close_handle_hook_win.h
+++ /dev/null
@@ -1,19 +0,0 @@
-// Copyright 2015 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 BASE_DEBUG_CLOSE_HANDLE_HOOK_WIN_H_
-#define BASE_DEBUG_CLOSE_HANDLE_HOOK_WIN_H_
-
-#include "base/base_export.h"
-
-namespace base {
-namespace debug {
-
-// Installs the hooks required to debug use of improper handles.
-BASE_EXPORT void InstallHandleHooks();
-
-} // namespace debug
-} // namespace base
-
-#endif // BASE_DEBUG_CLOSE_HANDLE_HOOK_WIN_H_
diff --git a/base/debug/close_handle_hook_win.cc b/base/debug/handle_hooks_win.cc
similarity index 61%
rename from base/debug/close_handle_hook_win.cc
rename to base/debug/handle_hooks_win.cc
index 68b0198..bc3eed4b 100644
--- a/base/debug/close_handle_hook_win.cc
+++ b/base/debug/handle_hooks_win.cc
@@ -2,17 +2,16 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include "base/debug/close_handle_hook_win.h"
+#include "base/debug/handle_hooks_win.h"
-#include <Windows.h>
+#include <windows.h>
+
#include <psapi.h>
#include <stddef.h>
-#include <algorithm>
-#include <memory>
-#include <vector>
-
+#include "base/logging.h"
#include "base/memory/raw_ptr.h"
+#include "base/numerics/safe_conversions.h"
#include "base/win/iat_patch_function.h"
#include "base/win/pe_image.h"
#include "base/win/scoped_handle.h"
@@ -20,24 +19,18 @@
namespace {
-typedef BOOL (WINAPI* CloseHandleType) (HANDLE handle);
+using CloseHandleType = decltype(&::CloseHandle);
+using DuplicateHandleType = decltype(&::DuplicateHandle);
-typedef BOOL (WINAPI* DuplicateHandleType)(HANDLE source_process,
- HANDLE source_handle,
- HANDLE target_process,
- HANDLE* target_handle,
- DWORD desired_access,
- BOOL inherit_handle,
- DWORD options);
-
-CloseHandleType g_close_function = NULL;
-DuplicateHandleType g_duplicate_function = NULL;
+CloseHandleType g_close_function = nullptr;
+DuplicateHandleType g_duplicate_function = nullptr;
// 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);
+ base::win::OnHandleBeingClosed(handle,
+ base::win::HandleOperation::kCloseHandleHook);
return g_close_function(handle);
}
@@ -50,7 +43,8 @@
DWORD options) {
if ((options & DUPLICATE_CLOSE_SOURCE) &&
(GetProcessId(source_process) == ::GetCurrentProcessId())) {
- base::win::OnHandleBeingClosed(source_handle);
+ base::win::OnHandleBeingClosed(
+ source_handle, base::win::HandleOperation::kDuplicateHandleHook);
}
return g_duplicate_function(source_process, source_handle, target_process,
@@ -74,9 +68,7 @@
AutoProtectMemory(const AutoProtectMemory&) = delete;
AutoProtectMemory& operator=(const AutoProtectMemory&) = delete;
- ~AutoProtectMemory() {
- RevertProtection();
- }
+ ~AutoProtectMemory() { RevertProtection(); }
// Grants write access to a given memory range.
bool ChangeProtection(void* address, size_t bytes);
@@ -101,7 +93,7 @@
return false;
DWORD is_executable = (PAGE_EXECUTE | PAGE_EXECUTE_READ |
- PAGE_EXECUTE_READWRITE | PAGE_EXECUTE_WRITECOPY) &
+ PAGE_EXECUTE_READWRITE | PAGE_EXECUTE_WRITECOPY) &
memory_info.Protect;
DWORD protect = is_executable ? PAGE_EXECUTE_READWRITE : PAGE_READWRITE;
@@ -128,9 +120,12 @@
old_protect_ = 0;
}
-// Performs an EAT interception.
-void EATPatch(HMODULE module, const char* function_name,
- void* new_function, void** old_function) {
+#if defined(ARCH_CPU_32_BITS)
+// Performs an EAT interception. Only supported on 32-bit.
+void EATPatch(HMODULE module,
+ const char* function_name,
+ void* new_function,
+ void** old_function) {
if (!module)
return;
@@ -150,30 +145,35 @@
return;
// Perform the patch.
- *eat_entry = static_cast<DWORD>(reinterpret_cast<uintptr_t>(new_function) -
- reinterpret_cast<uintptr_t>(module));
+ *eat_entry =
+ base::checked_cast<DWORD>(reinterpret_cast<uintptr_t>(new_function) -
+ reinterpret_cast<uintptr_t>(module));
}
+#endif // defined(ARCH_CPU_32_BITS)
// Performs an IAT interception.
-base::win::IATPatchFunction* IATPatch(HMODULE module, const char* function_name,
- void* new_function, void** old_function) {
+std::unique_ptr<base::win::IATPatchFunction> IATPatch(HMODULE module,
+ const char* function_name,
+ void* new_function,
+ void** old_function) {
if (!module)
- return NULL;
+ return nullptr;
- base::win::IATPatchFunction* patch = new base::win::IATPatchFunction;
+ auto patch = std::make_unique<base::win::IATPatchFunction>();
__try {
// There is no guarantee that |module| is still loaded at this point.
if (patch->PatchFromModule(module, "kernel32.dll", function_name,
new_function)) {
- delete patch;
- return NULL;
+ return nullptr;
}
- } __except((GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION ||
- GetExceptionCode() == EXCEPTION_GUARD_PAGE ||
- GetExceptionCode() == EXCEPTION_IN_PAGE_ERROR) ?
- EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH) {
+ } __except ((GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION ||
+ GetExceptionCode() == EXCEPTION_GUARD_PAGE ||
+ GetExceptionCode() == EXCEPTION_IN_PAGE_ERROR)
+ ? EXCEPTION_EXECUTE_HANDLER
+ : EXCEPTION_CONTINUE_SEARCH) {
// Leak the patch.
- return NULL;
+ std::ignore = patch.release();
+ return nullptr;
}
if (!(*old_function)) {
@@ -184,44 +184,32 @@
return patch;
}
-// Keeps track of all the hooks needed to intercept functions which could
-// possibly close handles.
-class HandleHooks {
- public:
- HandleHooks() {}
+} // namespace
- HandleHooks(const HandleHooks&) = delete;
- HandleHooks& operator=(const HandleHooks&) = delete;
-
- ~HandleHooks() {}
-
- void AddIATPatch(HMODULE module);
- void AddEATPatch();
-
- private:
- std::vector<base::win::IATPatchFunction*> hooks_;
-};
-
+// static
void HandleHooks::AddIATPatch(HMODULE module) {
if (!module)
return;
- base::win::IATPatchFunction* patch = NULL;
- patch =
+ auto close_handle_patch =
IATPatch(module, "CloseHandle", reinterpret_cast<void*>(&CloseHandleHook),
reinterpret_cast<void**>(&g_close_function));
- if (!patch)
+ if (!close_handle_patch)
return;
- hooks_.push_back(patch);
+ // This is intentionally leaked.
+ std::ignore = close_handle_patch.release();
- patch = IATPatch(module, "DuplicateHandle",
- reinterpret_cast<void*>(&DuplicateHandleHook),
- reinterpret_cast<void**>(&g_duplicate_function));
- if (!patch)
+ auto duplicate_handle_patch = IATPatch(
+ module, "DuplicateHandle", reinterpret_cast<void*>(&DuplicateHandleHook),
+ reinterpret_cast<void**>(&g_duplicate_function));
+ if (!duplicate_handle_patch)
return;
- hooks_.push_back(patch);
+ // This is intentionally leaked.
+ std::ignore = duplicate_handle_patch.release();
}
+#if defined(ARCH_CPU_32_BITS)
+// static
void HandleHooks::AddEATPatch() {
// An attempt to restore the entry on the table at destruction is not safe.
EATPatch(GetModuleHandleA("kernel32.dll"), "CloseHandle",
@@ -231,33 +219,24 @@
reinterpret_cast<void*>(&DuplicateHandleHook),
reinterpret_cast<void**>(&g_duplicate_function));
}
+#endif // defined(ARCH_CPU_32_BITS)
-void PatchLoadedModules(HandleHooks* hooks) {
+// static
+void HandleHooks::PatchLoadedModules() {
const DWORD kSize = 256;
DWORD returned;
- std::unique_ptr<HMODULE[]> modules(new HMODULE[kSize]);
- if (!EnumProcessModules(GetCurrentProcess(), modules.get(),
- kSize * sizeof(HMODULE), &returned)) {
+ auto modules = std::make_unique<HMODULE[]>(kSize);
+ if (!::EnumProcessModules(GetCurrentProcess(), modules.get(),
+ kSize * sizeof(HMODULE), &returned)) {
return;
}
returned /= sizeof(HMODULE);
returned = std::min(kSize, returned);
for (DWORD current = 0; current < returned; current++) {
- hooks->AddIATPatch(modules[current]);
+ AddIATPatch(modules[current]);
}
}
-} // namespace
-
-void InstallHandleHooks() {
- static HandleHooks* hooks = new HandleHooks();
-
- // Performing EAT interception first is safer in the presence of other
- // threads attempting to call CloseHandle.
- hooks->AddEATPatch();
- PatchLoadedModules(hooks);
-}
-
} // namespace debug
} // namespace base
diff --git a/base/debug/handle_hooks_win.h b/base/debug/handle_hooks_win.h
new file mode 100644
index 0000000..5459393
--- /dev/null
+++ b/base/debug/handle_hooks_win.h
@@ -0,0 +1,41 @@
+// Copyright 2015 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 BASE_DEBUG_HANDLE_HOOKS_WIN_H_
+#define BASE_DEBUG_HANDLE_HOOKS_WIN_H_
+
+#include "base/base_export.h"
+#include "base/win/windows_types.h"
+#include "build/build_config.h"
+
+namespace base {
+namespace debug {
+
+// Provides the ability to intercept functions which could possibly close
+// handles in support of the handle tracker.
+// This is a currently a container class for static functions because there is
+// ongoing work to make the patches unhook, currently blocked by test failures.
+// See https://crbug.com/1327397.
+class BASE_EXPORT HandleHooks {
+ public:
+ HandleHooks() = delete;
+
+ HandleHooks(const HandleHooks&) = delete;
+ HandleHooks& operator=(const HandleHooks&) = delete;
+
+ // Patch IAT for a specified module.
+ static void AddIATPatch(HMODULE module);
+ // Add an EAT patch on kernel32.dll. This patch does not get removed. This is
+ // only supported on 32-bit because the EAT only supports 32-bit RVAs.
+#if defined(ARCH_CPU_32_BITS)
+ static void AddEATPatch();
+#endif
+ // Patch IAT for all currently loaded modules.
+ static void PatchLoadedModules();
+};
+
+} // namespace debug
+} // namespace base
+
+#endif // BASE_DEBUG_HANDLE_HOOKS_WIN_H_
diff --git a/base/win/scoped_handle.cc b/base/win/scoped_handle.cc
index de68545..1c1cdce 100644
--- a/base/win/scoped_handle.cc
+++ b/base/win/scoped_handle.cc
@@ -11,6 +11,21 @@
using base::win::internal::ScopedHandleVerifier;
+std::ostream& operator<<(std::ostream& os, HandleOperation operation) {
+ switch (operation) {
+ case HandleOperation::kHandleAlreadyTracked:
+ return os << "Handle Already Tracked";
+ case HandleOperation::kCloseHandleNotTracked:
+ return os << "Closing an untracked handle";
+ case HandleOperation::kCloseHandleNotOwner:
+ return os << "Closing a handle owned by something else";
+ case HandleOperation::kCloseHandleHook:
+ return os << "CloseHandleHook validation failure";
+ case HandleOperation::kDuplicateHandleHook:
+ return os << "DuplicateHandleHook validation failure";
+ }
+}
+
// Static.
bool HandleTraits::CloseHandle(HANDLE handle) {
return ScopedHandleVerifier::Get()->CloseHandle(handle);
@@ -36,8 +51,8 @@
return ScopedHandleVerifier::Get()->Disable();
}
-void OnHandleBeingClosed(HANDLE handle) {
- return ScopedHandleVerifier::Get()->OnHandleBeingClosed(handle);
+void OnHandleBeingClosed(HANDLE handle, HandleOperation operation) {
+ return ScopedHandleVerifier::Get()->OnHandleBeingClosed(handle, operation);
}
} // namespace win
diff --git a/base/win/scoped_handle.h b/base/win/scoped_handle.h
index bfa8188..3a7d47e 100644
--- a/base/win/scoped_handle.h
+++ b/base/win/scoped_handle.h
@@ -7,6 +7,8 @@
#include "base/win/windows_types.h"
+#include <ostream>
+
#include "base/base_export.h"
#include "base/check_op.h"
#include "base/dcheck_is_on.h"
@@ -26,6 +28,16 @@
namespace base {
namespace win {
+enum class HandleOperation {
+ kHandleAlreadyTracked,
+ kCloseHandleNotTracked,
+ kCloseHandleNotOwner,
+ kCloseHandleHook,
+ kDuplicateHandleHook
+};
+
+std::ostream& operator<<(std::ostream& os, HandleOperation operation);
+
// Generic wrapper for raw handles that takes care of closing handles
// automatically. The class interface follows the style of
// the ScopedFILE class with two additions:
@@ -113,8 +125,9 @@
}
private:
- FRIEND_TEST_ALL_PREFIXES(ScopedHandleTest, HandleVerifierWrongOwner);
- FRIEND_TEST_ALL_PREFIXES(ScopedHandleTest, HandleVerifierUntrackedHandle);
+ FRIEND_TEST_ALL_PREFIXES(ScopedHandleDeathTest, HandleVerifierWrongOwner);
+ FRIEND_TEST_ALL_PREFIXES(ScopedHandleDeathTest,
+ HandleVerifierUntrackedHandle);
Handle handle_;
};
@@ -183,7 +196,7 @@
GenericScopedHandle<HandleTraits, DummyVerifierTraits>;
using CheckedScopedHandle = GenericScopedHandle<HandleTraits, VerifierTraits>;
-#if DCHECK_IS_ON() && !defined(ARCH_CPU_64_BITS)
+#if DCHECK_IS_ON()
using ScopedHandle = CheckedScopedHandle;
#else
using ScopedHandle = UncheckedScopedHandle;
@@ -198,7 +211,8 @@
// 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.
-BASE_EXPORT void OnHandleBeingClosed(HANDLE handle);
+BASE_EXPORT void OnHandleBeingClosed(HANDLE handle, HandleOperation operation);
+
} // namespace win
} // namespace base
diff --git a/base/win/scoped_handle_unittest.cc b/base/win/scoped_handle_unittest.cc
index 0df21ce..1b4ccbc 100644
--- a/base/win/scoped_handle_unittest.cc
+++ b/base/win/scoped_handle_unittest.cc
@@ -8,11 +8,13 @@
#include "base/base_switches.h"
#include "base/command_line.h"
+#include "base/debug/handle_hooks_win.h"
#include "base/files/file_path.h"
#include "base/scoped_native_library.h"
#include "base/test/multiprocess_test.h"
#include "base/test/test_timeouts.h"
#include "base/win/scoped_handle.h"
+#include "build/build_config.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "testing/multiprocess_func_list.h"
@@ -20,13 +22,49 @@
namespace base {
namespace win {
+namespace {
+
+std::string FailureMessage(const std::string& msg) {
+#if !defined(DEBUG) && defined(OFFICIAL_BUILD)
+ // Official release builds strip all fatal messages for saving binary size,
+ // see base/check.h.
+ return "";
+#else
+ return msg;
+#endif
+}
+
+} // namespace
+
namespace testing {
extern "C" bool __declspec(dllexport) RunTest();
} // namespace testing
-TEST(ScopedHandleTest, ScopedHandle) {
+class ScopedHandleTest : public ::testing::Test,
+ public ::testing::WithParamInterface<bool> {
+ public:
+ ScopedHandleTest(const ScopedHandleTest&) = delete;
+ ScopedHandleTest& operator=(const ScopedHandleTest&) = delete;
+
+ protected:
+ ScopedHandleTest() {
+ if (HooksEnabled()) {
+#if defined(ARCH_CPU_32_BITS)
+ // EAT patch is only supported on 32-bit.
+ base::debug::HandleHooks::AddEATPatch();
+#endif
+ base::debug::HandleHooks::PatchLoadedModules();
+ }
+ }
+
+ static bool HooksEnabled() { return GetParam(); }
+};
+
+using ScopedHandleDeathTest = ScopedHandleTest;
+
+TEST_P(ScopedHandleTest, ScopedHandle) {
// Any illegal error code will do. We just need to test that it is preserved
- // by ScopedHandle to avoid bug 528394.
+ // by ScopedHandle to avoid https://crbug.com/528394.
const DWORD magic_error = 0x12345678;
HANDLE handle = ::CreateMutex(nullptr, false, nullptr);
@@ -49,7 +87,7 @@
EXPECT_EQ(magic_error, ::GetLastError());
}
-TEST(ScopedHandleTest, HandleVerifierTrackedHasBeenClosed) {
+TEST_P(ScopedHandleDeathTest, HandleVerifierTrackedHasBeenClosed) {
HANDLE handle = ::CreateMutex(nullptr, false, nullptr);
ASSERT_NE(HANDLE(nullptr), handle);
using NtCloseFunc = decltype(&::NtClose);
@@ -64,10 +102,35 @@
// Destructing a ScopedHandle with an illegally closed handle should
// fail.
},
- "");
+ FailureMessage("CloseHandle failed"));
}
-TEST(ScopedHandleTest, HandleVerifierDoubleTracking) {
+TEST_P(ScopedHandleDeathTest, HandleVerifierCloseTrackedHandle) {
+ // This test is only valid if hooks are enabled.
+ if (!HooksEnabled())
+ return;
+ ASSERT_DEATH(
+ {
+ HANDLE handle = ::CreateMutex(nullptr, false, nullptr);
+ ASSERT_NE(HANDLE(nullptr), handle);
+
+ // Start tracking the handle so that closes outside of the checker are
+ // caught.
+ base::win::CheckedScopedHandle handle_holder(handle);
+
+ // Closing a tracked handle using ::CloseHandle should crash due to hook
+ // noticing the illegal close.
+ ::CloseHandle(handle);
+ },
+ // This test must match the CloseHandleHook causing this failure, because
+ // if the hook doesn't crash and instead the handle is double closed by
+ // the `handle_holder` going out of scope, then there is still a crash,
+ // but a different crash and one we are not explicitly testing here. This
+ // other crash is tested in HandleVerifierTrackedHasBeenClosed above.
+ FailureMessage("CloseHandleHook validation failure"));
+}
+
+TEST_P(ScopedHandleDeathTest, HandleVerifierDoubleTracking) {
HANDLE handle = ::CreateMutex(nullptr, false, nullptr);
ASSERT_NE(HANDLE(nullptr), handle);
@@ -76,7 +139,7 @@
ASSERT_DEATH({ base::win::CheckedScopedHandle handle_holder2(handle); }, "");
}
-TEST(ScopedHandleTest, HandleVerifierWrongOwner) {
+TEST_P(ScopedHandleDeathTest, HandleVerifierWrongOwner) {
HANDLE handle = ::CreateMutex(nullptr, false, nullptr);
ASSERT_NE(HANDLE(nullptr), handle);
@@ -86,12 +149,12 @@
base::win::CheckedScopedHandle handle_holder2;
handle_holder2.handle_ = handle;
},
- "");
+ FailureMessage("Closing a handle owned by something else"));
ASSERT_TRUE(handle_holder.is_valid());
handle_holder.Close();
}
-TEST(ScopedHandleTest, HandleVerifierUntrackedHandle) {
+TEST_P(ScopedHandleDeathTest, HandleVerifierUntrackedHandle) {
HANDLE handle = ::CreateMutex(nullptr, false, nullptr);
ASSERT_NE(HANDLE(nullptr), handle);
@@ -100,7 +163,7 @@
base::win::CheckedScopedHandle handle_holder;
handle_holder.handle_ = handle;
},
- "");
+ FailureMessage("Closing an untracked handle"));
ASSERT_TRUE(::CloseHandle(handle));
}
@@ -113,7 +176,7 @@
#define MAYBE_MultiProcess MultiProcess
#endif
-TEST(ScopedHandleTest, MAYBE_MultiProcess) {
+TEST_P(ScopedHandleTest, MAYBE_MultiProcess) {
// Initializing ICU in the child process causes a scoped handle to be created
// before the test gets a chance to test the race condition, so disable ICU
// for the child process here.
@@ -145,5 +208,18 @@
return 0;
}
+INSTANTIATE_TEST_SUITE_P(HooksEnabled,
+ ScopedHandleTest,
+ ::testing::Values(true));
+INSTANTIATE_TEST_SUITE_P(HooksDisabled,
+ ScopedHandleTest,
+ ::testing::Values(false));
+INSTANTIATE_TEST_SUITE_P(HooksEnabled,
+ ScopedHandleDeathTest,
+ ::testing::Values(true));
+INSTANTIATE_TEST_SUITE_P(HooksDisabled,
+ ScopedHandleDeathTest,
+ ::testing::Values(false));
+
} // namespace win
} // namespace base
diff --git a/base/win/scoped_handle_verifier.cc b/base/win/scoped_handle_verifier.cc
index e64bd2c..e7ab4b9 100644
--- a/base/win/scoped_handle_verifier.cc
+++ b/base/win/scoped_handle_verifier.cc
@@ -18,6 +18,7 @@
#include "base/trace_event/base_tracing.h"
#include "base/win/base_win_buildflags.h"
#include "base/win/current_module.h"
+#include "base/win/scoped_handle.h"
extern "C" {
__declspec(dllexport) void* GetHandleVerifier();
@@ -40,21 +41,25 @@
using NativeLock = base::internal::LockImpl;
NOINLINE void ReportErrorOnScopedHandleOperation(
- const base::debug::StackTrace& creation_stack) {
+ const base::debug::StackTrace& creation_stack,
+ HandleOperation operation) {
auto creation_stack_copy = creation_stack;
base::debug::Alias(&creation_stack_copy);
- CHECK(false);
+ base::debug::Alias(&operation);
+ CHECK(false) << operation;
__builtin_unreachable();
}
NOINLINE void ReportErrorOnScopedHandleOperation(
const base::debug::StackTrace& creation_stack,
- const ScopedHandleVerifierInfo& other) {
+ const ScopedHandleVerifierInfo& other,
+ HandleOperation operation) {
auto other_stack_copy = *other.stack;
base::debug::Alias(&other_stack_copy);
auto creation_stack_copy = creation_stack;
base::debug::Alias(&creation_stack_copy);
- CHECK(false);
+ base::debug::Alias(&operation);
+ CHECK(false) << operation;
__builtin_unreachable();
}
@@ -107,7 +112,7 @@
bool CloseHandleWrapper(HANDLE handle) {
if (!::CloseHandle(handle))
- CHECK(false); // CloseHandle failed.
+ CHECK(false) << "CloseHandle failed";
return true;
}
@@ -202,9 +207,10 @@
enabled_ = false;
}
-void ScopedHandleVerifier::OnHandleBeingClosed(HANDLE handle) {
+void ScopedHandleVerifier::OnHandleBeingClosed(HANDLE handle,
+ HandleOperation operation) {
if (enabled_)
- OnHandleBeingClosedImpl(handle);
+ OnHandleBeingClosedImpl(handle, operation);
}
HMODULE ScopedHandleVerifier::GetModule() const {
@@ -227,7 +233,8 @@
thread_id});
if (!result.second) {
// Attempt to start tracking already tracked handle.
- ReportErrorOnScopedHandleOperation(creation_stack_, result.first->second);
+ ReportErrorOnScopedHandleOperation(creation_stack_, result.first->second,
+ HandleOperation::kHandleAlreadyTracked);
}
}
@@ -239,18 +246,22 @@
HandleMap::iterator i = map_.find(handle);
if (i == map_.end()) {
// Attempting to close an untracked handle.
- ReportErrorOnScopedHandleOperation(creation_stack_);
+ ReportErrorOnScopedHandleOperation(creation_stack_,
+ HandleOperation::kCloseHandleNotTracked);
}
if (i->second.owner != owner) {
// Attempting to close a handle not owned by opener.
- ReportErrorOnScopedHandleOperation(creation_stack_, i->second);
+ ReportErrorOnScopedHandleOperation(creation_stack_, i->second,
+ HandleOperation::kCloseHandleNotOwner);
}
map_.erase(i);
}
-NOINLINE void ScopedHandleVerifier::OnHandleBeingClosedImpl(HANDLE handle) {
+NOINLINE void ScopedHandleVerifier::OnHandleBeingClosedImpl(
+ HANDLE handle,
+ HandleOperation operation) {
if (closing_.Get())
return;
@@ -258,7 +269,7 @@
HandleMap::iterator i = map_.find(handle);
if (i != map_.end()) {
// CloseHandle called on tracked handle.
- ReportErrorOnScopedHandleOperation(creation_stack_, i->second);
+ ReportErrorOnScopedHandleOperation(creation_stack_, i->second, operation);
}
}
diff --git a/base/win/scoped_handle_verifier.h b/base/win/scoped_handle_verifier.h
index 84bb547d..7085e99 100644
--- a/base/win/scoped_handle_verifier.h
+++ b/base/win/scoped_handle_verifier.h
@@ -18,6 +18,7 @@
namespace base {
namespace win {
+enum class HandleOperation;
namespace internal {
struct HandleHash {
@@ -76,7 +77,7 @@
virtual void StopTracking(HANDLE handle, const void* owner, const void* pc1,
const void* pc2);
virtual void Disable();
- virtual void OnHandleBeingClosed(HANDLE handle);
+ virtual void OnHandleBeingClosed(HANDLE handle, HandleOperation operation);
virtual HMODULE GetModule() const;
private:
@@ -86,7 +87,7 @@
const void* pc2);
void StopTrackingImpl(HANDLE handle, const void* owner, const void* pc1,
const void* pc2);
- void OnHandleBeingClosedImpl(HANDLE handle);
+ void OnHandleBeingClosedImpl(HANDLE handle, HandleOperation operation);
static base::internal::LockImpl* GetLock();
static void InstallVerifier();
diff --git a/chrome/app/chrome_main_delegate.cc b/chrome/app/chrome_main_delegate.cc
index be62a1d..4fc8a2c4 100644
--- a/chrome/app/chrome_main_delegate.cc
+++ b/chrome/app/chrome_main_delegate.cc
@@ -85,7 +85,7 @@
#include <algorithm>
-#include "base/debug/close_handle_hook_win.h"
+#include "base/debug/handle_hooks_win.h"
#include "base/files/important_file_writer_cleaner.h"
#include "base/threading/platform_thread_win.h"
#include "base/win/atl.h"
@@ -807,8 +807,8 @@
*exit_code = 0;
return true; // Got a --version switch; exit with a success error code.
}
-// TODO(crbug.com/1052397): Revisit the macro expression once build flag switch
-// of lacros-chrome is complete.
+ // TODO(crbug.com/1052397): Revisit the macro expression once build flag
+ // switch of lacros-chrome is complete.
#if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS_LACROS)
// This will directly exit if the user asked for help.
HandleHelpSwitches(command_line);
@@ -822,11 +822,25 @@
return true;
}
-// HandleVerifier detects and reports incorrect handle manipulations. It tracks
-// handle operations on builds that support DCHECK only.
-// TODO(crbug/1104358): Support 64-bit handle hooks.
-#if DCHECK_IS_ON() && !defined(ARCH_CPU_64_BITS)
- base::debug::InstallHandleHooks();
+ // HandleVerifier detects and reports incorrect handle manipulations. It
+ // tracks handle operations on builds that support DCHECK only.
+#if DCHECK_IS_ON()
+ // This portion of the hook setup is just for child processes. Browser part is
+ // in ChromeBrowserMainPartsWin::PostProfileInit.
+ if (!is_browser) {
+ // Performing EAT interception first is safer in the presence of other
+ // threads attempting to call CloseHandle.
+#if defined(ARCH_CPU_32_BITS)
+ // Patching EAT of kernel32.dll is only supported on 32-bit because RVA can
+ // only hold 32-bit values.
+ base::debug::HandleHooks::AddEATPatch();
+#endif
+ // Patch once. Cannot monitor for further modules in a child process as
+ // monitoring needs ModuleWatcher, but likely no more should really load in
+ // a child process from this point on. If we miss any then we will lose some
+ // detection but still generate no false positive crashes.
+ base::debug::HandleHooks::PatchLoadedModules();
+ }
#else
base::win::DisableHandleVerifier();
#endif
diff --git a/chrome/browser/chrome_browser_main_win.cc b/chrome/browser/chrome_browser_main_win.cc
index 600097b..dc6f7b0 100644
--- a/chrome/browser/chrome_browser_main_win.cc
+++ b/chrome/browser/chrome_browser_main_win.cc
@@ -20,6 +20,8 @@
#include "base/bind.h"
#include "base/callback_helpers.h"
#include "base/command_line.h"
+#include "base/dcheck_is_on.h"
+#include "base/debug/handle_hooks_win.h"
#include "base/enterprise_util.h"
#include "base/environment.h"
#include "base/feature_list.h"
@@ -44,6 +46,7 @@
#include "base/win/windows_version.h"
#include "base/win/wrapped_window_proc.h"
#include "build/branding_buildflags.h"
+#include "build/build_config.h"
#include "chrome/browser/about_flags.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/enterprise/util/critical_policy_section_metrics_win.h"
@@ -383,74 +386,6 @@
return true;
}
-// Used as the callback for ModuleWatcher events in this process. Dispatches
-// them to the ModuleDatabase.
-// Note: This callback may be invoked on any thread, even those not owned by the
-// task scheduler, under the loader lock, directly on the thread where the
-// DLL is currently loading.
-void OnModuleEvent(const ModuleWatcher::ModuleEvent& event) {
- {
- TRACE_EVENT1("browser", "OnModuleEvent", "module_path",
- event.module_path.BaseName().AsUTF8Unsafe());
-
- switch (event.event_type) {
- case ModuleWatcher::ModuleEventType::kModuleAlreadyLoaded: {
- // kModuleAlreadyLoaded comes from the enumeration of loaded modules
- // using CreateToolhelp32Snapshot().
- uint32_t time_date_stamp = 0;
- if (TryGetModuleTimeDateStamp(event.module_load_address,
- event.module_path, event.module_size,
- &time_date_stamp)) {
- ModuleDatabase::HandleModuleLoadEvent(
- content::PROCESS_TYPE_BROWSER, event.module_path,
- event.module_size, time_date_stamp);
- } else {
- // Failed to get the TimeDateStamp directly from memory. The next step
- // to try is to read the file on disk. This must be done in a blocking
- // task.
- base::ThreadPool::PostTask(
- FROM_HERE,
- {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
- base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
- base::BindOnce(&HandleModuleLoadEventWithoutTimeDateStamp,
- event.module_path, event.module_size));
- }
- break;
- }
- case ModuleWatcher::ModuleEventType::kModuleLoaded: {
- ModuleDatabase::HandleModuleLoadEvent(
- content::PROCESS_TYPE_BROWSER, event.module_path, event.module_size,
- GetModuleTimeDateStamp(event.module_load_address));
- break;
- }
- }
- }
- // Since OnModuleEvent can be invoked from any thread, the above trace event's
- // END might be the last event on this thread, emit an empty event to force
- // the END to be flushed. TODO(crbug.com/1021571): Remove this once fixed.
- PERFETTO_INTERNAL_ADD_EMPTY_EVENT();
-}
-
-// Helper function for initializing the module database subsystem and populating
-// the provided |module_watcher|.
-void SetupModuleDatabase(std::unique_ptr<ModuleWatcher>* module_watcher) {
- DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
- DCHECK(module_watcher);
-
- bool third_party_blocking_policy_enabled =
-#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
- ModuleDatabase::IsThirdPartyBlockingPolicyEnabled();
-#else
- false;
-#endif
-
- ModuleDatabase::GetTaskRunner()->PostTask(
- FROM_HERE, base::BindOnce(&InitializeModuleDatabase,
- third_party_blocking_policy_enabled));
-
- *module_watcher = ModuleWatcher::Create(base::BindRepeating(&OnModuleEvent));
-}
-
void ShowCloseBrowserFirstMessageBox() {
chrome::ShowWarningMessageBox(
nullptr, l10n_util::GetStringUTF16(IDS_PRODUCT_NAME),
@@ -663,6 +598,18 @@
.get());
#endif
+#if DCHECK_IS_ON()
+ // Patching EAT of kernel32.dll is only supported on 32-bit because RVA can
+ // only hold 32-bit values.
+#if defined(ARCH_CPU_32_BITS)
+ base::debug::HandleHooks::AddEATPatch();
+#endif
+ // Patch currently loaded modules. Future ones will get patched by the module
+ // watcher. Note: if any modules load between now and when SetupModuleDatabase
+ // is called then these will be missed.
+ base::debug::HandleHooks::PatchLoadedModules();
+#endif // DCHECK_IS_ON()
+
// Create the module database and hook up the in-process module watcher. This
// needs to be done before any child processes are initialized as the
// ModuleDatabase is an endpoint for IPC from child processes.
@@ -934,3 +881,82 @@
// duplicates, perhaps harmonize with switches::RemoveSwitchesForAutostart.
return restart_command;
}
+
+// Used as the callback for ModuleWatcher events in this process. Dispatches
+// them to the ModuleDatabase.
+// Note: This callback may be invoked on any thread, even those not owned by the
+// task scheduler, under the loader lock, directly on the thread where the
+// DLL is currently loading.
+void ChromeBrowserMainPartsWin::OnModuleEvent(
+ const ModuleWatcher::ModuleEvent& event) {
+ {
+ TRACE_EVENT1("browser", "OnModuleEvent", "module_path",
+ event.module_path.BaseName().AsUTF8Unsafe());
+
+ switch (event.event_type) {
+ case ModuleWatcher::ModuleEventType::kModuleAlreadyLoaded: {
+ // kModuleAlreadyLoaded comes from the enumeration of loaded modules
+ // using CreateToolhelp32Snapshot().
+ uint32_t time_date_stamp = 0;
+ if (TryGetModuleTimeDateStamp(event.module_load_address,
+ event.module_path, event.module_size,
+ &time_date_stamp)) {
+ ModuleDatabase::HandleModuleLoadEvent(
+ content::PROCESS_TYPE_BROWSER, event.module_path,
+ event.module_size, time_date_stamp);
+ } else {
+ // Failed to get the TimeDateStamp directly from memory. The next step
+ // to try is to read the file on disk. This must be done in a blocking
+ // task.
+ base::ThreadPool::PostTask(
+ FROM_HERE,
+ {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
+ base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+ base::BindOnce(&HandleModuleLoadEventWithoutTimeDateStamp,
+ event.module_path, event.module_size));
+ }
+ break;
+ }
+ case ModuleWatcher::ModuleEventType::kModuleLoaded: {
+#if DCHECK_IS_ON() && defined(ARCH_CPU_64_BITS)
+ // This is only needed on 64-bit because on 32-bit the EAT from kernel32
+ // is already patched. This is thread safe against itself as this is
+ // always called under loader lock.
+ HMODULE module =
+ reinterpret_cast<HMODULE>(event.module_load_address.get());
+ base::debug::HandleHooks::AddIATPatch(module);
+#endif // DCHECK_IS_ON() && defined(ARCH_CPU_64_BITS)
+ ModuleDatabase::HandleModuleLoadEvent(
+ content::PROCESS_TYPE_BROWSER, event.module_path, event.module_size,
+ GetModuleTimeDateStamp(event.module_load_address));
+ break;
+ }
+ }
+ }
+ // Since OnModuleEvent can be invoked from any thread, the above trace event's
+ // END might be the last event on this thread, emit an empty event to force
+ // the END to be flushed. TODO(crbug.com/1021571): Remove this once fixed.
+ PERFETTO_INTERNAL_ADD_EMPTY_EVENT();
+}
+
+// Helper function for initializing the module database subsystem and populating
+// the provided |module_watcher|.
+void ChromeBrowserMainPartsWin::SetupModuleDatabase(
+ std::unique_ptr<ModuleWatcher>* module_watcher) {
+ DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+ DCHECK(module_watcher);
+
+ bool third_party_blocking_policy_enabled =
+#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
+ ModuleDatabase::IsThirdPartyBlockingPolicyEnabled();
+#else
+ false;
+#endif
+
+ ModuleDatabase::GetTaskRunner()->PostTask(
+ FROM_HERE, base::BindOnce(&InitializeModuleDatabase,
+ third_party_blocking_policy_enabled));
+
+ *module_watcher = ModuleWatcher::Create(base::BindRepeating(
+ &ChromeBrowserMainPartsWin::OnModuleEvent, base::Unretained(this)));
+}
diff --git a/chrome/browser/chrome_browser_main_win.h b/chrome/browser/chrome_browser_main_win.h
index 66bcce9..87e802df 100644
--- a/chrome/browser/chrome_browser_main_win.h
+++ b/chrome/browser/chrome_browser_main_win.h
@@ -10,8 +10,7 @@
#include <memory>
#include "chrome/browser/chrome_browser_main.h"
-
-class ModuleWatcher;
+#include "chrome/common/conflicts/module_watcher_win.h"
namespace base {
class CommandLine;
@@ -77,6 +76,9 @@
const base::CommandLine& command_line);
private:
+ void OnModuleEvent(const ModuleWatcher::ModuleEvent& event);
+ void SetupModuleDatabase(std::unique_ptr<ModuleWatcher>* module_watcher);
+
// Watches module load events and forwards them to the ModuleDatabase.
std::unique_ptr<ModuleWatcher> module_watcher_;
};
