Integrate the handle verifier from multiple modules (DLLs)

Integrate the handle verifier from multiple modules (DLLs)

This allows handles created on the exe (or any other module) to be properly tracked.

BUG=472362
Committed: https://crrev.com/8545e0e789c271bbc75d12a3fd92dcfbf800e54f
Cr-Commit-Position: refs/heads/master@{#324335}

[rvargas (doing something else), 2015-03-31 02:24:50]

Patchset #1 (id:1) has been deleted

[rvargas (doing something else), 2015-04-01 03:06:18]

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

[grt (UTC plus 2), 2015-04-01 15:33:04]

drive-by

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

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:7: #include <unordered_map>
would you mind switching from unordered_map to "base/containers/hash_tables.h"
and base::hash_map? They amount to the same thing, but i think it's to our
advantage to not have fragmentation in the codebase.

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:71: ActiveVerifier(bool enabled)
explicit

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:78: virtual bool CloseHandle(HANDLE handle);
why virtual? if it's for a subtle "because we need a vtable" reason, please
document it.

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:87: ~ActiveVerifier();
if the methods above truly need to be virtual, make this virtual as well.

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:109: g_active_verifier = new ActiveVerifier(false);
indentation

[rvargas (doing something else), 2015-04-01 16:51:03]

thanks

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

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:7: #include <unordered_map>
On 2015/04/01 15:33:04, grt wrote:
> would you mind switching from unordered_map to "base/containers/hash_tables.h"
> and base::hash_map? They amount to the same thing, but i think it's to our
> advantage to not have fragmentation in the codebase.

This was used here because of inconsistencies in our pre-standard implementation
that basically broke for int64 values. Maybe that is fixed now, but updating it
is independent of the issue being fixed here.

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:71: ActiveVerifier(bool enabled)
On 2015/04/01 15:33:04, grt wrote:
> explicit

Acknowledged.

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:78: virtual bool CloseHandle(HANDLE handle);
On 2015/04/01 15:33:04, grt wrote:
> why virtual? if it's for a subtle "because we need a vtable" reason, please
> document it.

Yes the whole point of the CL is that we need to forward the call to another
module and if we don't have a vtable then we end up executing the function that
is linked directly into the current module (bad thing). Will add comment.

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:87: ~ActiveVerifier();
On 2015/04/01 15:33:04, grt wrote:
> if the methods above truly need to be virtual, make this virtual as well.

There's no destructor implementation.

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:109: g_active_verifier = new ActiveVerifier(false);
On 2015/04/01 15:33:04, grt wrote:
> indentation

... I saw that while testing and I forgot to fix it. Oh well.

[Will Harris, 2015-04-01 17:44:24]

another drive-by, but I might have the wrong end of the stick.

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

https://codereview.chromium.org/1045513004/diff/60001/base/win/scoped_handle....
base/win/scoped_handle.cc:17: __declspec(dllexport) void* GetHandleVerifier();
won't this function end up being exported from all chrome modules linked with
base i.e. chrome.dll chrome_child.dll and chrome.exe - don't you just want it
exported from chrome.exe?

https://codereview.chromium.org/1045513004/diff/60001/base/win/scoped_handle....
base/win/scoped_handle.cc:105: HMODULE main_module = ::GetModuleHandle(NULL);
to confirm you want to call the exe's version of GetHandleVerifier if you're
inside a DLL?

if so, I think this call to GetModuleHandle will never fail, so the call on line
111 will never happen?  Unless I'm missing something here.

[rvargas (doing something else), 2015-04-01 18:23:54]

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

https://codereview.chromium.org/1045513004/diff/60001/base/win/scoped_handle....
base/win/scoped_handle.cc:17: __declspec(dllexport) void* GetHandleVerifier();
On 2015/04/01 17:44:24, Will Harris wrote:
> won't this function end up being exported from all chrome modules linked with
> base i.e. chrome.dll chrome_child.dll and chrome.exe - don't you just want it
> exported from chrome.exe?

sorta, kind of yes... I mean, I'm fine with every module exporting the function
because... what I was really looking for is a general way to make sure that all
modules (not just the main dll and the exe) are connected, without having to add
logic at the embedder layer (aka, unknown_foo.dll!main?) to locate one specific
module.

That's why PS1 (intentionally here) shows an sketch of not having an export and
relying in something dirtier: something on the environment. Which means the
"master" is whoever calls this code first!.

Anyways, in practice though, for the foreseeable future we'll have uses of
ScopedHanlde in the exe (the sandbox code), and that means that the first module
to look for this thing is the exe, which means that other modules can assume
that the exe is the master (although to be fair, I'd prefer the master to be
chrome.dll), and so they can just get the hmodule of the exe.

So yes, we could add logic to prevent other modules from exporting this, but I'm
not sure it's worth it (especially because base should not really be aware of
how many modules the app have, and adding callbacks to configure this from the
embedder is not that nice). But yes, this also has the implied assumption that
the exe is the first to come up :(

https://codereview.chromium.org/1045513004/diff/60001/base/win/scoped_handle....
base/win/scoped_handle.cc:105: HMODULE main_module = ::GetModuleHandle(NULL);
On 2015/04/01 17:44:24, Will Harris wrote:
> to confirm you want to call the exe's version of GetHandleVerifier if you're
> inside a DLL?
> 
> if so, I think this call to GetModuleHandle will never fail, so the call on
line
> 111 will never happen?  Unless I'm missing something here.

Correct. I'm just being extremely cautious about error handling. I could DCHECK
(or CHECK?) if you want me to. (that _should_ be more of a build problem... for
instance moving the fn definition to a namespace)

[grt (UTC plus 2), 2015-04-01 20:33:02]

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

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:7: #include <unordered_map>
On 2015/04/01 16:51:03, rvargas (soon out of office) wrote:
> On 2015/04/01 15:33:04, grt wrote:
> > would you mind switching from unordered_map to
"base/containers/hash_tables.h"
> > and base::hash_map? They amount to the same thing, but i think it's to our
> > advantage to not have fragmentation in the codebase.
> 
> This was used here because of inconsistencies in our pre-standard
implementation
> that basically broke for int64 values.

That ain't good.

> Maybe that is fixed now, but updating it
> is independent of the issue being fixed here.

Fair enough.

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:78: virtual bool CloseHandle(HANDLE handle);
On 2015/04/01 16:51:03, rvargas (soon out of office) wrote:
> On 2015/04/01 15:33:04, grt wrote:
> > why virtual? if it's for a subtle "because we need a vtable" reason, please
> > document it.
> 
> Yes the whole point of the CL is that we need to forward the call to another
> module and if we don't have a vtable then we end up executing the function
that
> is linked directly into the current module (bad thing).

Yup, that's what I thought.

> Will add comment.

Thanks.

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:87: ~ActiveVerifier();
On 2015/04/01 16:51:03, rvargas (soon out of office) wrote:
> On 2015/04/01 15:33:04, grt wrote:
> > if the methods above truly need to be virtual, make this virtual as well.
> 
> There's no destructor implementation.

Ah, so there isn't. Ack.

[grt (UTC plus 2), 2015-04-02 02:19:03]

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

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:78: virtual bool CloseHandle(HANDLE handle);
On 2015/04/01 16:51:03, rvargas (soon out of office) wrote:
> On 2015/04/01 15:33:04, grt wrote:
> > why virtual? if it's for a subtle "because we need a vtable" reason, please
> > document it.
> 
> Yes the whole point of the CL is that we need to forward the call to another
> module and if we don't have a vtable then we end up executing the function
that
> is linked directly into the current module (bad thing). Will add comment.

Wait, why is that necessary? Are not all modules that link this in (chrome.exe,
chrome.dll) built at the same time, such that they have identical
implementations? If all state is held within the instance (as opposed to being
in module-specific global vars), why does it matter which copy of the assembly
instructions is used? From what I see, all state is accessed through the |this|
pointer, so none of these need to be virtual.

I have a vague sense of "I must be missing something", but I can't think of what
it is.

Why not re-frame the point of the CL so that the state established by the first
module is used throughout the process (rather than forwarding the calls around)?

[rvargas (doing something else), 2015-04-02 17:53:58]

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

https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
base/win/scoped_handle.cc:78: virtual bool CloseHandle(HANDLE handle);
On 2015/04/02 02:19:03, grt wrote:
> On 2015/04/01 16:51:03, rvargas (soon out of office) wrote:
> > On 2015/04/01 15:33:04, grt wrote:
> > > why virtual? if it's for a subtle "because we need a vtable" reason,
please
> > > document it.
> > 
> > Yes the whole point of the CL is that we need to forward the call to another
> > module and if we don't have a vtable then we end up executing the function
> that
> > is linked directly into the current module (bad thing). Will add comment.
> 
> Wait, why is that necessary? Are not all modules that link this in
(chrome.exe,
> chrome.dll) built at the same time, such that they have identical
> implementations? If all state is held within the instance (as opposed to being
> in module-specific global vars), why does it matter which copy of the assembly
> instructions is used? From what I see, all state is accessed through the
|this|
> pointer, so none of these need to be virtual.
> 
> I have a vague sense of "I must be missing something", but I can't think of
what
> it is.
> 
> Why not re-frame the point of the CL so that the state established by the
first
> module is used throughout the process (rather than forwarding the calls
around)?

Sortof. All the state is inside the object, but there is a map that does memory
allocations, and we cannot allow the memory pools to be mixed (allocated from
the exe, then reallocated from the dll, for example), because that will corrupt
everything. (Which is also the reason there's no way to delete the object).

I could go to export pure c code instead, but I figured it is better to have a
single export and handle the other required "exports" with the vtable.

[grt (UTC plus 2), 2015-04-02 17:55:45]

On 2015/04/02 17:53:58, rvargas (soon out of office) wrote:
>
https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle.cc
> File base/win/scoped_handle.cc (right):
> 
>
https://codereview.chromium.org/1045513004/diff/40001/base/win/scoped_handle....
> base/win/scoped_handle.cc:78: virtual bool CloseHandle(HANDLE handle);
> On 2015/04/02 02:19:03, grt wrote:
> > On 2015/04/01 16:51:03, rvargas (soon out of office) wrote:
> > > On 2015/04/01 15:33:04, grt wrote:
> > > > why virtual? if it's for a subtle "because we need a vtable" reason,
> please
> > > > document it.
> > > 
> > > Yes the whole point of the CL is that we need to forward the call to
another
> > > module and if we don't have a vtable then we end up executing the function
> > that
> > > is linked directly into the current module (bad thing). Will add comment.
> > 
> > Wait, why is that necessary? Are not all modules that link this in
> (chrome.exe,
> > chrome.dll) built at the same time, such that they have identical
> > implementations? If all state is held within the instance (as opposed to
being
> > in module-specific global vars), why does it matter which copy of the
assembly
> > instructions is used? From what I see, all state is accessed through the
> |this|
> > pointer, so none of these need to be virtual.
> > 
> > I have a vague sense of "I must be missing something", but I can't think of
> what
> > it is.
> > 
> > Why not re-frame the point of the CL so that the state established by the
> first
> > module is used throughout the process (rather than forwarding the calls
> around)?
> 
> Sortof. All the state is inside the object, but there is a map that does
memory
> allocations, and we cannot allow the memory pools to be mixed (allocated from
> the exe, then reallocated from the dll, for example), because that will
corrupt
> everything. (Which is also the reason there's no way to delete the object).
> 
> I could go to export pure c code instead, but I figured it is better to have a
> single export and handle the other required "exports" with the vtable.

Gotcha. Thanks for clarifying.

[Will Harris, 2015-04-02 18:11:15]

wfh@chromium.org changed reviewers:
+ wfh@chromium.org

[Will Harris, 2015-04-02 18:11:15]

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

https://codereview.chromium.org/1045513004/diff/60001/base/win/scoped_handle....
base/win/scoped_handle.cc:105: HMODULE main_module = ::GetModuleHandle(NULL);
is it not possible for two modules to both create their own handle verifiers

e.g. module A starts and calls ActiveVerifier::InstallVerifier at the same time
module B starts and calls ActiveVerifier::InstallVerifier.

They each have their own private instance of g_lock so they both enter the code
and then query the main module, both get the answer NULL back, and both create
their own ActiveVerifier?

[rvargas (doing something else), 2015-04-02 18:25:24]

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

https://codereview.chromium.org/1045513004/diff/60001/base/win/scoped_handle....
base/win/scoped_handle.cc:105: HMODULE main_module = ::GetModuleHandle(NULL);
On 2015/04/02 18:11:15, Will Harris wrote:
> is it not possible for two modules to both create their own handle verifiers
> 
> e.g. module A starts and calls ActiveVerifier::InstallVerifier at the same
time
> module B starts and calls ActiveVerifier::InstallVerifier.
> 
> They each have their own private instance of g_lock so they both enter the
code
> and then query the main module, both get the answer NULL back, and both create
> their own ActiveVerifier?

Correct.

The lock here (118) is meant to protect threads running on the same module
(whatever that means), not another thread in another module. I'm fine with that
race (that particular client will not report everything it could).

Patch Set 1 has a global lock that makes sure all modules are completely in
sync, but I don't think it's worth doing that here. In practice, the exe creates
the verifier before launching the first dll (chrome).

[Will Harris, 2015-04-02 19:35:22]

On 2015/04/02 18:25:24, rvargas (soon out of office) wrote:
>
https://codereview.chromium.org/1045513004/diff/60001/base/win/scoped_handle.cc
> File base/win/scoped_handle.cc (right):
> 
>
https://codereview.chromium.org/1045513004/diff/60001/base/win/scoped_handle....
> base/win/scoped_handle.cc:105: HMODULE main_module = ::GetModuleHandle(NULL);
> On 2015/04/02 18:11:15, Will Harris wrote:
> > is it not possible for two modules to both create their own handle verifiers
> > 
> > e.g. module A starts and calls ActiveVerifier::InstallVerifier at the same
> time
> > module B starts and calls ActiveVerifier::InstallVerifier.
> > 
> > They each have their own private instance of g_lock so they both enter the
> code
> > and then query the main module, both get the answer NULL back, and both
create
> > their own ActiveVerifier?
> 
> Correct.
> 
> The lock here (118) is meant to protect threads running on the same module
> (whatever that means), not another thread in another module. I'm fine with
that
> race (that particular client will not report everything it could).
> 
> Patch Set 1 has a global lock that makes sure all modules are completely in
> sync, but I don't think it's worth doing that here. In practice, the exe
creates
> the verifier before launching the first dll (chrome).

Ack.

[cpu_(ooo_6.6-7.5), 2015-04-02 20:49:10]

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

https://codereview.chromium.org/1045513004/diff/60001/base/win/scoped_handle....
base/win/scoped_handle.cc:215: return g_active_verifier->StartTracking(handle,
owner, pc1, pc2);
so we always do the pattern of 212-123

How about we do

     ActiveVerifier::Get()->StartTracking(handle, owner, pc1, pc2)

and 
    ActiveVerifier::Get()->StopTracking(handle, owner, pc1, pc2)

and we move the if() logic inside the get?

[rvargas (doing something else), 2015-04-02 21:26:31]

thanks

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

https://codereview.chromium.org/1045513004/diff/60001/base/win/scoped_handle....
base/win/scoped_handle.cc:215: return g_active_verifier->StartTracking(handle,
owner, pc1, pc2);
On 2015/04/02 20:49:10, cpu wrote:
> so we always do the pattern of 212-123
> 
> How about we do
> 
>      ActiveVerifier::Get()->StartTracking(handle, owner, pc1, pc2)
> 
> and 
>     ActiveVerifier::Get()->StopTracking(handle, owner, pc1, pc2)
> 
> and we move the if() logic inside the get?

Done.

[cpu_(ooo_6.6-7.5), 2015-04-03 01:05:57]

lgtm

pls commit after the branch point.

[Will Harris, 2015-04-04 19:17:48]

lgtm % nit

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

https://codereview.chromium.org/1045513004/diff/100001/base/win/scoped_handle...
base/win/scoped_handle.cc:10: #include "base/environment.h"
nit: this include is no longer needed

[rvargas (doing something else), 2015-04-08 23:42:28]

The CQ bit was checked by rvargas@chromium.org

[rvargas (doing something else), 2015-04-08 23:42:28]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1045513004/#ps120001
(title: "Remove stray header")

[commit-bot: I haz the power, 2015-04-08 23:46:33]

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

[commit-bot: I haz the power, 2015-04-09 01:41:02]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-04-09 01:41:53]

Patchset 6 (id:??) landed as
https://crrev.com/8545e0e789c271bbc75d12a3fd92dcfbf800e54f
Cr-Commit-Position: refs/heads/master@{#324335}