Enable handle verifier for tests and add some tests.

Disable handle verifier hooks if running under drmemory.

BUG=571304
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_chromium_dbg_ng

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

Cr-Commit-Position: refs/heads/master@{#372877}

9 files changed, 132 insertions, 8 deletions

diff --git a/base/BUILD.gn b/base/BUILD.gn
index 7e5c458..2cb0efd 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -24,6 +24,12 @@ import("//build/config/ui.gni")
 import("//build/nocompile.gni")
 import("//testing/test.gni")
 
+declare_args() {
+  # Whether to disable the handle verifier hooks.
+  # Hookless parts of the handle verifier will still function.
+  win_disable_handle_verifier_hooks = false
+}
+
 if (is_android) {
   import("//build/config/android/rules.gni")
 }
@@ -1115,6 +1121,10 @@ component("base") {
 
     deps += [ "//base/trace_event/etw_manifest:chrome_events_win" ]
 
+    if (is_win && win_disable_handle_verifier_hooks) {
+      defines += [ "DISABLE_HANDLE_VERIFIER_HOOKS" ]
+    }
+
     if (is_component_build) {
       # Copy the VS runtime DLLs into the isolate so that they don't have to be
       # preinstalled on the target machine. The debug runtimes have a "d" at
@@ -1884,6 +1894,11 @@ test("base_unittests") {
       data += [ "$root_out_dir/base_unittests.dSYM/" ]
     }
   }
+
+  # This disables one of the handle verifier tests.
+  if (is_win && win_disable_handle_verifier_hooks) {
+    defines = [ "DISABLE_HANDLE_VERIFIER_HOOKS" ]
+  }
 }
 
 if (enable_nocompile_tests) {
diff --git a/base/base.gyp b/base/base.gyp
index 929a3e4..3e02c60 100644
--- a/base/base.gyp
+++ b/base/base.gyp
@@ -139,6 +139,11 @@
           ],
         }],
         ['OS == "win"', {
+          'conditions': [
+            ['win_disable_handle_verifier_hooks == 1', {
+              'defines': ['DISABLE_HANDLE_VERIFIER_HOOKS'],
+            }],
+          ],
           # Specify delayload for base.dll.
           'msvs_settings': {
             'VCLinkerTool': {
@@ -702,6 +707,9 @@
                 '../third_party/icu/icu.gyp:icudata',
               ],
             }],
+            ['win_disable_handle_verifier_hooks == 1', {
+                'defines': ['DISABLE_HANDLE_VERIFIER_HOOKS'],
+            }],
           ],
         }, {  # OS != "win"
           'dependencies': [
diff --git a/base/debug/close_handle_hook_win.cc b/base/debug/close_handle_hook_win.cc
index 6ff6fa2..0d617b5 100644
--- a/base/debug/close_handle_hook_win.cc
+++ b/base/debug/close_handle_hook_win.cc
@@ -17,6 +17,7 @@
 #include "base/win/iat_patch_function.h"
 #include "base/win/pe_image.h"
 #include "base/win/scoped_handle.h"
+#include "base/win/windows_version.h"
 #include "build/build_config.h"
 
 namespace {
@@ -259,12 +260,19 @@ void PatchLoadedModules(HandleHooks* hooks) {
 }  // namespace
 
 void InstallHandleHooks() {
+#if !defined(DISABLE_HANDLE_VERIFIER_HOOKS)
+#if defined(_DEBUG)
+  // Handle hooks cause shutdown asserts in Debug on Windows 7. crbug.com/571304
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
+    return;
+#endif
   HandleHooks* hooks = g_hooks.Pointer();
 
   // Performing EAT interception first is safer in the presence of other
   // threads attempting to call CloseHandle.
   hooks->AddEATPatch();
   PatchLoadedModules(hooks);
+#endif
 }
 
 void RemoveHandleHooks() {
diff --git a/base/test/test_suite.cc b/base/test/test_suite.cc
index 82510a2..2653e1f 100644
--- a/base/test/test_suite.cc
+++ b/base/test/test_suite.cc
@@ -40,7 +40,10 @@
 #endif  // OS_IOS
 #endif  // OS_MACOSX
 
-#if !defined(OS_WIN)
+#if defined(OS_WIN)
+#include "base/debug/close_handle_hook_win.h"
+#include "base/win/windows_version.h"
+#else
 #include "base/i18n/rtl.h"
 #if !defined(OS_IOS)
 #include "base/strings/string_util.h"
@@ -317,6 +320,8 @@ void TestSuite::Initialize() {
 
   CHECK(debug::EnableInProcessStackDumping());
 #if defined(OS_WIN)
+  base::debug::InstallHandleHooks();
+
   RouteStdioToConsole(true);
   // Make sure we run with high resolution timer to minimize differences
   // between production code and test code.
diff --git a/base/win/scoped_handle.cc b/base/win/scoped_handle.cc
index 9c21603..debe223 100644
--- a/base/win/scoped_handle.cc
+++ b/base/win/scoped_handle.cc
@@ -44,7 +44,7 @@ base::LazyInstance<NativeLock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
 
 bool CloseHandleWrapper(HANDLE handle) {
   if (!::CloseHandle(handle))
-    CHECK(false);
+    LOG(FATAL) << "CloseHandle failed.";
   return true;
 }
 
@@ -166,7 +166,7 @@ void ActiveVerifier::StartTracking(HANDLE handle, const void* owner,
   if (!result.second) {
     Info other = result.first->second;
     base::debug::Alias(&other);
-    CHECK(false);
+    LOG(FATAL) << "Attempt to start tracking already tracked handle.";
   }
 }
 
@@ -178,12 +178,12 @@ void ActiveVerifier::StopTracking(HANDLE handle, const void* owner,
   AutoNativeLock lock(*lock_);
   HandleMap::iterator i = map_.find(handle);
   if (i == map_.end())
-    CHECK(false);
+    LOG(FATAL) << "Attempting to close an untracked handle.";
 
   Info other = i->second;
   if (other.owner != owner) {
     base::debug::Alias(&other);
-    CHECK(false);
+    LOG(FATAL) << "Attempting to close a handle not owned by opener.";
   }
 
   map_.erase(i);
@@ -207,7 +207,7 @@ void ActiveVerifier::OnHandleBeingClosed(HANDLE handle) {
 
   Info other = i->second;
   base::debug::Alias(&other);
-  CHECK(false);
+  LOG(FATAL) << "CloseHandle called on tracked handle.";
 }
 
 }  // namespace
diff --git a/base/win/scoped_handle.h b/base/win/scoped_handle.h
index 404ab66..ac01485 100644
--- a/base/win/scoped_handle.h
+++ b/base/win/scoped_handle.h
@@ -8,6 +8,7 @@
 #include <windows.h>
 
 #include "base/base_export.h"
+#include "base/gtest_prod_util.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ -108,6 +109,8 @@ class GenericScopedHandle {
   }
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(ScopedHandleTest, ActiveVerifierWrongOwner);
+  FRIEND_TEST_ALL_PREFIXES(ScopedHandleTest, ActiveVerifierUntrackedHandle);
   Handle handle_;
 };
 
diff --git a/base/win/scoped_handle_unittest.cc b/base/win/scoped_handle_unittest.cc
index b573b66..b9663ef 100644
--- a/base/win/scoped_handle_unittest.cc
+++ b/base/win/scoped_handle_unittest.cc
@@ -2,10 +2,17 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <windows.h>
+#include <winternl.h>
+
 #include "base/win/scoped_handle.h"
+#include "base/win/windows_version.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace base {
+namespace win {
+
 TEST(ScopedHandleTest, ScopedHandle) {
   // Any illegal error code will do. We just need to test that it is preserved
   // by ScopedHandle to avoid bug 528394.
@@ -30,3 +37,75 @@ TEST(ScopedHandleTest, ScopedHandle) {
   handle_holder = handle_source.Pass();
   EXPECT_EQ(magic_error, ::GetLastError());
 }
+
+// This test requires that the CloseHandle hook be in place.
+#if !defined(DISABLE_HANDLE_VERIFIER_HOOKS)
+TEST(ScopedHandleTest, ActiveVerifierCloseTracked) {
+#if defined(_DEBUG)
+  // Handle hooks cause shutdown asserts in Debug on Windows 7. crbug.com/571304
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
+    return;
+#endif
+  HANDLE handle = ::CreateMutex(nullptr, FALSE, nullptr);
+  ASSERT_NE(HANDLE(NULL), handle);
+  ASSERT_DEATH({
+    base::win::ScopedHandle handle_holder(handle);
+    // Calling CloseHandle on a tracked handle should crash.
+    ::CloseHandle(handle);
+  }, "CloseHandle called on tracked handle.");
+}
+#endif
+
+TEST(ScopedHandleTest, ActiveVerifierTrackedHasBeenClosed) {
+  HANDLE handle = ::CreateMutex(nullptr, FALSE, nullptr);
+  ASSERT_NE(HANDLE(NULL), handle);
+  typedef NTSTATUS(WINAPI * NtCloseFunc)(HANDLE);
+  NtCloseFunc ntclose = reinterpret_cast<NtCloseFunc>(
+      GetProcAddress(GetModuleHandle(L"ntdll.dll"), "NtClose"));
+  ASSERT_NE(nullptr, ntclose);
+
+  ASSERT_DEATH({
+    base::win::ScopedHandle handle_holder(handle);
+    ntclose(handle);
+    // Destructing a ScopedHandle with an illegally closed handle should fail.
+  }, "CloseHandle failed.");
+}
+
+TEST(ScopedHandleTest, ActiveVerifierDoubleTracking) {
+  HANDLE handle = ::CreateMutex(nullptr, FALSE, nullptr);
+  ASSERT_NE(HANDLE(NULL), handle);
+
+  base::win::ScopedHandle handle_holder(handle);
+
+  ASSERT_DEATH({
+    base::win::ScopedHandle handle_holder2(handle);
+  }, "Attempt to start tracking already tracked handle.");
+}
+
+TEST(ScopedHandleTest, ActiveVerifierWrongOwner) {
+  HANDLE handle = ::CreateMutex(nullptr, FALSE, nullptr);
+  ASSERT_NE(HANDLE(NULL), handle);
+
+  base::win::ScopedHandle handle_holder(handle);
+  ASSERT_DEATH({
+    base::win::ScopedHandle handle_holder2;
+    handle_holder2.handle_ = handle;
+  }, "Attempting to close a handle not owned by opener.");
+  ASSERT_TRUE(handle_holder.IsValid());
+  handle_holder.Close();
+}
+
+TEST(ScopedHandleTest, ActiveVerifierUntrackedHandle) {
+  HANDLE handle = ::CreateMutex(nullptr, FALSE, nullptr);
+  ASSERT_NE(HANDLE(NULL), handle);
+
+  ASSERT_DEATH({
+    base::win::ScopedHandle handle_holder;
+    handle_holder.handle_ = handle;
+  }, "Attempting to close an untracked handle.");
+
+  ASSERT_TRUE(::CloseHandle(handle));
+}
+
+}  // namespace win
+}  // namespace base
diff --git a/build/common.gypi b/build/common.gypi
index 545eb24..c6aeca4 100644
--- a/build/common.gypi
+++ b/build/common.gypi
@@ -1589,6 +1589,10 @@
     'libjpeg_ijg_gyp_path': '<(DEPTH)/third_party/libjpeg/libjpeg.gyp',
     'libjpeg_turbo_gyp_path': '<(DEPTH)/third_party/libjpeg_turbo/libjpeg.gyp',
 
+    # Whether to disable handle verifier hooks.
+    # Hookless parts of the handle verifier will still function.
+    'win_disable_handle_verifier_hooks%': '0',
+
     'conditions': [
       ['buildtype=="Official"', {
         # Continue to embed build meta data in Official builds, basically the
@@ -2315,6 +2319,8 @@
       # TODO(rnk): Combine with tsan config to share the builder.
       # http://crbug.com/108155
       ['build_for_tool=="drmemory"', {
+        # Disable the hook based handle verifier, as DrMemory does this job.
+        'win_disable_handle_verifier_hooks': '1',
         # These runtime checks force initialization of stack vars which blocks
         # DrMemory's uninit detection.
         'win_debug_RuntimeChecks': '0',
diff --git a/chrome/app/chrome_main_delegate.cc b/chrome/app/chrome_main_delegate.cc
index e4fa69a..5f7d686 100644
--- a/chrome/app/chrome_main_delegate.cc
+++ b/chrome/app/chrome_main_delegate.cc
@@ -194,7 +194,7 @@ bool IsSandboxedProcess() {
   return is_sandboxed_process_func && is_sandboxed_process_func();
 }
 
-bool UseHooks() {
+bool UseHandleVerifier() {
 #if defined(ARCH_CPU_X86_64)
   return false;
 #elif defined(NDEBUG)
@@ -519,7 +519,7 @@ bool ChromeMainDelegate::BasicStartupComplete(int* exit_code) {
     return true;
   }
 
-  if (UseHooks())
+  if (UseHandleVerifier())
     base::debug::InstallHandleHooks();
   else
     base::win::DisableHandleVerifier();