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

See https://codereview.chromium.org/490043002/ for the initial review.

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

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

base/win/scoped_handle.cc

@@ -4,38 +4,98 @@
 
 #include "base/win/scoped_handle.h"
 
-#include <map>
+#include <unordered_map>
 
 #include "base/debug/alias.h"
+#include "base/hash.h"
 #include "base/lazy_instance.h"
-#include "base/synchronization/lock.h"
-#include "base/win/windows_version.h"
+#include "base/logging.h"
+#include "base/synchronization/lock_impl.h"
 
 namespace {
 
+struct HandleHash {
+  size_t operator()(const HANDLE& handle) const {
+    char buffer[sizeof(handle)];
+    memcpy(buffer, &handle, sizeof(handle));
+    return base::Hash(buffer, sizeof(buffer));
+  }
+};
+
 struct Info {
   const void* owner;
   const void* pc1;
   const void* pc2;
   DWORD thread_id;
 };
-typedef std::map<HANDLE, Info> HandleMap;
+typedef std::unordered_map<HANDLE, Info, HandleHash> HandleMap;
 
+// g_lock protects g_handle_map and g_closing.
+typedef base::internal::LockImpl NativeLock;
+base::LazyInstance<NativeLock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
 base::LazyInstance<HandleMap>::Leaky g_handle_map = LAZY_INSTANCE_INITIALIZER;
-base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
+bool g_closing = false;
+
+// g_verifier_enabled is not protected by g_lock because that would require
+// using the lock (hence, synchornizing multiple threads) even when the
+// verifier is not in use. Note that this variable is initialized to track all
+// handles, and it should only move to the disabled state, and never back to
+// enabled, because that would crash when seeing handles created while the
+// verifier was disabled. This also implies that it is OK if the value change is
+// not propagated immediately to all CPUs (as would happen with a lock).
+bool g_verifier_enabled = true;
+
+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);
+};
 
 }  // 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();
 
-  AutoLock lock(g_lock.Get());
+  AutoNativeLock lock(g_lock.Get());
 
   Info handle_info = { owner, pc1, pc2, thread_id };
   std::pair<HANDLE, Info> item(handle, handle_info);
@@ -50,7 +110,10 @@ void VerifierTraits::StartTracking(HANDLE handle, const void* owner,
 // Static.
 void VerifierTraits::StopTracking(HANDLE handle, const void* owner,
                                   const void* pc1, const void* pc2) {
-  AutoLock lock(g_lock.Get());
+  if (!g_verifier_enabled)
+    return;
+
+  AutoNativeLock lock(g_lock.Get());
   HandleMap::iterator i = g_handle_map.Get().find(handle);
   if (i == g_handle_map.Get().end())
     CHECK(false);
@@ -64,5 +127,23 @@ void VerifierTraits::StopTracking(HANDLE handle, const void* owner,
   g_handle_map.Get().erase(i);
 }
 
+void DisableHandleVerifier() {
+  g_verifier_enabled = false;
+}
+
+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

@@ -101,9 +101,7 @@ class GenericScopedHandle {
       Verifier::StopTracking(handle_, this, BASE_WIN_GET_CALLER,
                              tracked_objects::GetProgramCounter());
 
-      if (!Traits::CloseHandle(handle_))
-        CHECK(false);
-
+      Traits::CloseHandle(handle_);
       handle_ = Traits::NullHandle();
     }
   }
@@ -120,9 +118,7 @@ class HandleTraits {
   typedef HANDLE Handle;
 
   // Closes the handle.
-  static bool CloseHandle(HANDLE handle) {
-    return ::CloseHandle(handle) != FALSE;
-  }
+  static bool BASE_EXPORT CloseHandle(HANDLE handle);
 
   // Returns true if the handle value is valid.
   static bool IsHandleValid(HANDLE handle) {
@@ -168,6 +164,17 @@ class BASE_EXPORT VerifierTraits {
 
 typedef GenericScopedHandle<HandleTraits, VerifierTraits> ScopedHandle;
 
+// This function may be called by the embedder to disable the use of
+// VerifierTraits at runtime. It has no effect if DummyVerifierTraits is used
+// for ScopedHandle.
+void BASE_EXPORT DisableHandleVerifier();
+
+// 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

@@ -129,6 +129,8 @@ 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_delegate.cc

@@ -46,6 +46,7 @@
 #include <atlbase.h>
 #include <malloc.h>
 #include <algorithm>
+#include "chrome/app/close_handle_hook_win.h"
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/terminate_on_heap_corruption_experiment_win.h"
 #include "sandbox/win/src/sandbox.h"
@@ -418,6 +419,8 @@ bool ChromeMainDelegate::BasicStartupComplete(int* exit_code) {
     *exit_code = 1;
     return true;
   }
+
+  InstallCloseHandleHooks();
 #endif
 
   chrome::RegisterPathProvider();
@@ -839,6 +842,10 @@ void ChromeMainDelegate::ProcessExiting(const std::string& process_type) {
   // Android doesn't use InitChromeLogging, so we close the log file manually.
   logging::CloseLogFile();
 #endif  // !defined(OS_ANDROID)
+
+#if defined(OS_WIN)
+  RemoveCloseHandleHooks();
+#endif
 }
 
 #if defined(OS_MACOSX)

chrome/app/close_handle_hook_win.cc

@@ -0,0 +1,118 @@
+// 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()) {
+    CloseHandleHooks* hooks = g_hooks.Pointer();
+    hooks->AddIATPatch(L"chrome.exe");
+    hooks->AddIATPatch(GetModuleName(GetChromeDLLModule()));
+  } else {
+    base::win::DisableHandleVerifier();
+  }
+}
+
+void RemoveCloseHandleHooks() {
+  g_hooks.Get().Unpatch();
+}

chrome/app/close_handle_hook_win.h

@@ -0,0 +1,14 @@
+// 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_