Avoid leaking SerializedHandles.

This CL automatically closes SerializedHandles at the receiving side of ResourceMessageParams (the host side for ResourceMessageCallParams; the plugin side for ResourceMessageReplyParams), if they are not taken by message handlers.

BUG=None
TEST=None


Review URL: https://chromiumcodereview.appspot.com/11312017

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

5 files changed, 136 insertions, 45 deletions

diff --git a/ppapi/proxy/gamepad_resource.cc b/ppapi/proxy/gamepad_resource.cc
index 3055513..8a49437 100644
--- a/ppapi/proxy/gamepad_resource.cc
+++ b/ppapi/proxy/gamepad_resource.cc
@@ -98,8 +98,8 @@ void GamepadResource::Sample(PP_GamepadsSampleData* data) {
 void GamepadResource::OnPluginMsgSendMemory(
     const ResourceMessageReplyParams& params) {
   // On failure, the handle will be null and the CHECK below will be tripped.
-  base::SharedMemoryHandle handle;
-  params.GetSharedMemoryHandleAtIndex(0, &handle);
+  base::SharedMemoryHandle handle = base::SharedMemory::NULLHandle();
+  params.TakeSharedMemoryHandleAtIndex(0, &handle);
 
   shared_memory_.reset(new base::SharedMemory(handle, true));
   CHECK(shared_memory_->Map(sizeof(ContentGamepadHardwareBuffer)));
diff --git a/ppapi/proxy/resource_message_params.cc b/ppapi/proxy/resource_message_params.cc
index 6d3fa02..9228d7e 100644
--- a/ppapi/proxy/resource_message_params.cc
+++ b/ppapi/proxy/resource_message_params.cc
@@ -4,21 +4,37 @@
 
 #include "ppapi/proxy/resource_message_params.h"
 
+#include "base/logging.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/proxy/ppapi_messages.h"
 
 namespace ppapi {
 namespace proxy {
 
+ResourceMessageParams::SerializedHandles::SerializedHandles()
+    : should_close_(false) {
+}
+
+ResourceMessageParams::SerializedHandles::~SerializedHandles() {
+  if (should_close_) {
+    for (std::vector<SerializedHandle>::iterator iter = data_.begin();
+         iter != data_.end(); ++iter) {
+      iter->Close();
+    }
+  }
+}
+
 ResourceMessageParams::ResourceMessageParams()
     : pp_resource_(0),
-      sequence_(0) {
+      sequence_(0),
+      handles_(new SerializedHandles()) {
 }
 
 ResourceMessageParams::ResourceMessageParams(PP_Resource resource,
                                              int32_t sequence)
     : pp_resource_(resource),
-      sequence_(sequence) {
+      sequence_(sequence),
+      handles_(new SerializedHandles()) {
 }
 
 ResourceMessageParams::~ResourceMessageParams() {
@@ -27,51 +43,56 @@ ResourceMessageParams::~ResourceMessageParams() {
 void ResourceMessageParams::Serialize(IPC::Message* msg) const {
   IPC::ParamTraits<PP_Resource>::Write(msg, pp_resource_);
   IPC::ParamTraits<int32_t>::Write(msg, sequence_);
-  IPC::ParamTraits<std::vector<SerializedHandle> >::Write(msg, handles_);
+  IPC::ParamTraits<std::vector<SerializedHandle> >::Write(msg,
+                                                          handles_->data());
 }
 
 bool ResourceMessageParams::Deserialize(const IPC::Message* msg,
                                         PickleIterator* iter) {
+  DCHECK(handles_->data().empty());
+  handles_->set_should_close(true);
   return IPC::ParamTraits<PP_Resource>::Read(msg, iter, &pp_resource_) &&
          IPC::ParamTraits<int32_t>::Read(msg, iter, &sequence_) &&
          IPC::ParamTraits<std::vector<SerializedHandle> >::Read(
-             msg, iter, &handles_);
+             msg, iter, &handles_->data());
 }
 
-const SerializedHandle* ResourceMessageParams::GetHandleOfTypeAtIndex(
+SerializedHandle ResourceMessageParams::TakeHandleOfTypeAtIndex(
     size_t index,
     SerializedHandle::Type type) const {
-  if (handles_.size() <= index)
-    return NULL;
-  if (handles_[index].type() != type)
-    return NULL;
-  return &handles_[index];
+  SerializedHandle handle;
+  std::vector<SerializedHandle>& data = handles_->data();
+  if (index < data.size() && data[index].type() == type) {
+    handle = data[index];
+    data[index] = SerializedHandle();
+  }
+  return handle;
 }
 
-bool ResourceMessageParams::GetSharedMemoryHandleAtIndex(
+bool ResourceMessageParams::TakeSharedMemoryHandleAtIndex(
     size_t index,
     base::SharedMemoryHandle* handle) const {
-  const SerializedHandle* serialized = GetHandleOfTypeAtIndex(
+  SerializedHandle serialized = TakeHandleOfTypeAtIndex(
       index, SerializedHandle::SHARED_MEMORY);
-  if (!serialized)
+  if (!serialized.is_shmem())
     return false;
-  *handle = serialized->shmem();
+  *handle = serialized.shmem();
   return true;
 }
 
-bool ResourceMessageParams::GetSocketHandleAtIndex(
+bool ResourceMessageParams::TakeSocketHandleAtIndex(
     size_t index,
     IPC::PlatformFileForTransit* handle) const {
-  const SerializedHandle* serialized = GetHandleOfTypeAtIndex(
+  SerializedHandle serialized = TakeHandleOfTypeAtIndex(
       index, SerializedHandle::SOCKET);
-  if (!serialized)
+  if (!serialized.is_socket())
     return false;
-  *handle = serialized->descriptor();
+  *handle = serialized.descriptor();
   return true;
 }
 
-void ResourceMessageParams::AppendHandle(const SerializedHandle& handle) {
-  handles_.push_back(handle);
+void ResourceMessageParams::AppendHandle(const SerializedHandle& handle) const {
+  handles_->data().push_back(handle);
 }
 
 ResourceMessageCallParams::ResourceMessageCallParams()
diff --git a/ppapi/proxy/resource_message_params.h b/ppapi/proxy/resource_message_params.h
index bc52234..780adb0 100644
--- a/ppapi/proxy/resource_message_params.h
+++ b/ppapi/proxy/resource_message_params.h
@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "base/memory/ref_counted.h"
 #include "ipc/ipc_message_utils.h"
 #include "ppapi/c/pp_resource.h"
 #include "ppapi/proxy/ppapi_proxy_export.h"
@@ -23,32 +24,40 @@ class PPAPI_PROXY_EXPORT ResourceMessageParams {
   PP_Resource pp_resource() const { return pp_resource_; }
   int32_t sequence() const { return sequence_; }
 
-  const std::vector<SerializedHandle> handles() const { return handles_; }
-
-  // Returns a pointer to the handle at the given index if it exists and is of
-  // the given type. If the index doesn't exist or the handle isn't of the
-  // given type, returns NULL. Note that the pointer will be into an internal
-  // vector so will be invalidated if the params are mutated.
-  const SerializedHandle* GetHandleOfTypeAtIndex(
-      size_t index,
-      SerializedHandle::Type type) const;
-
-  // Helper functions to return shared memory handles passed in the params
-  // struct. If the index has a valid handle of the given type, it will be
-  // placed in the output parameter and the function will return true. If the
-  // handle doesn't exist or is a different type, the functions will return
-  // false and the output parameter will be untouched.
+  // Note that the caller doesn't take ownership of the returned handles.
+  const std::vector<SerializedHandle>& handles() const {
+    return handles_->data();
+  }
+
+  // Returns the handle at the given index if it exists and is of the given
+  // type. The corresponding slot in the list is set to an invalid handle.
+  // If the index doesn't exist or the handle isn't of the given type, returns
+  // an invalid handle.
+  // Note that the caller is responsible for closing the returned handle, if it
+  // is valid.
+  SerializedHandle TakeHandleOfTypeAtIndex(size_t index,
+                                           SerializedHandle::Type type) const;
+
+  // Helper functions to return shared memory or socket handles passed in the
+  // params struct.
+  // If the index has a valid handle of the given type, it will be placed in the
+  // output parameter, the corresponding slot in the list will be set to an
+  // invalid handle, and the function will return true. If the handle doesn't
+  // exist or is a different type, the functions will return false and the
+  // output parameter will be untouched.
   //
-  // Note that the handle could still be a "null" or invalid handle of
-  // the right type and the functions will succeed.
-  bool GetSharedMemoryHandleAtIndex(size_t index,
-                                    base::SharedMemoryHandle* handle) const;
-  bool GetSocketHandleAtIndex(size_t index,
-                              IPC::PlatformFileForTransit* handle) const;
+  // Note: 1) the handle could still be a "null" or invalid handle of the right
+  //          type and the functions will succeed.
+  //       2) the caller is responsible for closing the returned handle, if it
+  //          is valid.
+  bool TakeSharedMemoryHandleAtIndex(size_t index,
+                                     base::SharedMemoryHandle* handle) const;
+  bool TakeSocketHandleAtIndex(size_t index,
+                               IPC::PlatformFileForTransit* handle) const;
 
   // Appends the given handle to the list of handles sent with the call or
   // reply.
-  void AppendHandle(const SerializedHandle& handle);
+  void AppendHandle(const SerializedHandle& handle) const;
 
  protected:
   ResourceMessageParams();
@@ -58,6 +67,29 @@ class PPAPI_PROXY_EXPORT ResourceMessageParams {
   virtual bool Deserialize(const IPC::Message* msg, PickleIterator* iter);
 
  private:
+  class SerializedHandles : public base::RefCounted<SerializedHandles> {
+   public:
+    SerializedHandles();
+    ~SerializedHandles();
+
+    void set_should_close(bool value) { should_close_ = value; }
+    std::vector<SerializedHandle>& data() { return data_; }
+
+   private:
+    friend class base::RefCounted<SerializedHandles>;
+
+    // Whether the handles stored in |data_| should be closed when this object
+    // goes away.
+    //
+    // It is set to true by ResourceMessageParams::Deserialize(), so that the
+    // receiving side of the params (the host side for
+    // ResourceMessageCallParams; the plugin side for
+    // ResourceMessageReplyParams) will close those handles which haven't been
+    // taken using any of the Take*() methods.
+    bool should_close_;
+    std::vector<SerializedHandle> data_;
+  };
+
   PP_Resource pp_resource_;
 
   // Identifier for this message. Sequence numbers are quasi-unique within a
@@ -78,7 +110,9 @@ class PPAPI_PROXY_EXPORT ResourceMessageParams {
   // A list of all handles transferred in the message. Handles go here so that
   // the NaCl adapter can extract them generally when it rewrites them to
   // go between Windows and NaCl (Posix) apps.
-  std::vector<SerializedHandle> handles_;
+  // TODO(yzshen): Mark it as mutable so that we can take/append handles using a
+  // const reference. We need to change all the callers and make it not mutable.
+  mutable scoped_refptr<SerializedHandles> handles_;
 };
 
 // Parameters common to all ResourceMessage "Call" requests.
diff --git a/ppapi/proxy/serialized_structs.cc b/ppapi/proxy/serialized_structs.cc
index 6622d4a..f83ae0b 100644
--- a/ppapi/proxy/serialized_structs.cc
+++ b/ppapi/proxy/serialized_structs.cc
@@ -5,11 +5,19 @@
 #include "ppapi/proxy/serialized_structs.h"
 
 #include "base/pickle.h"
+#include "base/platform_file.h"
+#include "base/shared_memory.h"
+#include "build/build_config.h"
+#include "ipc/ipc_platform_file.h"
 #include "ppapi/c/dev/ppb_font_dev.h"
 #include "ppapi/c/pp_file_info.h"
 #include "ppapi/c/pp_rect.h"
 #include "ppapi/shared_impl/var.h"
 
+#if defined(OS_NACL)
+#include <unistd.h>
+#endif
+
 namespace ppapi {
 namespace proxy {
 
@@ -106,6 +114,31 @@ bool SerializedHandle::IsHandleValid() const {
   return false;
 }
 
+void SerializedHandle::Close() {
+  if (IsHandleValid()) {
+    switch (type_) {
+      case INVALID:
+        NOTREACHED();
+        break;
+      case SHARED_MEMORY:
+        base::SharedMemory::CloseHandle(shm_handle_);
+        break;
+      case SOCKET:
+      case CHANNEL_HANDLE:
+        base::PlatformFile file =
+            IPC::PlatformFileForTransitToPlatformFile(descriptor_);
+#if !defined(OS_NACL)
+        base::ClosePlatformFile(file);
+#else
+        close(file);
+#endif
+        break;
+      // No default so the compiler will warn us if a new type is added.
+    }
+  }
+  *this = SerializedHandle();
+}
+
 // static
 bool SerializedHandle::WriteHeader(const Header& hdr, Pickle* pickle) {
   if (!pickle->WriteInt(hdr.type))
diff --git a/ppapi/proxy/serialized_structs.h b/ppapi/proxy/serialized_structs.h
index 1779fd8..e577a8f 100644
--- a/ppapi/proxy/serialized_structs.h
+++ b/ppapi/proxy/serialized_structs.h
@@ -164,6 +164,9 @@ class PPAPI_PROXY_EXPORT SerializedHandle {
     return Header(type_, size_);
   }
 
+  // Closes the handle and sets it to invalid.
+  void Close();
+
   // Write/Read a Header, which contains all the data except the handle. This
   // allows us to write the handle in a platform-specific way, as is necessary
   // in NaClIPCAdapter to share handles with NaCl from Windows.