Integrate the handle verifier from multiple modules (DLLs)

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 280302169209b637129fe85e6cbbb15fc9854e23..3f6d3e61e25a5e3e28bf682e42dbbc2a39c986c3 100644
--- a/base/win/scoped_handle.cc
+++ b/base/win/scoped_handle.cc
@@ -7,11 +7,17 @@
 #include <unordered_map>

[grt (UTC plus 2), 2015/04/01 15:33:04]

would you mind switching from unordered_map to "base/containers/hash_tables.h"
and base::hash_map? They amount to the same thing, but i think it's to our
advantage to not have fragmentation in the codebase.

[rvargas (doing something else), 2015/04/01 16:51:03]

This was used here because of inconsistencies in our pre-standard implementation
that basically broke for int64 values. Maybe that is fixed now, but updating it
is independent of the issue being fixed here.

[grt (UTC plus 2), 2015/04/01 20:33:02]

That ain't good.
Fair enough.

 
 #include "base/debug/alias.h"
+#include "base/environment.h"
 #include "base/hash.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/synchronization/lock_impl.h"
 
+extern "C" {
+__declspec(dllexport) void* GetHandleVerifier();
+typedef void* (*GetHandleVerifierFn)();
+}
+
 namespace {
 
 struct HandleHash {
@@ -30,20 +36,9 @@ struct Info {
 };
 typedef std::unordered_map<HANDLE, Info, HandleHash> HandleMap;
 
-// g_lock protects g_handle_map and g_closing.
+// g_lock protects the handle map and setting g_active_verifier.
 typedef base::internal::LockImpl NativeLock;
 base::LazyInstance<NativeLock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
-base::LazyInstance<HandleMap>::Leaky g_handle_map = 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))
@@ -68,82 +63,178 @@ class AutoNativeLock {
   DISALLOW_COPY_AND_ASSIGN(AutoNativeLock);
 };
 
