Improve the ScopedHandle verifier.

Improve the ScopedHandle verifier.

1. Automate the selection of the proper channel to enable the verifier.
   Now the code is enabled at runtime.

2. Switch to a hash_map to track handles.

3. Intercept CloseHandle to detect the code that is closing handles owned
   by ScopedHandles. The initial implementation only covers chrome.exe/dll,
   but the plan is to extend that in the future to all modules loaded in the
   process.

BUG=362176
R=cpu@chromium.org

[rvargas (doing something else), 2014-08-21 02:57:28]

PTAL

[cpu_(ooo_6.6-7.5), 2014-08-22 01:52:05]

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.c...
base/win/scoped_handle.cc:63: template <>
this branch is for nacl?

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.c...
base/win/scoped_handle.cc:67: return
base::Hash(reinterpret_cast<char*>(&int_handle), sizeof(int_handle));
shouldn't we be doing bit_cast<char[N]> or somesuch?

I might be having dejavu here.

https://codereview.chromium.org/490043002/diff/40001/chrome/app/close_handle_...
File chrome/app/close_handle_hook_win.cc (right):

https://codereview.chromium.org/490043002/diff/40001/chrome/app/close_handle_...
chrome/app/close_handle_hook_win.cc:113:
hooks->AddIATPatch(GetModuleName(GetChromeDLLModule()));
can we just instead 
AddIATPatch(L"chrome.dll") and "chrome_child.dll" and have the AddPatch handle
failure?

[rvargas (doing something else), 2014-08-22 02:10:32]

Thanks

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.c...
base/win/scoped_handle.cc:63: template <>
On 2014/08/22 01:52:04, cpu (slow to respond) wrote:
> this branch is for nacl?

Not really. It is for completeness if people are trying to use another compiler
(we only ack those two on base_hash)

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.c...
base/win/scoped_handle.cc:67: return
base::Hash(reinterpret_cast<char*>(&int_handle), sizeof(int_handle));
On 2014/08/22 01:52:05, cpu (slow to respond) wrote:
> shouldn't we be doing bit_cast<char[N]> or somesuch?
> 

Maybe? what is N? sizeof? I wanted to be explicit/cautious and that's why I have
two casts instead of <const char*>(&handle) and let the compiler merge them at
will.


I'm not really sure if GCC's aliasing warning will trigger here given that it is
sending the pointer to another function. I can remove the whole GCC part,
although it may be better to have an alias warning (if at all) than a cryptic
error about a missing operator.


> I might be having dejavu here.

https://codereview.chromium.org/490043002/diff/40001/chrome/app/close_handle_...
File chrome/app/close_handle_hook_win.cc (right):

https://codereview.chromium.org/490043002/diff/40001/chrome/app/close_handle_...
chrome/app/close_handle_hook_win.cc:113:
hooks->AddIATPatch(GetModuleName(GetChromeDLLModule()));
On 2014/08/22 01:52:05, cpu (slow to respond) wrote:
> can we just instead 
> AddIATPatch(L"chrome.dll") and "chrome_child.dll" and have the AddPatch handle
> failure?

Patch() does LoadLibrary so it will not fail. And we'll need GetModuleName()
anyway to patch all DLLs loaded on the process :)

[cpu_(ooo_6.6-7.5), 2014-08-22 22:10:21]

lets have the hash be pretty and then lgtm.

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.c...
base/win/scoped_handle.cc:63: template <>
On 2014/08/22 02:10:32, rvargas wrote:
> On 2014/08/22 01:52:04, cpu (slow to respond) wrote:
> > this branch is for nacl?
> 
> Not really. It is for completeness if people are trying to use another
compiler
> (we only ack those two on base_hash)

Acknowledged.

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.c...
base/win/scoped_handle.cc:67: return
base::Hash(reinterpret_cast<char*>(&int_handle), sizeof(int_handle));
On 2014/08/22 02:10:31, rvargas wrote:
> On 2014/08/22 01:52:05, cpu (slow to respond) wrote:
> > shouldn't we be doing bit_cast<char[N]> or somesuch?
> > 
> 
> Maybe? what is N? sizeof? I wanted to be explicit/cautious and that's why I
have

yes sizeof.

> two casts instead of <const char*>(&handle) and let the compiler merge them at
> will.
> 
> 
> I'm not really sure if GCC's aliasing warning will trigger here given that it
is
> sending the pointer to another function. I can remove the whole GCC part,
> although it may be better to have an alias warning (if at all) than a cryptic
> error about a missing operator.
> 
> 
> > I might be having dejavu here.
> 

I think we should use bit_cast and have a single path, is that possible, like
using struct hash?

Another option is that there seems to be some handy macros to has 32 and 64 bit
values which we could use when we cast to ULONG_PTR.

