Add a sandbox API for broker handle duplication

Add a sandbox API for broker handle duplication 

BUG=119250
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129627
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=130029

[jschuh, 2012-03-26 19:22:02]

I know stayle tweaks left in the sndbox code, but I'm having an issue with my
checkout and wanted to give you guys sufficient heads-up for a large patch.

@jam - This is just for the stuff in /content. I've switched the plugin code to
the sandbox DuplicateHandle API I added, and removed some code that's now
unnecessary.

@cpu, @rvargas - I added policy for handle duplication and exposed it through an
explicit API rather than an intercept. I'm not using an intercept because I want
to actively discourage copying process handles into sandboxed processes. After
this patch I'll need to land another large patch to fix the DuplicateHandle
calls in the GPU (which is its own yak to shave thanks to the accelerated video
changes). Once I have all instances converted over, I plan to add an intercept
for DuplicateHandle that has CHECK if the source or destination are not the
current process.

[jschuh, 2012-03-26 22:40:50]

Okay, should be ready for review now.

[rvargas (doing something else), 2012-03-27 00:35:32]

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_dispatcher.cc:22: 
nit: remove line

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_dispatcher.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_dispatcher.h:14: // This class handles registry-related IPC
calls.
nit: update

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_interception.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_interception.cc:7: #include "base/win/scoped_handle.h"
remove

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_interception.cc:23: if (!ValidParameter(target_handle,
sizeof(HANDLE), WRITE))
I wouldn't worry about this... maybe just target_handle = NULL

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_interception.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_interception.h:9: #define SANDBOX_SRC_HANDLE_INTERCEPTION_H__
nit: Naming this _interception is misleading

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_policy.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.cc:22: TargetPolicy::Semantics semantics,
nit: indentation

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.cc:28: PolicyRule handle(ASK_BROKER);
nit: handle_rule? (or rule)

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.cc:52: if
(!SandboxFactory::GetBrokerServices()->IsActiveTarget(target_process_id)) {
Calling GetBrokerServices feels weird.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_policy.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.h:12: #include "sandbox/src/nt_internals.h"
remove

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.h:20: // This class centralizes most of the knowledge
related to handle policy
nit: Period at the end.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.h:24: // policy rule for handles, in particular open
or create actions.
open and create?

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.h:25: static bool GenerateRules(const wchar_t* name,
Somewhere there should be stated that this name is just the type, not the object
name.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_policy_test.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy_test.cc:32: base::win::ScopedHandle
test_event(::CreateEvent(
nit: do you mind moving CreateEvent to the next line/

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy_test.cc:47: // Tests that we duplicating an object
works only when the policy allows it.
nit: drop "we"

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/ipc_tags.h
File sandbox/src/ipc_tags.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/ipc_tag...
sandbox/src/ipc_tags.h:31: IPC_DUPLICATEHANDLEPROXY_TAG,
I'm assuming that there will be no interception... 'cause that would mean that
this name is not correct. (but there are comments implying that there will be
interceptions)

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/sandbox.h
File sandbox/src/sandbox.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/sandbox...
sandbox/src/sandbox.h:92: virtual bool IsActiveTarget(DWORD process_id) = 0;
In general, this interface is intended for the user, not for internal code.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
File sandbox/tests/common/controller.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.cc:101: target_process_.Close();
?

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.cc:123: if (broker_ &&
broker_->IsActiveTarget(target_process_id_))
why not always?

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.cc:190: if (broker_ &&
broker_->IsActiveTarget(target_process_id_))
We should be able to do this within this class (without asking the broker).

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
File sandbox/tests/common/controller.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.h:105: // Return the process handle for an
asynchromous test.
only for an async test?
nit: async typo

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.h:108: // Return the process ID for an
asynchromous test.
and here

https://chromiumcodereview.appspot.com/9838083/diff/22044/sandbox/src/broker_...
File sandbox/src/broker_services.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/22044/sandbox/src/broker_...
sandbox/src/broker_services.h:48: virtual bool IsActiveTarget(DWORD process_id);
I'm not sure we need to expose this just to support tests.

[jschuh, 2012-03-27 01:36:18]

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_dispatcher.cc:22: 
On 2012/03/27 00:35:33, rvargas wrote:
> nit: remove line

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_dispatcher.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_dispatcher.h:14: // This class handles registry-related IPC
calls.
On 2012/03/27 00:35:33, rvargas wrote:
> nit: update

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_interception.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_interception.cc:7: #include "base/win/scoped_handle.h"
On 2012/03/27 00:35:33, rvargas wrote:
> remove

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_interception.cc:23: if (!ValidParameter(target_handle,
sizeof(HANDLE), WRITE))
On 2012/03/27 00:35:33, rvargas wrote:
> I wouldn't worry about this... maybe just target_handle = NULL

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_interception.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_interception.h:9: #define SANDBOX_SRC_HANDLE_INTERCEPTION_H__
On 2012/03/27 00:35:33, rvargas wrote:
> nit: Naming this _interception is misleading

I have a DuplicateHandle interception that will go in here once I've killed off
all the bad DulicateHandle calls. So, any objections to leaving it this way with
a todo?

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_policy.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.cc:22: TargetPolicy::Semantics semantics,
On 2012/03/27 00:35:33, rvargas wrote:
> nit: indentation

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.cc:28: PolicyRule handle(ASK_BROKER);
On 2012/03/27 00:35:33, rvargas wrote:
> nit: handle_rule? (or rule)

How about duplicate_rule?

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.cc:52: if
(!SandboxFactory::GetBrokerServices()->IsActiveTarget(target_process_id)) {
On 2012/03/27 00:35:33, rvargas wrote:
> Calling GetBrokerServices feels weird.

Can't think of a better way. It's the only thing that should know all the valid
children.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_policy.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.h:12: #include "sandbox/src/nt_internals.h"
On 2012/03/27 00:35:33, rvargas wrote:
> remove

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.h:20: // This class centralizes most of the knowledge
related to handle policy
On 2012/03/27 00:35:33, rvargas wrote:
> nit: Period at the end. 

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.h:24: // policy rule for handles, in particular open
or create actions.
On 2012/03/27 00:35:33, rvargas wrote:
> open and create?

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.h:25: static bool GenerateRules(const wchar_t* name,
On 2012/03/27 00:35:33, rvargas wrote:
> Somewhere there should be stated that this name is just the type, not the
object
> name.

Changed the variable to type_name.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_policy_test.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy_test.cc:32: base::win::ScopedHandle
test_event(::CreateEvent(
On 2012/03/27 00:35:33, rvargas wrote:
> nit: do you mind moving CreateEvent to the next line/

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy_test.cc:47: // Tests that we duplicating an object
works only when the policy allows it.
On 2012/03/27 00:35:33, rvargas wrote:
> nit: drop "we"

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/ipc_tags.h
File sandbox/src/ipc_tags.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/ipc_tag...
sandbox/src/ipc_tags.h:31: IPC_DUPLICATEHANDLEPROXY_TAG,
On 2012/03/27 00:35:33, rvargas wrote:
> I'm assuming that there will be no interception... 'cause that would mean that
> this name is not correct. (but there are comments implying that there will be
> interceptions)

There will be a DuplicateHandle interception after I remove all the dangerous
uses.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/sandbox.h
File sandbox/src/sandbox.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/sandbox...
sandbox/src/sandbox.h:92: virtual bool IsActiveTarget(DWORD process_id) = 0;
On 2012/03/27 00:35:33, rvargas wrote:
> In general, this interface is intended for the user, not for internal code.

I exposed it because it seemed generally useful. For example, I used it in the
test code.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
File sandbox/tests/common/controller.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.cc:101: target_process_.Close();
On 2012/03/27 00:35:33, rvargas wrote:
> ?

Done.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.cc:123: if (broker_ &&
broker_->IsActiveTarget(target_process_id_))
On 2012/03/27 00:35:33, rvargas wrote:
> why not always?

The process ID could get reused. Probably just paranoia, but doesn't seem to
hurt anything.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.cc:190: if (broker_ &&
broker_->IsActiveTarget(target_process_id_))
On 2012/03/27 00:35:33, rvargas wrote:
> We should be able to do this within this class (without asking the broker).

I originally had it with GetExitCodeProcess, but this seemed cleaner to me.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
File sandbox/tests/common/controller.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.h:105: // Return the process handle for an
asynchromous test.
On 2012/03/27 00:35:33, rvargas wrote:
> only for an async test?
> nit: async typo

Yep. It doesn't get set for synchronous tests.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.h:108: // Return the process ID for an
asynchromous test.
On 2012/03/27 00:35:33, rvargas wrote:
> and here

Done.

https://chromiumcodereview.appspot.com/9838083/diff/22044/sandbox/src/broker_...
File sandbox/src/broker_services.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/22044/sandbox/src/broker_...
sandbox/src/broker_services.h:48: virtual bool IsActiveTarget(DWORD process_id);
On 2012/03/27 00:35:33, rvargas wrote:
> I'm not sure we need to expose this just to support tests.

Seemed like a good utility method to have. Otherwise there's no good interface
to determine if a process is a child of a given broker.

[rvargas (doing something else), 2012-03-27 02:30:59]

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_interception.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_interception.h:9: #define SANDBOX_SRC_HANDLE_INTERCEPTION_H__
> I have a DuplicateHandle interception that will go in here once I've killed
off
> all the bad DulicateHandle calls. So, any objections to leaving it this way
with
> a todo?

And that interception will just DCHECK, right? No need for a TODO

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
File sandbox/src/handle_policy.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.cc:28: PolicyRule handle(ASK_BROKER);
On 2012/03/27 01:36:19, Justin Schuh wrote:
> On 2012/03/27 00:35:33, rvargas wrote:
> > nit: handle_rule? (or rule)
> 
> How about duplicate_rule?

That works

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
sandbox/src/handle_policy.cc:52: if
(!SandboxFactory::GetBrokerServices()->IsActiveTarget(target_process_id)) {
On 2012/03/27 01:36:19, Justin Schuh wrote:
> On 2012/03/27 00:35:33, rvargas wrote:
> > Calling GetBrokerServices feels weird.
> 
> Can't think of a better way. It's the only thing that should know all the
valid
> children.

What I don't like is going through the external interface, and exposing a new
method without an actual need for it.

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
File sandbox/tests/common/controller.cc (right):

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.cc:123: if (broker_ &&
broker_->IsActiveTarget(target_process_id_))
On 2012/03/27 01:36:19, Justin Schuh wrote:
> On 2012/03/27 00:35:33, rvargas wrote:
> > why not always?
> 
> The process ID could get reused. Probably just paranoia, but doesn't seem to
> hurt anything.

But we have a handle, there's no need to worry about the PID

https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
sandbox/tests/common/controller.cc:190: if (broker_ &&
broker_->IsActiveTarget(target_process_id_))
On 2012/03/27 01:36:19, Justin Schuh wrote:
> On 2012/03/27 00:35:33, rvargas wrote:
> > We should be able to do this within this class (without asking the broker).
> 
> I originally had it with GetExitCodeProcess, but this seemed cleaner to me.

But this only cares about creating two targets, that can be controlled directly
by the test (if needed, note that we don't).

[jschuh, 2012-03-27 05:29:09]

On 2012/03/27 02:30:59, rvargas wrote:
>
https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
> File sandbox/src/handle_interception.h (right):
> 
>
https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
> sandbox/src/handle_interception.h:9: #define
SANDBOX_SRC_HANDLE_INTERCEPTION_H__
> > I have a DuplicateHandle interception that will go in here once I've killed
> off
> > all the bad DulicateHandle calls. So, any objections to leaving it this way
> with
> > a todo?
> 
> And that interception will just DCHECK, right? No need for a TODO

Meh, added the todo anyway, as a bookmark for me.

>
https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
> File sandbox/src/handle_policy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
> sandbox/src/handle_policy.cc:28: PolicyRule handle(ASK_BROKER);
> On 2012/03/27 01:36:19, Justin Schuh wrote:
> > On 2012/03/27 00:35:33, rvargas wrote:
> > > nit: handle_rule? (or rule)
> > 
> > How about duplicate_rule?
> 
> That works
> 
>
https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/src/handle_...
> sandbox/src/handle_policy.cc:52: if
> (!SandboxFactory::GetBrokerServices()->IsActiveTarget(target_process_id)) {
> On 2012/03/27 01:36:19, Justin Schuh wrote:
> > On 2012/03/27 00:35:33, rvargas wrote:
> > > Calling GetBrokerServices feels weird.
> > 
> > Can't think of a better way. It's the only thing that should know all the
> valid
> > children.
> 
> What I don't like is going through the external interface, and exposing a new
> method without an actual need for it.

I can hang it off of BrokerServicesBase and cast it. I just found it convenient
enough to push out to the external interface, but if you feel strongly about it
I can make it private.


>
https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
> File sandbox/tests/common/controller.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
> sandbox/tests/common/controller.cc:123: if (broker_ &&
> broker_->IsActiveTarget(target_process_id_))
> On 2012/03/27 01:36:19, Justin Schuh wrote:
> > On 2012/03/27 00:35:33, rvargas wrote:
> > > why not always?
> > 
> > The process ID could get reused. Probably just paranoia, but doesn't seem to
> > hurt anything.
> 
> But we have a handle, there's no need to worry about the PID

Either way we need to determine if the process is still active, because it's
messy to track multiple processes with a single TestRunner.


>
https://chromiumcodereview.appspot.com/9838083/diff/19011/sandbox/tests/commo...
> sandbox/tests/common/controller.cc:190: if (broker_ &&
> broker_->IsActiveTarget(target_process_id_))
> On 2012/03/27 01:36:19, Justin Schuh wrote:
> > On 2012/03/27 00:35:33, rvargas wrote:
> > > We should be able to do this within this class (without asking the
broker).
> > 
> > I originally had it with GetExitCodeProcess, but this seemed cleaner to me.
> 
> But this only cares about creating two targets, that can be controlled
directly
> by the test (if needed, note that we don't).

Seemed better to encapsulate it in the TestRunner, and I'd still rather keep it
there. However, I can move IsActiveTarget out of the external interface and
switch back to GetExitCodeProcess.

[jschuh, 2012-03-27 17:20:32]

Okay, I moved IsActiveTarget into the BrokerServicesBase class and am grabbing
the instance now. So, it's no longer exposed in the exported interface. I also
switched the test code to use ::GetExitCodeProcess.

[rvargas (doing something else), 2012-03-27 17:42:23]

LGTM

https://chromiumcodereview.appspot.com/9838083/diff/18076/sandbox/src/handle_...
File sandbox/src/handle_interception.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/18076/sandbox/src/handle_...
sandbox/src/handle_interception.h:8: #ifndef SANDBOX_SRC_HANDLE_INTERCEPTION_H__
nit: same here.

https://chromiumcodereview.appspot.com/9838083/diff/18076/sandbox/src/handle_...
File sandbox/src/handle_policy.h (right):

https://chromiumcodereview.appspot.com/9838083/diff/18076/sandbox/src/handle_...
sandbox/src/handle_policy.h:5: #ifndef SANDBOX_SRC_HANDLE_POLICY_H__
nit: the current style is a single underscore at the end.

[jam, 2012-03-27 19:21:54]

lgtm

[rvargas (doing something else), 2012-03-28 00:45:17]

I forgot to mention: please make sure you build and run the 64-bit version the 3
test suites.

[cpu_(ooo_6.6-7.5), 2012-03-28 02:01:29]

this is easy one Ricardo is done :)

lgtm

[nsylvain, 2012-03-28 13:11:45]

A few tiny nits I had a few patchsets ago and lost internet access so i could
not upload them.  (not commit blocker)

http://codereview.chromium.org/9838083/diff/17022/content/common/sandbox_poli...
File content/common/sandbox_policy.cc (right):

http://codereview.chromium.org/9838083/diff/17022/content/common/sandbox_poli...
content/common/sandbox_policy.cc:445: BOOL inherit_handle,
i dont think you should be supporting inherit_handle.   inherit_handle should
probably be always false.   Although does not matter much if you want your call
to look as much like DuplicateHandle as possible.

http://codereview.chromium.org/9838083/diff/17022/sandbox/src/broker_services.h
File sandbox/src/broker_services.h (right):

http://codereview.chromium.org/9838083/diff/17022/sandbox/src/broker_services...
sandbox/src/broker_services.h:36: // The next five methods are the
BrokerServices interface
you can just remove "five"

http://codereview.chromium.org/9838083/diff/17022/sandbox/src/handle_dispatch...
File sandbox/src/handle_dispatcher.cc (right):

http://codereview.chromium.org/9838083/diff/17022/sandbox/src/handle_dispatch...
sandbox/src/handle_dispatcher.cc:57: 0, FALSE, DUPLICATE_SAME_ACCESS)) {
There is probably a better right that allows you to only query the object name.

[cpu_(ooo_6.6-7.5), 2012-03-28 22:05:23]

+1 on the inheritance comment by nicolas.

On 2012/03/28 13:11:45, nsylvain wrote:
> A few tiny nits I had a few patchsets ago and lost internet access so i could
> not upload them.  (not commit blocker)
> 
>
http://codereview.chromium.org/9838083/diff/17022/content/common/sandbox_poli...
> File content/common/sandbox_policy.cc (right):
> 
>
http://codereview.chromium.org/9838083/diff/17022/content/common/sandbox_poli...
> content/common/sandbox_policy.cc:445: BOOL inherit_handle,
> i dont think you should be supporting inherit_handle.   inherit_handle should
> probably be always false.   Although does not matter much if you want your
call
> to look as much like DuplicateHandle as possible.
> 
>
http://codereview.chromium.org/9838083/diff/17022/sandbox/src/broker_services.h
> File sandbox/src/broker_services.h (right):
> 
>
http://codereview.chromium.org/9838083/diff/17022/sandbox/src/broker_services...
> sandbox/src/broker_services.h:36: // The next five methods are the
> BrokerServices interface
> you can just remove "five"
> 
>
http://codereview.chromium.org/9838083/diff/17022/sandbox/src/handle_dispatch...
> File sandbox/src/handle_dispatcher.cc (right):
> 
>
http://codereview.chromium.org/9838083/diff/17022/sandbox/src/handle_dispatch...
> sandbox/src/handle_dispatcher.cc:57: 0, FALSE, DUPLICATE_SAME_ACCESS)) {
> There is probably a better right that allows you to only query the object
name.

[jschuh, 2012-03-28 22:09:29]

http://codereview.chromium.org/9838083/diff/17022/content/common/sandbox_poli...
File content/common/sandbox_policy.cc (right):

http://codereview.chromium.org/9838083/diff/17022/content/common/sandbox_poli...
content/common/sandbox_policy.cc:445: BOOL inherit_handle,
On 2012/03/28 13:11:46, nsylvain wrote:
> i dont think you should be supporting inherit_handle.   inherit_handle should
> probably be always false.   Although does not matter much if you want your
call
> to look as much like DuplicateHandle as possible.

Fair point. I'll remove it entirely.

http://codereview.chromium.org/9838083/diff/17022/sandbox/src/broker_services.h
File sandbox/src/broker_services.h (right):

http://codereview.chromium.org/9838083/diff/17022/sandbox/src/broker_services...
sandbox/src/broker_services.h:36: // The next five methods are the
BrokerServices interface
On 2012/03/28 13:11:46, nsylvain wrote:
> you can just remove "five"

Done.

http://codereview.chromium.org/9838083/diff/17022/sandbox/src/handle_dispatch...
File sandbox/src/handle_dispatcher.cc (right):

http://codereview.chromium.org/9838083/diff/17022/sandbox/src/handle_dispatch...
sandbox/src/handle_dispatcher.cc:57: 0, FALSE, DUPLICATE_SAME_ACCESS)) {
On 2012/03/28 13:11:46, nsylvain wrote:
> There is probably a better right that allows you to only query the object
name.

Actually, I can use a handle with no permissions.

http://codereview.chromium.org/9838083/diff/18076/sandbox/src/handle_intercep...
File sandbox/src/handle_interception.h (right):

http://codereview.chromium.org/9838083/diff/18076/sandbox/src/handle_intercep...
sandbox/src/handle_interception.h:8: #ifndef SANDBOX_SRC_HANDLE_INTERCEPTION_H__
On 2012/03/27 17:42:23, rvargas wrote:
> nit: same here.

Done.

http://codereview.chromium.org/9838083/diff/18076/sandbox/src/handle_policy.h
File sandbox/src/handle_policy.h (right):

http://codereview.chromium.org/9838083/diff/18076/sandbox/src/handle_policy.h...
sandbox/src/handle_policy.h:5: #ifndef SANDBOX_SRC_HANDLE_POLICY_H__
On 2012/03/27 17:42:23, rvargas wrote:
> nit: the current style is a single underscore at the end.

Done.

[cpu_(ooo_6.6-7.5), 2012-03-29 02:17:04]

lgtm

[jschuh, 2012-03-30 22:29:42]

The spaghetti handle mess got a little more complicated after I started this
patch. So, I'm going to land the sandbox API change independently, and untangle
the plugin/gpu mess in a separate patch.

[commit-bot: I haz the power, 2012-03-30 22:38:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jschuh@chromium.org/9838083/41001

[commit-bot: I haz the power, 2012-03-31 01:46:29]

Try job failure for 9838083-41001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Dan Beam, 2012-03-31 03:08:09]

ping, this seems to have broken sbox_integration_tests on Win7 Tests (dbg)(3) -
http://build.chromium.org/p/chromium/builders/Win7%20Tests%20%28dbg%29%283%29...