Base: Add a handle verifier to ScopedHandle.

Base: Add a handle verifier to ScopedHandle.
This provides basic tracking of handles for XP.

BUG=127931
TEST=none

TBR=abodenha, wez

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139736

[alexeypa (please no reviews), 2012-05-30 20:19:29]

A drive by, but I think this CL needs more work.

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle...
base/win/scoped_handle.cc:20: DWORD thread_id;
I think |pc| and |thread_id| can be hosted inside of GenericScopedHandle<T>
instances. See my other comment about LIST_ENTRY.

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle...
base/win/scoped_handle.cc:25: base::LazyInstance<std::set<const void*> >
g_owner_set =
|g_owner_set| is a way too heavy for the job it is doing. You could add
LIST_ENTRY to each instance of GenericScopedHandle<T> and link all of them
together. This will give you constant time on insertion and removal; and it will
does the same sort of check.

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle...
base/win/scoped_handle.cc:51: HandleMap::iterator i =
g_handle_map.Get().find(handle);
I think a simple linked list would be much simpler and more efficient. The
lookups can be accelerated by a bitmap or a bloom filter since all we need to
know if a value is in the set.

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle...
base/win/scoped_handle.cc:59: g_handle_map.Get()[handle] = handle_info;
This will do a second lookup in the map. it is better to use map::insert and
check the return value to see if the element exists in the map already.

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle.h
File base/win/scoped_handle.h (right):

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle...
base/win/scoped_handle.h:68: // We cannot track this case :(. Just tell the
verifier about it.
This is one of the most common ways of initializing GenericScopedHandle objects.

[alexeypa (please no reviews), 2012-05-30 20:26:37]

Oh, another thing. Won't Application Verifier
(http://www.microsoft.com/en-us/download/details.aspx?id=20028) give you similar
sort of checks? 

Sure it does not work with Chrome due to some issues with sandboxing than Chrome
uses, but I guess most of them go away if TLS checks are not enabled in
AppVerifier.

[rvargas (doing something else), 2012-05-30 21:39:01]

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle...
base/win/scoped_handle.cc:25: base::LazyInstance<std::set<const void*> >
g_owner_set =
On 2012/05/30 20:19:30, alexeypa wrote:
> |g_owner_set| is a way too heavy for the job it is doing. You could add
> LIST_ENTRY to each instance of GenericScopedHandle<T> and link all of them
> together. This will give you constant time on insertion and removal; and it
will
> does the same sort of check. 

Background: this is not a long term feature (it should not even get to a beta
channel, and maybe not even dev). It is just so that we can track the linked
bug... As of App verifier, we need data from the field, not on our tests (a
debug build would be enough in that case).

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle.h
File base/win/scoped_handle.h (right):

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle...
base/win/scoped_handle.h:68: // We cannot track this case :(. Just tell the
verifier about it.
On 2012/05/30 20:19:30, alexeypa wrote:
> This is one of the most common ways of initializing GenericScopedHandle
objects.

That is both unfortunate and surprising (given that this method was added three
months ago). I'm all ears if you have ideas for tracking this (short of removing
the method and changing callers to use a temp).

[rvargas (doing something else), 2012-05-30 22:12:33]

I'm not too concerned about overhead here... I could go to a hash table if you
guys think we should.

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle...
base/win/scoped_handle.cc:47: g_owner_set.Get().insert(owner);
I still hope this is not the common case.

https://chromiumcodereview.appspot.com/10453082/diff/1/base/win/scoped_handle...
base/win/scoped_handle.cc:59: g_handle_map.Get()[handle] = handle_info;
On 2012/05/30 20:19:30, alexeypa wrote:
> This will do a second lookup in the map. it is better to use map::insert and
> check the return value to see if the element exists in the map already.

Done.

[alexeypa (please no reviews), 2012-05-30 23:00:54]

Since this CL is only for tracking the bug my performance related comment can
probably be ignored. I have a bunch of other concerns though. :)

