Add check on invalid file descriptor at both broker and renderer sides.

Add check on invalid file descriptor at both broker and renderer sides.

The broker could send back an invalide channel handle if it fails to setup up renderer channel, e.g. when the broker fails to duplicate a file descriptor. Add a check in PpapiThread::SetupRendererChannel on this condition. Upon receiving an invalid channel handle from the broker, the BrokerDispatcherWrapper at the render side should check the channel handle before passing it down. Using an invalid channel handle to create a channel may cause LOG(FATAL) in IPC::Channel::ChannelImpl::CreatePipe().

Add a content unitest to check this.

BUG=103957
TEST=new unit test


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109747

[xhwang, 2011-11-03 22:07:40]

One thing not sure is to use -1 for invalid file descriptor.  But in
base/file_descriptor_posix, fd is int and initialized with -1 and I follow that
convention.

[Paweł Hajdan Jr., 2011-11-04 08:05:14]

Drive-by with minor test nits, mostly to keep it simpler.

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
File content/renderer/pepper_plugin_delegate_impl_unittest.cc (right):

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:1: // Copyright (c)
2009 The Chromium Authors. All rights reserved.
nit: 2011 not 2009.

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:11:
PepperPluginDelegateImplTest() {}
nit: Is the ctor needed here? Is the dtor needed?

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:25:
scoped_ptr<MockRenderProcess> mock_process_;
nit: I think you can just declare MockRenderProcess mock_process_, without
scoped_ptr and SetUp and TearDown. The entire test fixture is created fresh and
destroyed for each test, i.e. ctor, SetUp, test body, TearDown, dtor, ctor,
SetUp, test body, TearDown, dtor instead of ctor, SetUp, test body, TearDown,
SetUp, test body, TearDown, dtor

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:28:
DISALLOW_COPY_AND_ASSIGN(PepperPluginDelegateImplTest);
nit: No need for this in test fixtures.

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:36: IPC::ChannelHandle
channel_handle; // Invalid by default.
nit: Two spaces between code and comment please.

[xhwang, 2011-11-04 15:44:48]

Thanks for the great review.  Now I make the test much simpler and will update
the new patch shortly.

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
File content/renderer/pepper_plugin_delegate_impl_unittest.cc (right):

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:1: // Copyright (c)
2009 The Chromium Authors. All rights reserved.
On 2011/11/04 08:05:15, Paweł Hajdan Jr. wrote:
> nit: 2011 not 2009.

Done.

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:11:
PepperPluginDelegateImplTest() {}
On 2011/11/04 08:05:15, Paweł Hajdan Jr. wrote:
> nit: Is the ctor needed here? Is the dtor needed?

Done.

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:25:
scoped_ptr<MockRenderProcess> mock_process_;
On 2011/11/04 08:05:15, Paweł Hajdan Jr. wrote:
> nit: I think you can just declare MockRenderProcess mock_process_, without
> scoped_ptr and SetUp and TearDown. The entire test fixture is created fresh
and
> destroyed for each test, i.e. ctor, SetUp, test body, TearDown, dtor, ctor,
> SetUp, test body, TearDown, dtor instead of ctor, SetUp, test body, TearDown,
> SetUp, test body, TearDown, dtor

Great to know that.  Thanks!

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:28:
DISALLOW_COPY_AND_ASSIGN(PepperPluginDelegateImplTest);
On 2011/11/04 08:05:15, Paweł Hajdan Jr. wrote:
> nit: No need for this in test fixtures.

Done.

http://codereview.chromium.org/8436008/diff/1/content/renderer/pepper_plugin_...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:36: IPC::ChannelHandle
channel_handle; // Invalid by default.
On 2011/11/04 08:05:15, Paweł Hajdan Jr. wrote:
> nit: Two spaces between code and comment please.

Done.

[Paweł Hajdan Jr., 2011-11-04 16:58:39]

Code I commented in the drive-by LGTM with a minor comment.

Note that you still need ddorwin's review, I only took a look at the test.

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl_unittest.cc (right):

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:12: //
ppapi::proxy::ProxyChannel needs to be initialized on the render process.
nit: The comment seems quite confusing now in this place.

Should it say something more like "We need a render process for
ppapi::proxy::ProxyChannel"?

[xhwang, 2011-11-04 22:34:04]

