Convert plugin and GPU process to brokered handle duplication.

Convert plugin and GPU process to brokered handle duplication.

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

[jschuh, 2012-04-08 16:39:52]

Most of this CL is plumbing that needs to be changed all at once. To summarize,
I removed all process handles with PROCESS_DUP_HANDLE permission from all
sandboxed processes. This mainly entailed switching the side that did the handle
duplication, and using the new BrokerDuplicateHandle function.

@apatrick - GPU. Please double-check that I'm not leaking handles.

@ananta - Plugins and accelerated video.

@cpu - Sandbox. To make the policy work I have to launch unsandboxed plugins via
the broker (with a wide-open policy). I chose that approach avoid arbitrary
outbound handle duplication, or adding more convoluted brokering.

@jam - Overarching owner and sanity check.


Once this change lands I'm going to add hooks (debug build only) that will catch
dangerous handle duplication patterns. My plan is to just terminate the process
with a useful log message. Hopefully that will prevent this from recurring.

[apatrick_chromium, 2012-04-09 18:40:58]

I've done a first pass and by inspection it looks good. I need to do some more
work to finish the review. See comments inline.

http://codereview.chromium.org/9958034/diff/16151/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (left):

http://codereview.chromium.org/9958034/diff/16151/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_channel_host_factory.cc:208: gpu_channel_ = new
GpuChannelHost(this, gpu_host_id_, gpu_client_id_);
gpu_host_id_ is not a PID. I think a mistake has been made in some code
reorganization in the past. I will have to debug this to understand why
gpu_host_id_ is being interpreted as a PID in GpuChannelHost. gpu_host_id_ is
just an int that uniquely identifies a given GpuProcessHost instance.

http://codereview.chromium.org/9958034/diff/16151/content/common/gpu/gpu_chan...
File content/common/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/9958034/diff/16151/content/common/gpu/gpu_chan...
content/common/gpu/gpu_channel.cc:62: GpuChannel::~GpuChannel() { }
nit: drop } to next line so I can set breakpoints here.

http://codereview.chromium.org/9958034/diff/16151/content/common/gpu/gpu_comm...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/9958034/diff/16151/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.cc:514: DCHECK(transfer_buffer !=
NULL);
It might be better to crash if you think this could fail. Otherwise the GPU
process has a handy null pointer to work with.

[jschuh, 2012-04-10 19:02:12]

After discussing with Carlos I made some additional sandbox changes. So, I have
to land this CL first: http://codereview.chromium.org/9960045/

I also removed some additional unnecessary code because we're no longer passing
the GPU process around.

http://codereview.chromium.org/9958034/diff/16151/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (left):

http://codereview.chromium.org/9958034/diff/16151/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_channel_host_factory.cc:208: gpu_channel_ = new
GpuChannelHost(this, gpu_host_id_, gpu_client_id_);
On 2012/04/09 18:40:58, apatrick_chromium wrote:
> gpu_host_id_ is not a PID. I think a mistake has been made in some code
> reorganization in the past. I will have to debug this to understand why
> gpu_host_id_ is being interpreted as a PID in GpuChannelHost. gpu_host_id_ is
> just an int that uniquely identifies a given GpuProcessHost instance.

Okay, I plumbed it back in. Oddly enough, it seems to have no impact on the
tests.

http://codereview.chromium.org/9958034/diff/16151/content/common/gpu/gpu_chan...
File content/common/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/9958034/diff/16151/content/common/gpu/gpu_chan...
content/common/gpu/gpu_channel.cc:62: GpuChannel::~GpuChannel() { }
On 2012/04/09 18:40:58, apatrick_chromium wrote:
> nit: drop } to next line so I can set breakpoints here.

Done.

http://codereview.chromium.org/9958034/diff/16151/content/common/gpu/gpu_comm...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/9958034/diff/16151/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.cc:514: DCHECK(transfer_buffer !=
NULL);
On 2012/04/09 18:40:58, apatrick_chromium wrote:
> It might be better to crash if you think this could fail. Otherwise the GPU
> process has a handy null pointer to work with.

It should fail only on a policy violation or a dead target. That said, it would
be bad to have the NULL hanging around, so I'll do a hard CHECK.

[ananta, 2012-04-10 19:36:17]

video acceleration and plugin changes LGTM

[apatrick_chromium, 2012-04-10 20:00:35]

http://codereview.chromium.org/9958034/diff/15140/content/common/gpu/client/g...
File content/common/gpu/client/gpu_channel_host.h (right):

http://codereview.chromium.org/9958034/diff/15140/content/common/gpu/client/g...
content/common/gpu/client/gpu_channel_host.h:155: base::ProcessId
gpu_process_id() const { return channel_->peer_pid(); }
I checked the one caller of this function and I think it actually wants the host
ID rather than the PID. So this should be:

int gpu_process_id() const { return gpu_host_id_; }

The name of the function should be gpu_host_id() or similar but I'll fix that
once you've landed. I'm a bit baffled that the tests worked. My best guess is
that both the host ID and PID have the property of uniquely identifying a
GpuProcessHost object, unless a PID is recycled.

[jschuh, 2012-04-10 23:30:59]

http://codereview.chromium.org/9958034/diff/15140/content/common/gpu/client/g...
File content/common/gpu/client/gpu_channel_host.h (right):

