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


Review URL: http://codereview.chromium.org/8436008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@109747 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 83 insertions, 5 deletions

diff --git a/content/content_tests.gypi b/content/content_tests.gypi
index 0e249c5..a7a2df1 100644
--- a/content/content_tests.gypi
+++ b/content/content_tests.gypi
@@ -229,6 +229,7 @@
         'renderer/media/video_capture_message_filter_unittest.cc',
         'renderer/media/webrtc_audio_device_unittest.cc',
         'renderer/paint_aggregator_unittest.cc',
+        'renderer/pepper_plugin_delegate_impl_unittest.cc',
         'renderer/v8_value_converter_impl_unittest.cc',
         'test/run_all_unittests.cc',
       ],
diff --git a/content/ppapi_plugin/ppapi_thread.cc b/content/ppapi_plugin/ppapi_thread.cc
index c35bbd7..c2dc8c2 100644
--- a/content/ppapi_plugin/ppapi_thread.cc
+++ b/content/ppapi_plugin/ppapi_thread.cc
@@ -301,6 +301,8 @@ bool PpapiThread::SetupRendererChannel(base::ProcessHandle host_process_handle,
   // This ensures this process will be notified when it is closed even if a
   // connection is not established.
   handle->socket = base::FileDescriptor(dispatcher->TakeRendererFD(), true);
+  if (handle->socket.fd == -1)
+    return false;
 #endif
 
   // From here, the dispatcher will manage its own lifetime according to the
diff --git a/content/renderer/pepper_plugin_delegate_impl.cc b/content/renderer/pepper_plugin_delegate_impl.cc
index d621a45..94bbaf0 100644
--- a/content/renderer/pepper_plugin_delegate_impl.cc
+++ b/content/renderer/pepper_plugin_delegate_impl.cc
@@ -363,13 +363,22 @@ class HostDispatcherWrapper
             PP_Module pp_module,
             ppapi::proxy::Dispatcher::GetInterfaceFunc local_get_interface,
             const ppapi::Preferences& preferences) {
+    if (channel_handle.name.empty())
+      return false;
+
+#if defined(OS_POSIX)
+    if (channel_handle.socket.fd == -1)
+      return false;
+#endif
+
     dispatcher_delegate_.reset(new DispatcherDelegate);
     dispatcher_.reset(new ppapi::proxy::HostDispatcher(
         plugin_process_handle, pp_module, local_get_interface));
 
-    if (!dispatcher_->InitHostWithChannel(
-            dispatcher_delegate_.get(),
-            channel_handle, true, preferences)) {
+    if (!dispatcher_->InitHostWithChannel(dispatcher_delegate_.get(),
+                                          channel_handle,
+                                          true,  // Client.
+                                          preferences)) {
       dispatcher_.reset();
       dispatcher_delegate_.reset();
       return false;
@@ -480,13 +489,21 @@ BrokerDispatcherWrapper::~BrokerDispatcherWrapper() {
 bool BrokerDispatcherWrapper::Init(
     base::ProcessHandle broker_process_handle,
     const IPC::ChannelHandle& channel_handle) {
+  if (channel_handle.name.empty())
+    return false;
+
+#if defined(OS_POSIX)
+  if (channel_handle.socket.fd == -1)
+    return false;
+#endif
+
   dispatcher_delegate_.reset(new DispatcherDelegate);
   dispatcher_.reset(
       new ppapi::proxy::BrokerHostDispatcher(broker_process_handle));
 
   if (!dispatcher_->InitBrokerWithChannel(dispatcher_delegate_.get(),
                                           channel_handle,
-                                          true)) {
+                                          true)) {  // Client.
     dispatcher_.reset();
     dispatcher_delegate_.reset();
     return false;
@@ -609,7 +626,7 @@ void PpapiBrokerImpl::OnBrokerChannelConnected(
   if (dispatcher->Init(broker_process_handle, channel_handle)) {
     dispatcher_.reset(dispatcher.release());
 
-    // Process all pending channel requests from the renderers.
+    // Process all pending channel requests from the plugins.
     for (ClientMap::iterator i = pending_connects_.begin();
          i != pending_connects_.end(); ++i) {
       base::WeakPtr<webkit::ppapi::PPB_Broker_Impl>& weak_ptr = i->second;
diff --git a/content/renderer/pepper_plugin_delegate_impl_unittest.cc b/content/renderer/pepper_plugin_delegate_impl_unittest.cc
new file mode 100644
index 0000000..73baa25
--- /dev/null
+++ b/content/renderer/pepper_plugin_delegate_impl_unittest.cc
@@ -0,0 +1,58 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/renderer/pepper_plugin_delegate_impl.h"
+
+#if defined(OS_POSIX)
+#include <fcntl.h>
+#include <sys/socket.h>
+#endif  // defined(OS_POSIX)
+
+#include "content/test/mock_render_process.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class PepperPluginDelegateImplTest : public ::testing::Test {
+ protected:
+  MessageLoopForIO message_loop_;
+  // We need a render process for ppapi::proxy::ProxyChannel to work.
+  MockRenderProcess mock_process_;
+};
+
+// Try to initialize BrokerDispatcherWrapper with invalid ChannelHandle.
+// Initialization should fail.
+TEST_F(PepperPluginDelegateImplTest, BrokerDispatcherWrapperInitFailure) {
+  BrokerDispatcherWrapper dispatcher_wrapper;
+  base::ProcessHandle broker_process_handle = base::kNullProcessHandle;
+  IPC::ChannelHandle invalid_channel;  // Invalid by default.
+
+  // An invalid handle should result in a failure (false) without a LOG(FATAL),
+  // 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 and go through the LOG(FATAL) path.
+  EXPECT_FALSE(dispatcher_wrapper.Init(broker_process_handle, invalid_channel));
+  EXPECT_FALSE(dispatcher_wrapper.Init(broker_process_handle, invalid_channel));
+}
+
+// On valid ChannelHandle, initialization should succeed.
+TEST_F(PepperPluginDelegateImplTest, BrokerDispatcherWrapperInitSuccess) {
+  BrokerDispatcherWrapper dispatcher_wrapper;
+  base::ProcessHandle broker_process_handle = base::kNullProcessHandle;
+  const char kChannelName[] = "PepperPluginDelegateImplTestChannelName";
+#if defined(OS_POSIX)
+  int fds[2] = {-1, -1};
+  ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_STREAM, 0, fds));
+  // Channel::ChannelImpl::CreatePipe needs the fd to be non-blocking.
+  ASSERT_EQ(0, fcntl(fds[1], F_SETFL, O_NONBLOCK));
+  base::FileDescriptor file_descriptor(fds[1], true);  // Auto close.
+  IPC::ChannelHandle valid_channel(kChannelName, file_descriptor);
+#else
+  IPC::ChannelHandle valid_channel(kChannelName);
+#endif  // defined(OS_POSIX));
+
+  EXPECT_TRUE(dispatcher_wrapper.Init(broker_process_handle, valid_channel));
+
+#if defined(OS_POSIX)
+  EXPECT_EQ(0, ::close(fds[0]));
+#endif  // defined(OS_POSIX));
+}