-}  // namespace
+// Implements the actual object that is verifying handles for this process.
+// The active instance is shared across the module boundary but there is no
+// way to delete this object from the wrong side of it (or any side, actually).
+class ActiveVerifier {
+ public:
+  ActiveVerifier(bool enabled)

[grt (UTC plus 2), 2015/04/01 15:33:04]

explicit

[rvargas (doing something else), 2015/04/01 16:51:03]

Acknowledged.

+      : enabled_(enabled), closing_(false), lock_(g_lock.Pointer()) {
+  }
 
-namespace base {
-namespace win {
+  static void InstallVerifier();
 
-// Static.
-bool HandleTraits::CloseHandle(HANDLE handle) {
-  if (!g_verifier_enabled)
-    return CloseHandleWrapper(handle);
+  // The methods required by HandleTraits:
+  virtual bool CloseHandle(HANDLE handle);

[grt (UTC plus 2), 2015/04/01 15:33:04]

why virtual? if it's for a subtle "because we need a vtable" reason, please
document it.

[rvargas (doing something else), 2015/04/01 16:51:03]

Yes the whole point of the CL is that we need to forward the call to another
module and if we don't have a vtable then we end up executing the function that
is linked directly into the current module (bad thing). Will add comment.

[grt (UTC plus 2), 2015/04/01 20:33:02]

Yup, that's what I thought.
Thanks.

[grt (UTC plus 2), 2015/04/02 02:19:03]

Wait, why is that necessary? Are not all modules that link this in (chrome.exe,
chrome.dll) built at the same time, such that they have identical
implementations? If all state is held within the instance (as opposed to being
in module-specific global vars), why does it matter which copy of the assembly
instructions is used? From what I see, all state is accessed through the |this|
pointer, so none of these need to be virtual.

I have a vague sense of "I must be missing something", but I can't think of what
it is.

Why not re-frame the point of the CL so that the state established by the first
module is used throughout the process (rather than forwarding the calls around)?

[rvargas (doing something else), 2015/04/02 17:53:57]

Sortof. All the state is inside the object, but there is a map that does memory
allocations, and we cannot allow the memory pools to be mixed (allocated from
the exe, then reallocated from the dll, for example), because that will corrupt
everything. (Which is also the reason there's no way to delete the object).

I could go to export pure c code instead, but I figured it is better to have a
single export and handle the other required "exports" with the vtable.

+  virtual void StartTracking(HANDLE handle, const void* owner,
+                             const void* pc1, const void* pc2);
+  virtual void StopTracking(HANDLE handle, const void* owner,
+                            const void* pc1, const void* pc2);
+  virtual void Disable();
+  virtual void OnHandleBeingClosed(HANDLE handle);
+
+ private:
+  ~ActiveVerifier();

[grt (UTC plus 2), 2015/04/01 15:33:04]

if the methods above truly need to be virtual, make this virtual as well.

[rvargas (doing something else), 2015/04/01 16:51:03]

There's no destructor implementation.

[grt (UTC plus 2), 2015/04/01 20:33:02]

Ah, so there isn't. Ack.

+
+  bool enabled_;
+  bool closing_;
+  NativeLock* lock_;
+  HandleMap map_;
+  DISALLOW_COPY_AND_ASSIGN(ActiveVerifier);
+};
+ActiveVerifier* g_active_verifier = NULL;
+
+// static
+void ActiveVerifier::InstallVerifier() {
+#if defined(COMPONENT_BUILD)
+  AutoNativeLock lock(g_lock.Get());
+  g_active_verifier = new ActiveVerifier(true);
+#else
+  HMODULE main_module = ::GetModuleHandle(NULL);
+  GetHandleVerifierFn get_handle_verifier =
+      reinterpret_cast<GetHandleVerifierFn>(::GetProcAddress(
+          main_module, "GetHandleVerifier"));
+
+  if (!get_handle_verifier) {
+      g_active_verifier = new ActiveVerifier(false);

[grt (UTC plus 2), 2015/04/01 15:33:04]

indentation

[rvargas (doing something else), 2015/04/01 16:51:03]

... I saw that while testing and I forgot to fix it. Oh well.

+    return;
+  }
+
+  ActiveVerifier* verifier =
+      reinterpret_cast<ActiveVerifier*>(get_handle_verifier());
 
   AutoNativeLock lock(g_lock.Get());
-  g_closing = true;
+  g_active_verifier = verifier ? verifier : new ActiveVerifier(true);
+#endif
+}
+
+bool ActiveVerifier::CloseHandle(HANDLE handle) {
+  if (!enabled_)
+    return CloseHandleWrapper(handle);
+
+  AutoNativeLock lock(*lock_);
+  closing_ = true;
   CloseHandleWrapper(handle);
-  g_closing = false;
+  closing_ = false;
 
   return true;
 }
 
-// Static.
-void VerifierTraits::StartTracking(HANDLE handle, const void* owner,
+void ActiveVerifier::StartTracking(HANDLE handle, const void* owner,
                                    const void* pc1, const void* pc2) {
-  if (!g_verifier_enabled)
+  if (!enabled_)
     return;
 
   // Grab the thread id before the lock.
   DWORD thread_id = GetCurrentThreadId();
 
-  AutoNativeLock lock(g_lock.Get());
+  AutoNativeLock lock(*lock_);
 
   Info handle_info = { owner, pc1, pc2, thread_id };
   std::pair<HANDLE, Info> item(handle, handle_info);
-  std::pair<HandleMap::iterator, bool> result = g_handle_map.Get().insert(item);
+  std::pair<HandleMap::iterator, bool> result = map_.insert(item);
   if (!result.second) {
     Info other = result.first->second;
-    debug::Alias(&other);
+    base::debug::Alias(&other);
     CHECK(false);
   }
 }
 
-// Static.
-void VerifierTraits::StopTracking(HANDLE handle, const void* owner,
+void ActiveVerifier::StopTracking(HANDLE handle, const void* owner,
                                   const void* pc1, const void* pc2) {
-  if (!g_verifier_enabled)
+  if (!enabled_)
     return;
 
-  AutoNativeLock lock(g_lock.Get());
-  HandleMap::iterator i = g_handle_map.Get().find(handle);
-  if (i == g_handle_map.Get().end())
+  AutoNativeLock lock(*lock_);
+  HandleMap::iterator i = map_.find(handle);
+  if (i == map_.end())
     CHECK(false);
 
   Info other = i->second;
   if (other.owner != owner) {
-    debug::Alias(&other);
+    base::debug::Alias(&other);
     CHECK(false);
   }
 
-  g_handle_map.Get().erase(i);
+  map_.erase(i);
 }
 
-void DisableHandleVerifier() {
-  g_verifier_enabled = false;
+void ActiveVerifier::Disable() {
+  enabled_ = false;
 }
 
-void OnHandleBeingClosed(HANDLE handle) {
-  AutoNativeLock lock(g_lock.Get());
-  if (g_closing)
+void ActiveVerifier::OnHandleBeingClosed(HANDLE handle) {
+  AutoNativeLock lock(*lock_);
+  if (closing_)
     return;
 
-  HandleMap::iterator i = g_handle_map.Get().find(handle);
-  if (i == g_handle_map.Get().end())
+  HandleMap::iterator i = map_.find(handle);
+  if (i == map_.end())
     return;
 
   Info other = i->second;
-  debug::Alias(&other);
+  base::debug::Alias(&other);
   CHECK(false);
 }
 
+}  // namespace
+
+void* GetHandleVerifier() {
+  return g_active_verifier;
+}
+
+namespace base {
+namespace win {
+
+// Static.
+bool HandleTraits::CloseHandle(HANDLE handle) {
+  if (!g_active_verifier)
+    ActiveVerifier::InstallVerifier();
+
+  return g_active_verifier->CloseHandle(handle);
+}
+
+// Static.
+void VerifierTraits::StartTracking(HANDLE handle, const void* owner,
+                                   const void* pc1, const void* pc2) {
+  if (!g_active_verifier)
+    ActiveVerifier::InstallVerifier();
+
+  return g_active_verifier->StartTracking(handle, owner, pc1, pc2);
+}
+
+// Static.
+void VerifierTraits::StopTracking(HANDLE handle, const void* owner,
+                                  const void* pc1, const void* pc2) {
+  if (!g_active_verifier)
+    ActiveVerifier::InstallVerifier();
+
+  return g_active_verifier->StopTracking(handle, owner, pc1, pc2);
+}
+
+void DisableHandleVerifier() {
+  if (!g_active_verifier)
+    ActiveVerifier::InstallVerifier();
+
+  return g_active_verifier->Disable();
+}
+
+void OnHandleBeingClosed(HANDLE handle) {
+  if (!g_active_verifier)
+    ActiveVerifier::InstallVerifier();
+
+  return g_active_verifier->OnHandleBeingClosed(handle);
+}
+
 }  // namespace win
 }  // namespace base