http://codereview.chromium.org/9958034/diff/15140/content/common/gpu/client/g...
content/common/gpu/client/gpu_channel_host.h:155: base::ProcessId
gpu_process_id() const { return channel_->peer_pid(); }
On 2012/04/10 20:00:35, apatrick_chromium wrote:
> I checked the one caller of this function and I think it actually wants the
host
> ID rather than the PID. So this should be:
> 
> int gpu_process_id() const { return gpu_host_id_; }
> 
> The name of the function should be gpu_host_id() or similar but I'll fix that
> once you've landed. I'm a bit baffled that the tests worked. My best guess is
> that both the host ID and PID have the property of uniquely identifying a
> GpuProcessHost object, unless a PID is recycled.

I just corrected it and added the gpu_host_id stuff in. The dependent sandbox
patch is landed so I'm running through the trybots now.

[apatrick_chromium, 2012-04-10 23:35:22]

lgtm

[jam, 2012-04-11 23:02:05]

lgtm

[cpu_(ooo_6.6-7.5), 2012-04-11 23:11:26]

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

http://codereview.chromium.org/9958034/diff/27025/content/common/sandbox_poli...
content/common/sandbox_policy.cc:465: // If our process is the target just
duplicate the handle.
I liked better when the cheap check
if (!g_target_services)
was in front.

http://codereview.chromium.org/9958034/diff/27025/content/common/sandbox_poli...
content/common/sandbox_policy.cc:483: if (!g_target_services || ::GetLastError()
!= ERROR_ACCESS_DENIED)
the code is returning earlier in both branches so even more reason for my
comment above.

[cpu_(ooo_6.6-7.5), 2012-04-11 23:12:49]

http://codereview.chromium.org/9958034/diff/27025/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/9958034/diff/27025/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:412: // Add policy for proxy window pump.
what is what are we allowing to dup here? "window pump" conjures the idea of
window handles but that cannot be the case..

[jschuh, 2012-04-11 23:52:32]

http://codereview.chromium.org/9958034/diff/27025/chrome/common/chrome_conten...
File chrome/common/chrome_content_client.cc (right):

http://codereview.chromium.org/9958034/diff/27025/chrome/common/chrome_conten...
chrome/common/chrome_content_client.cc:412: // Add policy for proxy window pump.
On 2012/04/11 23:12:49, cpu wrote:
> what is what are we allowing to dup here? "window pump" conjures the idea of
> window handles but that cannot be the case..

It's very confusing, but it's explained here in 
WebPluginDelegateProxy::HandleInputEvent():
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/webplugin...

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

http://codereview.chromium.org/9958034/diff/27025/content/common/sandbox_poli...
content/common/sandbox_policy.cc:465: // If our process is the target just
duplicate the handle.
On 2012/04/11 23:11:27, cpu wrote:
> I liked better when the cheap check
> if (!g_target_services)
> was in front.

What about the case where the sandboxed process has permission to duplicate the
handle, but the policy doesn't allow it? It seems worse to go to the broker and
then try duplicating directly. Although, maybe that's the rarer corner case.

[jschuh, 2012-04-12 03:37:32]

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

http://codereview.chromium.org/9958034/diff/27025/content/common/sandbox_poli...
content/common/sandbox_policy.cc:465: // If our process is the target just
duplicate the handle.
On 2012/04/11 23:52:32, Justin Schuh wrote:
> On 2012/04/11 23:11:27, cpu wrote:
> > I liked better when the cheap check
> > if (!g_target_services)
> > was in front.
> 
> What about the case where the sandboxed process has permission to duplicate
the
> handle, but the policy doesn't allow it? It seems worse to go to the broker
and
> then try duplicating directly. Although, maybe that's the rarer corner case.

Never mind. I just realized how dumb that is, given that it's almost never safe
for a sandboxed process to have permission to duplicate handles.

[jschuh, 2012-04-13 15:55:20]

All the dependencies I needed for this are in, so I'm planning to land it today.


@cpu - I think I covered all three comments from the last review, so if you
wouldn't mind a quick final pass.

[cpu_(ooo_6.6-7.5), 2012-04-13 17:39:11]

lgtm

[jschuh, 2012-04-13 22:09:36]

Apparently I didn't delete enough code, and broke Chrome OS.

@piman for one last review.

[piman, 2012-04-13 22:13:46]

http://codereview.chromium.org/9958034/diff/44001/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

http://codereview.chromium.org/9958034/diff/44001/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_channel_host_factory.cc:190: return NULL;
Hold on batman, we still need a failure path here:

if (request.channel_handle.name.empty())
     return NULL;


That should work, right?

[jschuh, 2012-04-13 22:26:20]

http://codereview.chromium.org/9958034/diff/44001/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

http://codereview.chromium.org/9958034/diff/44001/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_channel_host_factory.cc:190: return NULL;
On 2012/04/13 22:13:46, piman wrote:
> Hold on batman, we still need a failure path here:
> 
> if (request.channel_handle.name.empty())
>      return NULL;
> 
> 
> That should work, right?

Done.

[piman, 2012-04-13 22:31:51]

lgtm

[commit-bot: I haz the power, 2012-04-13 22:35:49]

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

[commit-bot: I haz the power, 2012-04-14 00:48:23]

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