That makes sense.  Thanks!

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl_unittest.cc (right):

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:12: //
ppapi::proxy::ProxyChannel needs to be initialized on the render process.
On 2011/11/04 16:58:39, Paweł Hajdan Jr. wrote:
> nit: The comment seems quite confusing now in this place.
> 
> Should it say something more like "We need a render process for
> ppapi::proxy::ProxyChannel"?

Done.

[ddorwin, 2011-11-04 23:10:03]

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:483: if
(channel_handle.name.empty())
We also need to make these checks in all
PpapiPluginProcessHost::Client::OnChannelOpened() implementations if they are
not already there.

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl_unittest.cc (right):

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:16: // Try to
initialize BrokerDispatcherWrapper with invalid ChannelHandle.
Are there some positive tests we should add while we're at it? Don't spend too
much time on it.

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:23: // Try
BrokerDispatcherWrapper.Init() TWICE to trigger the LOG(FATAL)
This block could probably be simplified. See if something like this makes sense:
An invalid handle should result in a failure (false) and no CHECK, such as the
one in CreatePipe().
Call it twice because without the invalid handle check, the posix code would hit
a one-time path due to a static variable.

[ddorwin, 2011-11-07 20:02:28]

http://codereview.chromium.org/8436008/diff/6005/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8436008/diff/6005/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:620: // Process all pending
channel requests from the renderers.
Please change "renderers" to "plugins". It's my copy and paste mistake
(apparently from PpapiPluginProcessHost::OnChannelConnected()).

[xhwang, 2011-11-07 22:00:14]

Mostly modified as commented.  Not sure if we need to add another unit test for
the HostDispatcherWrapper.

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:483: if
(channel_handle.name.empty())
On 2011/11/04 23:10:03, ddorwin wrote:
> We also need to make these checks in all
> PpapiPluginProcessHost::Client::OnChannelOpened() implementations if they are
> not already there.

Done.

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl_unittest.cc (right):

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:16: // Try to
initialize BrokerDispatcherWrapper with invalid ChannelHandle.
On 2011/11/04 23:10:03, ddorwin wrote:
> Are there some positive tests we should add while we're at it? Don't spend too
> much time on it.

Done.

http://codereview.chromium.org/8436008/diff/5001/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:23: // Try
BrokerDispatcherWrapper.Init() TWICE to trigger the LOG(FATAL)
On 2011/11/04 23:10:03, ddorwin wrote:
> This block could probably be simplified. See if something like this makes
sense:
> An invalid handle should result in a failure (false) and no CHECK, such as the
> one in CreatePipe().
> Call it twice because without the invalid handle check, the posix code would
hit
> a one-time path due to a static variable.

Done.

http://codereview.chromium.org/8436008/diff/6005/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8436008/diff/6005/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:620: // Process all pending
channel requests from the renderers.
On 2011/11/07 20:02:28, ddorwin wrote:
> Please change "renderers" to "plugins". It's my copy and paste mistake
> (apparently from PpapiPluginProcessHost::OnChannelConnected()).

Done.

[ddorwin, 2011-11-07 23:54:07]

