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..5856c8e400ba66093d3e4b1cf9a85d60b5e77745 100644
--- a/base/win/scoped_handle.cc
+++ b/base/win/scoped_handle.cc
@@ -7,11 +7,17 @@
 #include <unordered_map>
 
 #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();

[Will Harris, 2015/04/01 17:44:24]

won't this function end up being exported from all chrome modules linked with
base i.e. chrome.dll chrome_child.dll and chrome.exe - don't you just want it
exported from chrome.exe?

[rvargas (doing something else), 2015/04/01 18:23:53]

sorta, kind of yes... I mean, I'm fine with every module exporting the function
because... what I was really looking for is a general way to make sure that all
modules (not just the main dll and the exe) are connected, without having to add
logic at the embedder layer (aka, unknown_foo.dll!main?) to locate one specific
module.

That's why PS1 (intentionally here) shows an sketch of not having an export and
relying in something dirtier: something on the environment. Which means the
"master" is whoever calls this code first!.

Anyways, in practice though, for the foreseeable future we'll have uses of
ScopedHanlde in the exe (the sandbox code), and that means that the first module
to look for this thing is the exe, which means that other modules can assume
that the exe is the master (although to be fair, I'd prefer the master to be
chrome.dll), and so they can just get the hmodule of the exe.

So yes, we could add logic to prevent other modules from exporting this, but I'm
not sure it's worth it (especially because base should not really be aware of
how many modules the app have, and adding callbacks to configure this from the
embedder is not that nice). But yes, this also has the implied assumption that
the exe is the first to come up :(

+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,180 @@ 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:
+  explicit ActiveVerifier(bool enabled)
+      : 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. They are virtual because we need to
+  // forward the call execution to another module, instead of letting the
+  // compiler call the version that is linked in the current module.
+  virtual bool CloseHandle(HANDLE handle);
+  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();  // Not implemented.
+
+  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);

[Will Harris, 2015/04/01 17:44:24]

to confirm you want to call the exe's version of GetHandleVerifier if you're
inside a DLL?

if so, I think this call to GetModuleHandle will never fail, so the call on line
111 will never happen?  Unless I'm missing something here.

[rvargas (doing something else), 2015/04/01 18:23:53]

Correct. I'm just being extremely cautious about error handling. I could DCHECK
(or CHECK?) if you want me to. (that _should_ be more of a build problem... for
instance moving the fn definition to a namespace)

[Will Harris, 2015/04/02 18:11:15]

is it not possible for two modules to both create their own handle verifiers

e.g. module A starts and calls ActiveVerifier::InstallVerifier at the same time
module B starts and calls ActiveVerifier::InstallVerifier.

They each have their own private instance of g_lock so they both enter the code
and then query the main module, both get the answer NULL back, and both create
their own ActiveVerifier?

[rvargas (doing something else), 2015/04/02 18:25:23]

Correct.

The lock here (118) is meant to protect threads running on the same module
(whatever that means), not another thread in another module. I'm fine with that
race (that particular client will not report everything it could).

Patch Set 1 has a global lock that makes sure all modules are completely in
sync, but I don't think it's worth doing that here. In practice, the exe creates
the verifier before launching the first dll (chrome).

+  GetHandleVerifierFn get_handle_verifier =
+      reinterpret_cast<GetHandleVerifierFn>(::GetProcAddress(
+          main_module, "GetHandleVerifier"));
+
+  if (!get_handle_verifier) {
+    g_active_verifier = new ActiveVerifier(false);
+    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);

[cpu_(ooo_6.6-7.5), 2015/04/02 20:49:10]

so we always do the pattern of 212-123

How about we do

     ActiveVerifier::Get()->StartTracking(handle, owner, pc1, pc2)

and 
    ActiveVerifier::Get()->StopTracking(handle, owner, pc1, pc2)

and we move the if() logic inside the get?

[rvargas (doing something else), 2015/04/02 21:26:31]

Done.

+}
+
+// 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