Improve the ScopedHandle verifier.

base/win/scoped_handle.cc

Index: base/win/scoped_handle.cc
diff --git a/base/win/scoped_handle.cc b/base/win/scoped_handle.cc
index 7b38369d5aa2706a8f7469f994ed1c1d48801846..be887af1bc5007075ef2fdcb1ae0d213174794e7 100644
--- a/base/win/scoped_handle.cc
+++ b/base/win/scoped_handle.cc
@@ -4,11 +4,12 @@
 
 #include "base/win/scoped_handle.h"
 
-#include <map>
-
+#include "base/containers/hash_tables.h"
 #include "base/debug/alias.h"
+#include "base/hash.h"
 #include "base/lazy_instance.h"
-#include "base/synchronization/lock.h"
+#include "base/logging.h"
+#include "base/synchronization/lock_impl.h"
 #include "base/win/windows_version.h"
 
 namespace {
@@ -19,23 +20,82 @@ struct Info {
   const void* pc2;
   DWORD thread_id;
 };
-typedef std::map<HANDLE, Info> HandleMap;
+typedef base::hash_map<HANDLE, Info> HandleMap;
 
+typedef base::internal::LockImpl NativeLock;
 base::LazyInstance<HandleMap>::Leaky g_handle_map = LAZY_INSTANCE_INITIALIZER;
-base::LazyInstance<base::Lock>::Leaky g_lock = 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);
+};
 
 }  // namespace
 
+namespace BASE_HASH_NAMESPACE {
+#if defined(COMPILER_MSVC)
+inline size_t hash_value(const HANDLE& handle) {
+  ULONG_PTR int_handle = reinterpret_cast<ULONG_PTR>(handle);
+  return base::Hash(reinterpret_cast<char*>(&int_handle), sizeof(int_handle));
+}
+#elif defined (COMPILER_GCC)
+template <>

[cpu_(ooo_6.6-7.5), 2014/08/22 01:52:04]

this branch is for nacl?

[rvargas (doing something else), 2014/08/22 02:10:32]

Not really. It is for completeness if people are trying to use another compiler
(we only ack those two on base_hash)

[cpu_(ooo_6.6-7.5), 2014/08/22 22:10:20]

Acknowledged.

+struct hash<HANDLE> {
+  size_t operator()(const HANDLE& key) const {
+    ULONG_PTR int_handle = reinterpret_cast<ULONG_PTR>(handle);
+    return base::Hash(reinterpret_cast<char*>(&int_handle), sizeof(int_handle));

[cpu_(ooo_6.6-7.5), 2014/08/22 01:52:05]

shouldn't we be doing bit_cast<char[N]> or somesuch?

I might be having dejavu here.

[rvargas (doing something else), 2014/08/22 02:10:31]

Maybe? what is N? sizeof? I wanted to be explicit/cautious and that's why I have
two casts instead of <const char*>(&handle) and let the compiler merge them at
will.


I'm not really sure if GCC's aliasing warning will trigger here given that it is
sending the pointer to another function. I can remove the whole GCC part,
although it may be better to have an alias warning (if at all) than a cryptic
error about a missing operator.

[cpu_(ooo_6.6-7.5), 2014/08/22 22:10:20]

yes sizeof.
I think we should use bit_cast and have a single path, is that possible, like
using struct hash?

Another option is that there seems to be some handy macros to has 32 and 64 bit
values which we could use when we cast to ULONG_PTR.

[rvargas (doing something else), 2014/08/23 02:22:07]

Done.

+  }
+};
+#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();
 
-  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 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