Yes, it would be good to have a negative test and a positive one (if it's not
too much work) for the plugin case too.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:380: true, // is client
Two spaces then "// Client."

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:506: true)) { // is_client
same

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl_unittest.cc (right):

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:23: // Try to
initialize BrokerDispatcherWrapper with invalid ChannelHandle.
This is a test-level comment and should be above the TEST_F.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:29: // An invalide
handle should result in a failure (false) without a CHECK, such
invalid
"CHECK" is misleading since it's a LOG(FATAL). Crash might be better but isn't
entirely accurate.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:32: // variable and
cause a LOG(FATAL).
... and not go through the LOG(FATAL) path.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:36: // On valid
ChannelHandle, initialization should succeed.
Success should be a different test - something like
BrokerDispatcherWrapperInitSuccess.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:37: const char
kChannelName[] = "/tmp/PepperPluginDelegateImplTestChannelName";
Is this a file or just a string? This is weird on Windows too.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:39: int fds[2] = {-1,
-1};
Is there a CreateSocketPair() or something like that we can use instead?

[xhwang, 2011-11-08 00:45:38]

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:380: true, // is client
On 2011/11/07 23:54:07, ddorwin wrote:
> Two spaces then "// Client."

Done.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl.cc:506: true)) { // is_client
On 2011/11/07 23:54:07, ddorwin wrote:
> same

Done.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
File content/renderer/pepper_plugin_delegate_impl_unittest.cc (right):

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:23: // Try to
initialize BrokerDispatcherWrapper with invalid ChannelHandle.
On 2011/11/07 23:54:07, ddorwin wrote:
> This is a test-level comment and should be above the TEST_F.

Done.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:29: // An invalide
handle should result in a failure (false) without a CHECK, such
On 2011/11/07 23:54:07, ddorwin wrote:
> invalid
> "CHECK" is misleading since it's a LOG(FATAL). Crash might be better but isn't
> entirely accurate.

Done.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:32: // variable and
cause a LOG(FATAL).
On 2011/11/07 23:54:07, ddorwin wrote:
> ... and not go through the LOG(FATAL) path.

Done.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:36: // On valid
ChannelHandle, initialization should succeed.
On 2011/11/07 23:54:07, ddorwin wrote:
> Success should be a different test - something like
> BrokerDispatcherWrapperInitSuccess.

Done.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:37: const char
kChannelName[] = "/tmp/PepperPluginDelegateImplTestChannelName";
On 2011/11/07 23:54:07, ddorwin wrote:
> Is this a file or just a string? This is weird on Windows too.

Done.

http://codereview.chromium.org/8436008/diff/12001/content/renderer/pepper_plu...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:39: int fds[2] = {-1,
-1};
On 2011/11/07 23:54:07, ddorwin wrote:
> Is there a CreateSocketPair() or something like that we can use instead?

bool SocketPair(int* fd1, int* fd2) is defined in ipc_channel_posix.cc but it's
mostly local and not declared in header files.

[ddorwin, 2011-11-08 01:03:11]

lgtm

Kicking off try bots.

[xhwang, 2011-11-11 22:26:16]

Hello piman and brettw,

Could you please do a OWNERS review on this change? Thanks!

-xhwang

[piman, 2011-11-11 22:42:10]

LGTM. Please fix nits before submitting.

http://codereview.chromium.org/8436008/diff/9006/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8436008/diff/9006/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:492: if
(channel_handle.name.empty())
nit:indent

http://codereview.chromium.org/8436008/diff/9006/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl_unittest.cc (right):

http://codereview.chromium.org/8436008/diff/9006/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:10: #endif  //
defined(OS_POSIX));
nit: remove trailing );

http://codereview.chromium.org/8436008/diff/9006/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:29: // An invalide
handle should result in a failure (false) without a LOG(FATAL),
nit:invalide->invalid

[xhwang, 2011-11-11 22:53:28]

Done as commented.  Thanks a lot for the quick and careful review!

http://codereview.chromium.org/8436008/diff/9006/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/8436008/diff/9006/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl.cc:492: if
(channel_handle.name.empty())
On 2011/11/11 22:42:10, piman wrote:
> nit:indent

Done.

http://codereview.chromium.org/8436008/diff/9006/content/renderer/pepper_plug...
File content/renderer/pepper_plugin_delegate_impl_unittest.cc (right):

http://codereview.chromium.org/8436008/diff/9006/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:10: #endif  //
defined(OS_POSIX));
On 2011/11/11 22:42:10, piman wrote:
> nit: remove trailing );

Done.

http://codereview.chromium.org/8436008/diff/9006/content/renderer/pepper_plug...
content/renderer/pepper_plugin_delegate_impl_unittest.cc:29: // An invalide
handle should result in a failure (false) without a LOG(FATAL),
On 2011/11/11 22:42:10, piman wrote:
> nit:invalide->invalid

Done.

[commit-bot: I haz the power, 2011-11-11 23:04:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/8436008/14002

[commit-bot: I haz the power, 2011-11-11 23:04:42]

Can't apply patch for file content/ppapi_plugin/ppapi_thread.cc.
While running patch -p1 --forward --force;
patching file content/ppapi_plugin/ppapi_thread.cc
Hunk #1 FAILED at 300.
1 out of 1 hunk FAILED -- saving rejects to file
content/ppapi_plugin/ppapi_thread.cc.rej

[commit-bot: I haz the power, 2011-11-11 23:29:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/8436008/15002

[commit-bot: I haz the power, 2011-11-12 00:47:28]

Change committed as 109747