Add HANDLE_FLAG_PROTECT_FROM_CLOSE flag to Tracked/ScopedHandle.

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>
 
 #include "base/debug/alias.h"
 #include "base/hash.h"
@@ skipping 114 matching lines @@
     g_active_verifier = new ActiveVerifier(false);
     return;
   }
 
   ActiveVerifier* verifier =
       reinterpret_cast<ActiveVerifier*>(get_handle_verifier());
 
   // 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
 }
 
 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 };
+
+  // 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) {
     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);
   }
 
+  // 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);
 }
 
 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;
 
+  // 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);
 }
 
 }  // namespace
 
 void* GetHandleVerifier() {
   return g_active_verifier;
 }
@@ skipping 21 matching lines @@
 void DisableHandleVerifier() {
   return ActiveVerifier::Get()->Disable();
 }
 
 void OnHandleBeingClosed(HANDLE handle) {
   return ActiveVerifier::Get()->OnHandleBeingClosed(handle);
 }
 
 }  // namespace win
 }  // namespace base