https://codereview.chromium.org/490043002/diff/40001/chrome/app/close_handle_...
File chrome/app/close_handle_hook_win.cc (right):

https://codereview.chromium.org/490043002/diff/40001/chrome/app/close_handle_...
chrome/app/close_handle_hook_win.cc:113:
hooks->AddIATPatch(GetModuleName(GetChromeDLLModule()));
On 2014/08/22 02:10:32, rvargas wrote:
> On 2014/08/22 01:52:05, cpu (slow to respond) wrote:
> > can we just instead 
> > AddIATPatch(L"chrome.dll") and "chrome_child.dll" and have the AddPatch
handle
> > failure?
> 
> Patch() does LoadLibrary so it will not fail. And we'll need GetModuleName()
> anyway to patch all DLLs loaded on the process :)

Acknowledged.

[rvargas (doing something else), 2014-08-23 02:22:07]

Thanks

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://codereview.chromium.org/490043002/diff/40001/base/win/scoped_handle.c...
base/win/scoped_handle.cc:67: return
base::Hash(reinterpret_cast<char*>(&int_handle), sizeof(int_handle));
On 2014/08/22 22:10:20, cpu wrote:
> On 2014/08/22 02:10:31, rvargas wrote:
> > On 2014/08/22 01:52:05, cpu (slow to respond) wrote:
> > > shouldn't we be doing bit_cast<char[N]> or somesuch?
> > > 
> > 
> > Maybe? what is N? sizeof? I wanted to be explicit/cautious and that's why I
> have
> 
> yes sizeof.
> 
> > two casts instead of <const char*>(&handle) and let the compiler merge them
at
> > will.
> > 
> > 
> > I'm not really sure if GCC's aliasing warning will trigger here given that
it
> is
> > sending the pointer to another function. I can remove the whole GCC part,
> > although it may be better to have an alias warning (if at all) than a
cryptic
> > error about a missing operator.
> > 
> > 
> > > I might be having dejavu here.
> > 
> 
> I think we should use bit_cast and have a single path, is that possible, like
> using struct hash?
> 
> Another option is that there seems to be some handy macros to has 32 and 64
bit
> values which we could use when we cast to ULONG_PTR.

Done.

[cpu_(ooo_6.6-7.5), 2014-08-25 18:18:50]

still lgtm

[rvargas (doing something else), 2014-08-25 19:36:24]

rvargas@chromium.org changed reviewers:
+ sky@chromium.org

[rvargas (doing something else), 2014-08-25 19:36:24]

sky: The only file that lacks owner's approval is chrome_dll.gypi. Do you mind
taking a look?

[sky, 2014-08-25 19:58:49]

https://codereview.chromium.org/490043002/diff/80001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/490043002/diff/80001/chrome/chrome_dll.gypi#n...
chrome/chrome_dll.gypi:74: # GN version: //chrome:main_dll
Do you need to update gn files too?

https://codereview.chromium.org/490043002/diff/80001/chrome/chrome_dll.gypi#n...
chrome/chrome_dll.gypi:126: 'app/close_handle_hook_win.cc',
I think you should create a 'sources' section that is not conditional and put
these files there (that's the general pattern we do with gyp files).

[rvargas (doing something else), 2014-08-25 21:25:48]

Thanks

https://codereview.chromium.org/490043002/diff/80001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/490043002/diff/80001/chrome/chrome_dll.gypi#n...
chrome/chrome_dll.gypi:74: # GN version: //chrome:main_dll
On 2014/08/25 19:58:49, sky wrote:
> Do you need to update gn files too?

Done. I was looking at chrome/app/build.gn and I missed chrome/build.gn (and
this line)

https://codereview.chromium.org/490043002/diff/80001/chrome/chrome_dll.gypi#n...
chrome/chrome_dll.gypi:126: 'app/close_handle_hook_win.cc',
On 2014/08/25 19:58:49, sky wrote:
> I think you should create a 'sources' section that is not conditional and put
> these files there (that's the general pattern we do with gyp files).

Done. Do you want me to add *.rc to the common sources and remove them for mac?

[sky, 2014-08-25 22:49:07]

LGTM

https://codereview.chromium.org/490043002/diff/80001/chrome/chrome_dll.gypi
File chrome/chrome_dll.gypi (right):

https://codereview.chromium.org/490043002/diff/80001/chrome/chrome_dll.gypi#n...
chrome/chrome_dll.gypi:126: 'app/close_handle_hook_win.cc',
On 2014/08/25 21:25:48, rvargas wrote:
> On 2014/08/25 19:58:49, sky wrote:
> > I think you should create a 'sources' section that is not conditional and
put
> > these files there (that's the general pattern we do with gyp files).
> 
> Done. Do you want me to add *.rc to the common sources and remove them for
mac?

If we do that for other rc files, yes.

[rvargas (doing something else), 2014-08-25 23:07:52]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-08-25 23:08:49]

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

[commit-bot: I haz the power, 2014-08-26 01:17:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-26 01:17:22]

Failed to commit the patch.

[rvargas (doing something else), 2014-08-26 18:03:40]

The CQ bit was checked by rvargas@chromium.org

[rvargas (doing something else), 2014-08-26 18:03:46]

The CQ bit was unchecked by rvargas@chromium.org

[rvargas (doing something else), 2014-08-26 18:07:26]

Patchset #5 (id:120001) has been deleted

[Nico, 2015-08-15 00:22:31]

On 2014/08/26 18:07:26, rvargas wrote:
> Patchset #5 (id:120001) has been deleted

Why did you delete the patch set that was landed? :-)

Looks like this adds an include of unordered_map – please don't use C++11
library features yet.

[rvargas (doing something else), 2015-08-15 00:31:27]

On 2015/08/15 00:22:31, Nico (hiding) wrote:
> On 2014/08/26 18:07:26, rvargas wrote:
> > Patchset #5 (id:120001) has been deleted
> 
> Why did you delete the patch set that was landed? :-)

