Add HANDLE_FLAG_PROTECT_FROM_CLOSE flag to Tracked/ScopedHandle.

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 33a8aa5c743b7b27eab6349fa1f0ee953dca6909..c24afd55dd79831dd96769ae8ea834913779513e 100644
--- a/base/win/scoped_handle.cc
+++ b/base/win/scoped_handle.cc
@@ -132,10 +132,6 @@ void ActiveVerifier::InstallVerifier() {
   // This lock only protects against races in this module, which is fine.
   AutoNativeLock lock(g_lock.Get());
   g_active_verifier = verifier ? verifier : new ActiveVerifier(true);
-
-  // TODO(shrikant): Enable handle verifier after figuring out
-  // AppContainer/DuplicateHandle error.
-  g_active_verifier->Disable();

[rvargas (doing something else), 2015/04/30 23:57:17]

We should not be disabling the verifier on all platforms to deal with a
_regression_ on (the verified code on) one platform. :(

[Shrikant Kelkar, 2015/05/01 00:57:31]

Acknowledged.

 #endif
 }
 
@@ -162,6 +158,11 @@ void ActiveVerifier::StartTracking(HANDLE handle, const void* owner,
   AutoNativeLock lock(*lock_);
 
   Info handle_info = { owner, pc1, pc2, thread_id };
+
+  // Idea here is to make our handles non-closable until we close it ourselves.
+  ::SetHandleInformation(handle, HANDLE_FLAG_PROTECT_FROM_CLOSE,
+                         HANDLE_FLAG_PROTECT_FROM_CLOSE);

[cpu_(ooo_6.6-7.5), 2015/05/01 00:42:52]

this can be done outside the lock.

[Shrikant Kelkar, 2015/05/01 00:57:31]

Done.

+

[Will Harris, 2015/05/01 00:03:45]

I don't think you can call SetHandleInformation on all handle types for example
you can't on a DC, it seems?

This might break ScopedCreateDC and others...?

https://msdn.microsoft.com/en-us/library/windows/desktop/ms724935.aspx

[cpu_(ooo_6.6-7.5), 2015/05/01 00:42:51]

Hopefully they are not using ScopedHandle because we call CloseHandle() and the
gdi/user32 handles cannot be closed this way.

[Will Harris, 2015/05/03 21:55:19]

ScopedCreateDC is specifically using VerifierTraits which means it's opting into
handle tracking.

https://code.google.com/p/chromium/codesearch#chromium/src/base/win/scoped_hd...

[rvargas (doing something else), 2015/05/06 01:47:23]

That's wrong, isn't it? the verifier doesn't work with anything that is not a
kernel handle (not win32k), and it may generate crashes when the actual value
overlaps.

   std::pair<HANDLE, Info> item(handle, handle_info);
   std::pair<HandleMap::iterator, bool> result = map_.insert(item);
   if (!result.second) {
@@ -187,6 +188,15 @@ void ActiveVerifier::StopTracking(HANDLE handle, const void* owner,
     CHECK(false);
   }
 
+  // We expect handle to be protected till this point.
+  DWORD flags = 0;
+  ::GetHandleInformation(handle, &flags);
+  if (!(flags & HANDLE_FLAG_PROTECT_FROM_CLOSE))
+    CHECK(FALSE);
+
+  // Unprotect handle so that it could be closed.
+  ::SetHandleInformation(handle, HANDLE_FLAG_PROTECT_FROM_CLOSE, 0);
+
   map_.erase(i);
 }
 
@@ -203,6 +213,15 @@ void ActiveVerifier::OnHandleBeingClosed(HANDLE handle) {
   if (i == map_.end())
     return;
 
+  // Mask out all protected handle close attempts. This will give us
+  // idea if protecting handle has any effect or not.
+  // TODO(shrikant): Remove this code once we see results of the above
+  // mentioned experiment.
+  DWORD flags = 0;
+  ::GetHandleInformation(handle, &flags);
+  if (flags & HANDLE_FLAG_PROTECT_FROM_CLOSE)
+    return;

[rvargas (doing something else), 2015/04/30 23:57:16]

hold on... we should not be modifying the behavior of the system though this
interception, even if it is for an experiment. All third_party code goes through
here.

[Shrikant Kelkar, 2015/05/01 00:06:48]

??
This code will trigger only if we are about crash through CHECK()

[cpu_(ooo_6.6-7.5), 2015/05/01 00:42:51]

Not sure if we want this or not, I need to think more about this, but if we do,
it needs to be outside the lock. I am not sure what Rick means by modifying the
system behavior. Isn't it the whole point of the hook to do so?


My current thinking was more along the lines of only CHECKing a fraction of the
time in 227, say 1/3rd (at random)

[Shrikant Kelkar, 2015/05/01 00:57:31]

Acknowledged.

[rvargas (doing something else), 2015/05/06 01:47:23]

ah, right. I missed that this was after searching for the handle in the map. But
then I guess I don't get what this line tries to do: the problem right now is
that there are not enough crashes hitting line 227, and too many crashes that
apparently evade this interception and crash later on. It seems it adds more
uncertainty to the crash data.

+
   Info other = i->second;
   base::debug::Alias(&other);
   CHECK(false);