Avoid leaking SerializedHandles.

ppapi/proxy/resource_message_params.h

Index: ppapi/proxy/resource_message_params.h
diff --git a/ppapi/proxy/resource_message_params.h b/ppapi/proxy/resource_message_params.h
index bc522343e5e9d4b0dcb3d6bcbda001abc02bf3a7..c22de18f4530305d4ddf3be4dfa278bb06a2e64b 100644
--- a/ppapi/proxy/resource_message_params.h
+++ b/ppapi/proxy/resource_message_params.h
@@ -7,6 +7,8 @@
 
 #include <vector>
 
+#include "base/basictypes.h"
+#include "base/memory/linked_ptr.h"
 #include "ipc/ipc_message_utils.h"
 #include "ppapi/c/pp_resource.h"
 #include "ppapi/proxy/ppapi_proxy_export.h"
@@ -23,32 +25,38 @@ 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_; }
+  // Note that the caller doesn't take ownership of the returned handles.
+  const std::vector<SerializedHandle>& handles() const {
+    return handles_->data;
+  }
 
-  // Returns a pointer to the handle at the given index if it exists and is of
+  // Removes and returns the handle at the given index if it exists and is of

[brettw, 2012/10/31 20:15:59]

For all of these functions, can you be clear that the list isn't modified, just
the handle is nulled out? When you use "remove" I assume that I would keep
calling Take...(0) to eventually consume all of the handles, since it seems like
each call is removing the front one from the list.

[yzshen1, 2012/10/31 21:15:27]

Done.

   // 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(
+  // 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 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.
+  // 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 removed from this struct and 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 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 +66,24 @@ class PPAPI_PROXY_EXPORT ResourceMessageParams {
   virtual bool Deserialize(const IPC::Message* msg, PickleIterator* iter);
 
  private:
+  struct SerializedHandles {
+    SerializedHandles();
+    ~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;
+
+    DISALLOW_COPY_AND_ASSIGN(SerializedHandles);
+  };
+
   PP_Resource pp_resource_;
 
   // Identifier for this message. Sequence numbers are quasi-unique within a
@@ -78,7 +104,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_;
+  // Mark it as mutable so that we can take/append handles using a const
+  // reference.
+  mutable linked_ptr<SerializedHandles> handles_;

[brettw, 2012/10/31 20:15:59]

I think linked_ptr is really to give you refcounting semantics when you don't
want your class to be refcounted. Since you're defining the class here, I think
it's cleaner to just make it refcounted and use a scoped_refptr here.

[yzshen1, 2012/10/31 21:15:27]

Done.

 };
 
 // Parameters common to all ResourceMessage "Call" requests.