Avoid leaking SerializedHandles.

ppapi/proxy/resource_message_params.h

 // Copyright (c) 2012 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.
 
 #ifndef PPAPI_PROXY_RESOURCE_MESSAGE_PARAMS_H_
 #define PPAPI_PROXY_RESOURCE_MESSAGE_PARAMS_H_
 
 #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"
 #include "ppapi/proxy/serialized_structs.h"
 
 namespace ppapi {
 namespace proxy {
 
 // Common parameters for resource call and reply params structures below.
 class PPAPI_PROXY_EXPORT ResourceMessageParams {
  public:
   virtual ~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();
   ResourceMessageParams(PP_Resource resource, int32_t sequence);
 
   virtual void Serialize(IPC::Message* msg) const;
   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
   // resource, but will overlap between different resource objects.
   //
   // If you send a lot of messages, the ID may wrap around. This is OK. All IDs
   // are valid and 0 and -1 aren't special, so those cases won't confuse us.
   // In practice, if you send more than 4 billion messages for a resource, the
   // old ones will be long gone and there will be no collisions.
   //
   // If there is a malicious plugin (or exceptionally bad luck) that causes a
   // wraparound and collision the worst that will happen is that we can get
   // confused between different callbacks. But since these can only cause
   // confusion within the plugin and within callbacks on the same resource,
   // there shouldn't be a security problem.
   int32_t sequence_;
 
   // 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.
 class PPAPI_PROXY_EXPORT ResourceMessageCallParams
     : public ResourceMessageParams {
  public:
   ResourceMessageCallParams();
   ResourceMessageCallParams(PP_Resource resource, int32_t sequence);
   virtual ~ResourceMessageCallParams();
 
@@ skipping 55 matching lines @@
   static bool Read(const Message* m, PickleIterator* iter, param_type* r) {
     return r->Deserialize(m, iter);
   }
   static void Log(const param_type& p, std::string* l) {
   }
 };
 
 }  // namespace IPC
 
 #endif  // PPAPI_PROXY_RESOURCE_MESSAGE_PARAMS_H_