If you need is to know who is double-closing a handle it will be more effective
to keep a trace of ntdll!NtCloseHandle calls. Yes, it requires patching of the
NTDLL code but it will take care of Receive() case as well as of the case when
handle is closed by a rogue 3rd-party DLL.

Sandboxing code intercepts some of NTDLL functions, so I think we can borrow
from it.

The other problem (which is also is relevant to the current implementation of
this CL) is how you are going to store the trace (or |set| and |map|) to the
crash dump. Is there a way to tell that a particular region of memory should go
to the dump?

[rvargas (doing something else), 2012-05-30 23:48:31]

On 2012/05/30 23:00:54, alexeypa wrote:
> Since this CL is only for tracking the bug my performance related comment can
> probably be ignored. I have a bunch of other concerns though. :)
> 
> If you need is to know who is double-closing a handle it will be more
effective
> to keep a trace of ntdll!NtCloseHandle calls. Yes, it requires patching of the
> NTDLL code but it will take care of Receive() case as well as of the case when
> handle is closed by a rogue 3rd-party DLL.

Sure... I want to avoid patching for the time being. I don't want two problems
instead of one :)

> 
> Sandboxing code intercepts some of NTDLL functions, so I think we can borrow
> from it.
> 
> The other problem (which is also is relevant to the current implementation of
> this CL) is how you are going to store the trace (or |set| and |map|) to the
> crash dump. Is there a way to tell that a particular region of memory should
go
> to the dump?

the Alias() that I have close to the CHECKs do the trick. (I don't need all the
handles in the process).

[alexeypa (please no reviews), 2012-05-31 00:11:35]

On 2012/05/30 23:48:31, rvargas wrote:
> the Alias() that I have close to the CHECKs do the trick. (I don't need all
the
> handles in the process).

Oh, I forgot that we save memory referenced by the stack for dev builds.

On that note if you eventually decide to implement the CloseHandle trace, it can
be saved by allocating a few pages at the base of the stack on the main thread.
It cannot be per-thread since we don't control all threads, so locking will
still be necessary but we will be able to tolerate it.

[rvargas (doing something else), 2012-05-31 00:17:00]

Adding owner.

[cpu_(ooo_6.6-7.5), 2012-05-31 02:37:21]

lgtm as long is a temporary/short lived change.

[willchan no longer on Chromium, 2012-05-31 03:01:23]

lgtm as a temporary change too

https://chromiumcodereview.appspot.com/10453082/diff/12001/base/win/scoped_ha...
File base/win/scoped_handle.cc (right):

https://chromiumcodereview.appspot.com/10453082/diff/12001/base/win/scoped_ha...
base/win/scoped_handle.cc:24: base::LazyInstance<HandleMap> g_handle_map =
LAZY_INSTANCE_INITIALIZER;
Perhaps you should use a leaky lazy instance? Does this ever get used on a
thread that's not joined at shutdown? Actually wait, isn't that the problem at
hand? I think it'll be safer to use base::LazyInstance<T>::Leaky.

[rvargas (doing something else), 2012-05-31 03:13:08]

done. Thanks.

[commit-bot: I haz the power, 2012-05-31 03:16:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/10453082/11005

[commit-bot: I haz the power, 2012-05-31 05:04:23]

Change committed as 139736

[Nico, 2012-06-05 19:58:44]

https://chromiumcodereview.appspot.com/10453082/diff/11005/base/win/scoped_ha...
File base/win/scoped_handle.cc (right):

https://chromiumcodereview.appspot.com/10453082/diff/11005/base/win/scoped_ha...
base/win/scoped_handle.cc:43: AutoLock(g_lock.Get());
This produces a temporary that lives for only one line. You want this to be a
RAII object; this line needs to read 

  AutoLock lock(g_lock.Get());

Also in line 67. I'll send you a CL.

(found by clang! in a win-only file!)