Extend the CloseHandle interception to all DLLs loaded in the process.

Extend the CloseHandle interception to all DLLs loaded in the process.

Some of the problems found by this code can be attributed to activity on
DLLs that are not being monitored.

BUG=362176
R=cpu@chromium.org

Committed: https://crrev.com/ae02eef3fba47e4955be8b5c6e6e47c86b43f32d
Cr-Commit-Position: refs/heads/master@{#297464}

[rvargas (doing something else), 2014-09-23 18:02:22]

On 2014/09/19 23:02:21, rvargas wrote:

ping

[cpu_(ooo_6.6-7.5), 2014-09-26 01:10:01]

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
File chrome/app/close_handle_hook_win.cc (right):

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:69: if (!VirtualProtect(address, bytes,
protect, &old_protect_))
do we want to deal with non executable pages?

Given this class is local to patching code, I don't see why.

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:82: DCHECK(address_);
remove address_ check, put it in ChangeProtection.

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:85: VirtualProtect(address_, bytes_,
old_protect_, &old_protect_);
sholdn't we dcheck that this VP fails?

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:98: if (!pe.VerifyMagic())
I guess I am confused why this is a silent return vs other cases where we
dcheck.

[rvargas (doing something else), 2014-09-26 18:47:49]

PTAL

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
File chrome/app/close_handle_hook_win.cc (right):

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:69: if (!VirtualProtect(address, bytes,
protect, &old_protect_))
On 2014/09/26 01:10:01, cpu wrote:
> do we want to deal with non executable pages?
> 
> Given this class is local to patching code, I don't see why.

Yes because this depends on the structure of the dll that we are patching, not
on who uses it. An EAT can happily live on a non executable page... or not.

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:82: DCHECK(address_);
On 2014/09/26 01:10:01, cpu wrote:
> remove address_ check, put it in ChangeProtection.

Why? This verifies an invariant from this function: if it is called with
changed_ being true, there has to be an address_.

I can add another DCHECK to ChangeProtection if you want to verify that the
caller of that function passes an address (which we probably should).

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:85: VirtualProtect(address_, bytes_,
old_protect_, &old_protect_);
On 2014/09/26 01:10:01, cpu wrote:
> sholdn't we dcheck that this VP fails?

Maybe, but what do we gain? I don't like to DCHECK on the OS doing anything
because that's not under control of this code.

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:98: if (!pe.VerifyMagic())
On 2014/09/26 01:10:01, cpu wrote:
> I guess I am confused why this is a silent return vs other cases where we
> dcheck.

Because it's valid for the caller to pass an address that for some reason is not
a PE (EnumProcessModules returning a random mapped file?). We fail to patch it
and tell that to the caller. However, it is expected that the caller should
verify that the HMODULE it received is valid.

Having said that, the only caller is passing the result of
GetModuleHandleA("kernel32.dll") without checking anything, so maybe we should
remove the dcheck... This is basically the sandbox code slightly adapted so I
didn't think too much about it.

[cpu_(ooo_6.6-7.5), 2014-09-29 18:24:53]

more to come. Afternoon seems crap so here ya go.

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
File chrome/app/close_handle_hook_win.cc (right):

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:69: if (!VirtualProtect(address, bytes,
protect, &old_protect_))
On 2014/09/26 18:47:49, rvargas wrote:
> On 2014/09/26 01:10:01, cpu wrote:
> > do we want to deal with non executable pages?
> > 
> > Given this class is local to patching code, I don't see why.
> 
> Yes because this depends on the structure of the dll that we are patching, not
> on who uses it. An EAT can happily live on a non executable page... or not.

Acknowledged.

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:82: DCHECK(address_);
On 2014/09/26 18:47:49, rvargas wrote:
> On 2014/09/26 01:10:01, cpu wrote:
> > remove address_ check, put it in ChangeProtection.
> 
> Why? This verifies an invariant from this function: if it is called with
> changed_ being true, there has to be an address_.
> 
> I can add another DCHECK to ChangeProtection if you want to verify that the
> caller of that function passes an address (which we probably should).

because the only setter of address_ is ChangeProtection. I think the best
practice is to dcheck early, during the commission of the crime.

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:85: VirtualProtect(address_, bytes_,
old_protect_, &old_protect_);
On 2014/09/26 18:47:49, rvargas wrote:
> On 2014/09/26 01:10:01, cpu wrote:
> > sholdn't we dcheck that this VP fails?
> 
> Maybe, but what do we gain? I don't like to DCHECK on the OS doing anything
> because that's not under control of this code.

Acknowledged.

https://codereview.chromium.org/587923002/diff/1/chrome/app/close_handle_hook...
chrome/app/close_handle_hook_win.cc:98: if (!pe.VerifyMagic())
On 2014/09/26 18:47:49, rvargas wrote:
> On 2014/09/26 01:10:01, cpu wrote:
> > I guess I am confused why this is a silent return vs other cases where we
> > dcheck.
> 
> Because it's valid for the caller to pass an address that for some reason is
not
> a PE (EnumProcessModules returning a random mapped file?). We fail to patch it
> and tell that to the caller. However, it is expected that the caller should
> verify that the HMODULE it received is valid.
> 
> Having said that, the only caller is passing the result of
> GetModuleHandleA("kernel32.dll") without checking anything, so maybe we should
> remove the dcheck... This is basically the sandbox code slightly adapted so I
> didn't think too much about it.

I think either removing dcheck or adding a comment with a sort version of this
explanation works for me.

[cpu_(ooo_6.6-7.5), 2014-09-30 17:48:10]

lgtm

[rvargas (doing something else), 2014-09-30 18:26:24]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-09-30 18:27:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/587923002/20001

[commit-bot: I haz the power, 2014-09-30 18:42:34]

Committed patchset #2 (id:20001) as 0653491f0883a9ec3b76c381bf3473ef21acc10a

[commit-bot: I haz the power, 2014-09-30 18:43:14]

Patchset 2 (id:??) landed as
https://crrev.com/ae02eef3fba47e4955be8b5c6e6e47c86b43f32d
Cr-Commit-Position: refs/heads/master@{#297464}