This never landed. See https://codereview.chromium.org/490043002/ instead. I
have no idea why #18 says it landed (I think something was acting up and I
started from scratch)


> 
> Looks like this adds an include of unordered_map – please don't use C++11
> library features yet.

ah? where? who?

[rvargas (doing something else), 2015-08-15 00:31:59]

On 2015/08/15 00:31:27, rvargas wrote:
> On 2015/08/15 00:22:31, Nico (hiding) wrote:
> > On 2014/08/26 18:07:26, rvargas wrote:
> > > Patchset #5 (id:120001) has been deleted
> > 
> > Why did you delete the patch set that was landed? :-)
> 
> This never landed. See https://codereview.chromium.org/490043002/ instead. I
> have no idea why #18 says it landed (I think something was acting up and I
> started from scratch)

and by that I meant https://codereview.chromium.org/506013004/

> 
> 
> > 
> > Looks like this adds an include of unordered_map – please don't use
C++11
> > library features yet.
> 
> ah? where? who?

[rvargas (doing something else), 2015-08-15 00:52:20]

On 2015/08/15 00:31:59, rvargas wrote:
> On 2015/08/15 00:31:27, rvargas wrote:
> > On 2015/08/15 00:22:31, Nico (hiding) wrote:
> > > On 2014/08/26 18:07:26, rvargas wrote:
> > > > Patchset #5 (id:120001) has been deleted
> > > 
> > > Why did you delete the patch set that was landed? :-)
> > 
> > This never landed. See https://codereview.chromium.org/490043002/ instead. I
> > have no idea why #18 says it landed (I think something was acting up and I
> > started from scratch)
> 
> and by that I meant https://codereview.chromium.org/506013004/

What a mess... that was then reverted and landed as
https://codereview.chromium.org/510633002

... and at some point during that review it looks like for some reason I moved
from where I was going (containers/hash_tables.h) to unordered_map.

:(

I'm guessing you care about compiling this with clang?

> 
> > 
> > 
> > > 
> > > Looks like this adds an include of unordered_map – please don't use
> C++11
> > > library features yet.
> > 
> > ah? where? who?

[rvargas (doing something else), 2015-08-15 01:01:00]

On 2015/08/15 00:52:20, rvargas wrote:
> On 2015/08/15 00:31:59, rvargas wrote:
> > On 2015/08/15 00:31:27, rvargas wrote:
> > > On 2015/08/15 00:22:31, Nico (hiding) wrote:
> > > > On 2014/08/26 18:07:26, rvargas wrote:
> > > > > Patchset #5 (id:120001) has been deleted
> > > > 
> > > > Why did you delete the patch set that was landed? :-)
> > > 
> > > This never landed. See https://codereview.chromium.org/490043002/ instead.
I
> > > have no idea why #18 says it landed (I think something was acting up and I
> > > started from scratch)
> > 
> > and by that I meant https://codereview.chromium.org/506013004/
> 
> What a mess... that was then reverted and landed as
> https://codereview.chromium.org/510633002
> 
> ... and at some point during that review it looks like for some reason I moved
> from where I was going (containers/hash_tables.h) to unordered_map.
> 
> :(
> 
> I'm guessing you care about compiling this with clang?
> 
> > 
> > > 
> > > 
> > > > 
> > > > Looks like this adds an include of unordered_map – please don't use
> > C++11
> > > > library features yet.
> > > 
> > > ah? where? who?

Some memories are starting to come back...

I believe
https://codereview.chromium.org/510633002/diff2/40001:60001/base/win/scoped_h...
is the result of PS3 failing on x64 because the hash function is never invoked
(something to do with HANDLE being a void* and some other hash being selected
before mine).

Do you mind opening a bug?