Integrate the handle verifier from multiple modules (DLLs)

base/win/scoped_handle.cc

 // Copyright (c) 2012 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 "base/win/scoped_handle.h"
 
 #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 {
   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::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))
     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);
 };
 
+// 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()) {
+  }
+
+  static void InstallVerifier();
+
+  // 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_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);
+  closing_ = false;
+
+  return true;
+}
+
+void ActiveVerifier::StartTracking(HANDLE handle, const void* owner,
+                                   const void* pc1, const void* pc2) {
+  if (!enabled_)
+    return;
+
+  // Grab the thread id before the lock.
+  DWORD thread_id = GetCurrentThreadId();
+
+  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 = map_.insert(item);
+  if (!result.second) {
+    Info other = result.first->second;
+    base::debug::Alias(&other);
+    CHECK(false);
+  }
+}
+
+void ActiveVerifier::StopTracking(HANDLE handle, const void* owner,
+                                  const void* pc1, const void* pc2) {
+  if (!enabled_)
+    return;
+
+  AutoNativeLock lock(*lock_);
+  HandleMap::iterator i = map_.find(handle);
+  if (i == map_.end())
+    CHECK(false);
+
+  Info other = i->second;
+  if (other.owner != owner) {
+    base::debug::Alias(&other);
+    CHECK(false);
+  }
+
+  map_.erase(i);
+}
+
+void ActiveVerifier::Disable() {
+  enabled_ = false;
+}
+
+void ActiveVerifier::OnHandleBeingClosed(HANDLE handle) {
+  AutoNativeLock lock(*lock_);
+  if (closing_)
+    return;
+
+  HandleMap::iterator i = map_.find(handle);
+  if (i == map_.end())
+    return;
+
+  Info other = i->second;
+  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_verifier_enabled)
-    return CloseHandleWrapper(handle);
+  if (!g_active_verifier)
+    ActiveVerifier::InstallVerifier();
 
-  AutoNativeLock lock(g_lock.Get());
-  g_closing = true;
-  CloseHandleWrapper(handle);
-  g_closing = false;
-
-  return true;
+  return g_active_verifier->CloseHandle(handle);
 }
 
 // Static.
 void VerifierTraits::StartTracking(HANDLE handle, const void* owner,
                                    const void* pc1, const void* pc2) {
-  if (!g_verifier_enabled)
-    return;
+  if (!g_active_verifier)
+    ActiveVerifier::InstallVerifier();
 
-  // Grab the thread id before the lock.
-  DWORD thread_id = GetCurrentThreadId();
-
-  AutoNativeLock lock(g_lock.Get());
-
-  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);
-  if (!result.second) {
-    Info other = result.first->second;
-    debug::Alias(&other);
-    CHECK(false);
-  }
+  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_verifier_enabled)
-    return;
+  if (!g_active_verifier)
+    ActiveVerifier::InstallVerifier();
 
-  AutoNativeLock lock(g_lock.Get());
-  HandleMap::iterator i = g_handle_map.Get().find(handle);
-  if (i == g_handle_map.Get().end())
-    CHECK(false);
-
-  Info other = i->second;
-  if (other.owner != owner) {
-    debug::Alias(&other);
-    CHECK(false);
-  }
-
-  g_handle_map.Get().erase(i);
+  return g_active_verifier->StopTracking(handle, owner, pc1, pc2);
 }
 
 void DisableHandleVerifier() {
-  g_verifier_enabled = false;
+  if (!g_active_verifier)
+    ActiveVerifier::InstallVerifier();
+
+  return g_active_verifier->Disable();
 }
 
 void OnHandleBeingClosed(HANDLE handle) {
-  AutoNativeLock lock(g_lock.Get());
-  if (g_closing)
-    return;
+  if (!g_active_verifier)
+    ActiveVerifier::InstallVerifier();
 
-  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);
+  return g_active_verifier->OnHandleBeingClosed(handle);
 }
 
 }  // namespace win
 }  // namespace base