ipc: Update Windows attachment brokering ownership semantics.

I updated the semantics to match the Mac attachment brokering semantics,
which are much more precise. This fixes a HANDLE leak.

BUG=493414

Review URL: https://codereview.chromium.org/1494313002

Cr-Commit-Position: refs/heads/master@{#363296}

ipc/attachment_broker_privileged.h

@@ -85,6 +85,11 @@ class IPC_EXPORT AttachmentBrokerPrivileged : public IPC::AttachmentBroker {
     // The broker did not have a channel of communication with the source
     // process.
     ERROR_SOURCE_NOT_FOUND = 12,
+    // The broker could not open the source or destination process with extra
+    // privileges.
+    ERROR_COULD_NOT_OPEN_SOURCE_OR_DEST = 13,
+    // The broker was asked to transfer a HANDLE with invalid permissions.
+    ERROR_INVALID_PERMISSIONS = 14,
     ERROR_MAX
   };
 

ipc/attachment_broker_privileged_win.cc

@@ -102,35 +102,41 @@ AttachmentBrokerPrivilegedWin::HandleWireFormat
 AttachmentBrokerPrivilegedWin::DuplicateWinHandle(
     const HandleWireFormat& wire_format,
     base::ProcessId source_pid) {
+  // If the source process is the destination process, then no additional work
+  // is required.
+  if (source_pid == wire_format.destination_process)
+    return wire_format;
+
   base::Process source_process =
       base::Process::OpenWithExtraPrivileges(source_pid);
   base::Process dest_process =
       base::Process::OpenWithExtraPrivileges(wire_format.destination_process);
-  int new_wire_format_handle = 0;
-  if (source_process.Handle() && dest_process.Handle()) {
-    DWORD desired_access = 0;
-    DWORD options = 0;
-    switch (wire_format.permissions) {
-      case HandleWin::INVALID:
-        LOG(ERROR) << "Received invalid permissions for duplication.";
-        return CopyWireFormat(wire_format, 0);
-      case HandleWin::DUPLICATE:
-        options = DUPLICATE_SAME_ACCESS;
-        break;
-      case HandleWin::FILE_READ_WRITE:
-        desired_access = FILE_GENERIC_READ | FILE_GENERIC_WRITE;
-        break;
-    }
-
-    HANDLE new_handle;
-    HANDLE original_handle = LongToHandle(wire_format.handle);
-    DWORD result = ::DuplicateHandle(source_process.Handle(), original_handle,
-                                     dest_process.Handle(), &new_handle,
-                                     desired_access, FALSE, options);
+  if (!source_process.Handle() || !dest_process.Handle()) {
+    LogError(ERROR_COULD_NOT_OPEN_SOURCE_OR_DEST);
+    return wire_format;
+  }
 
-    new_wire_format_handle = (result != 0) ? HandleToLong(new_handle) : 0;
+  DWORD desired_access = 0;
+  DWORD options = DUPLICATE_CLOSE_SOURCE;
+  switch (wire_format.permissions) {
+    case HandleWin::INVALID:
+      LogError(ERROR_INVALID_PERMISSIONS);
+      return CopyWireFormat(wire_format, 0);
+    case HandleWin::DUPLICATE:
+      options |= DUPLICATE_SAME_ACCESS;
+      break;
+    case HandleWin::FILE_READ_WRITE:
+      desired_access = FILE_GENERIC_READ | FILE_GENERIC_WRITE;
+      break;
   }
 
+  HANDLE new_handle;
+  HANDLE original_handle = LongToHandle(wire_format.handle);
+  DWORD result = ::DuplicateHandle(source_process.Handle(), original_handle,
+                                   dest_process.Handle(), &new_handle,
+                                   desired_access, FALSE, options);
+
+  int new_wire_format_handle = (result != 0) ? HandleToLong(new_handle) : 0;
   return CopyWireFormat(wire_format, new_wire_format_handle);
 }
 

ipc/attachment_broker_privileged_win_unittest.cc

@@ -115,6 +115,13 @@ bool CheckContentsOfTestMessage(const IPC::Message& message) {
   return success;
 }
 
+// Returns 0 on error.
+DWORD GetCurrentProcessHandleCount() {
+  DWORD handle_count = 0;
+  BOOL success = ::GetProcessHandleCount(::GetCurrentProcess(), &handle_count);
+  return success ? handle_count : 0;
+}
+
 enum TestResult {
   RESULT_UNKNOWN,
   RESULT_SUCCESS,
@@ -230,9 +237,14 @@ class IPCAttachmentBrokerPrivilegedWinTest : public IPCTestBase {
     broker_->DesignateBrokerCommunicationChannel(channel());
     ASSERT_TRUE(ConnectChannel());
     ASSERT_TRUE(StartClient());
+
+    handle_count_ = GetCurrentProcessHandleCount();
+    EXPECT_NE(handle_count_, 0u);
   }
 
   void CommonTearDown() {
+    EXPECT_EQ(handle_count_, handle_count_);
+
     // Close the channel so the client's OnChannelError() gets fired.
     channel()->Close();
 
@@ -268,6 +280,7 @@ class IPCAttachmentBrokerPrivilegedWinTest : public IPCTestBase {
   ProxyListener proxy_listener_;
   scoped_ptr<IPC::AttachmentBrokerUnprivilegedWin> broker_;
   MockObserver observer_;
+  DWORD handle_count_;
 };
 
 // A broker which always sets the current process as the destination process
@@ -358,12 +371,11 @@ TEST_F(IPCAttachmentBrokerPrivilegedWinTest, SendHandleToSelf) {
   get_broker()->GetAttachmentWithId(*id, &received_attachment);
   ASSERT_NE(received_attachment.get(), nullptr);
 
-  // Check that it's a new entry in the HANDLE table.
+  // Check that it's the same entry in the HANDLE table.
   HANDLE h2 = GetHandleFromBrokeredAttachment(received_attachment);
-  EXPECT_NE(h2, h);
-  EXPECT_NE(h2, nullptr);
+  EXPECT_EQ(h2, h);
 
-  // But it still points to the same file.
+  // And still points to the same file.
   std::string contents = ReadFromFile(h);
   EXPECT_EQ(contents, std::string(kDataBuffer));
 
@@ -380,8 +392,12 @@ TEST_F(IPCAttachmentBrokerPrivilegedWinTest, SendTwoHandles) {
   get_proxy_listener()->set_listener(&result_listener);
 
   HANDLE h = CreateTempFile();
+  HANDLE h2;
+  BOOL result = ::DuplicateHandle(GetCurrentProcess(), h, GetCurrentProcess(),
+                                  &h2, 0, FALSE, DUPLICATE_SAME_ACCESS);
+  ASSERT_TRUE(result);
   IPC::HandleWin handle_win1(h, IPC::HandleWin::FILE_READ_WRITE);
-  IPC::HandleWin handle_win2(h, IPC::HandleWin::FILE_READ_WRITE);
+  IPC::HandleWin handle_win2(h2, IPC::HandleWin::FILE_READ_WRITE);
   IPC::Message* message = new TestTwoHandleWinMsg(handle_win1, handle_win2);
   sender()->Send(message);
   base::MessageLoop::current()->Run();
@@ -403,8 +419,12 @@ TEST_F(IPCAttachmentBrokerPrivilegedWinTest, SendHandleTwice) {
   get_proxy_listener()->set_listener(&result_listener);
 
   HANDLE h = CreateTempFile();
+  HANDLE h2;
+  BOOL result = ::DuplicateHandle(GetCurrentProcess(), h, GetCurrentProcess(),
+                                  &h2, 0, FALSE, DUPLICATE_SAME_ACCESS);
+  ASSERT_TRUE(result);
   SendMessageWithAttachment(h);
-  SendMessageWithAttachment(h);
+  SendMessageWithAttachment(h2);
   base::MessageLoop::current()->Run();
 
   // Check the result.

ipc/attachment_broker_unprivileged_win.cc

@@ -21,12 +21,15 @@ bool AttachmentBrokerUnprivilegedWin::SendAttachmentToProcess(
     base::ProcessId destination_process) {
   switch (attachment->GetBrokerableType()) {
     case BrokerableAttachment::WIN_HANDLE: {
-      const internal::HandleAttachmentWin* handle_attachment =
-          static_cast<const internal::HandleAttachmentWin*>(attachment.get());
+      internal::HandleAttachmentWin* handle_attachment =
+          static_cast<internal::HandleAttachmentWin*>(attachment.get());
       internal::HandleAttachmentWin::WireFormat format =
           handle_attachment->GetWireFormat(destination_process);
-      return get_sender()->Send(
+      bool success = get_sender()->Send(
           new AttachmentBrokerMsg_DuplicateWinHandle(format));
+      if (success)
+        handle_attachment->reset_handle_ownership();
+      return success;
     }
     case BrokerableAttachment::MACH_PORT:
     case BrokerableAttachment::PLACEHOLDER:

ipc/handle_attachment_win.cc

@@ -11,20 +11,22 @@ namespace internal {
 
 HandleAttachmentWin::HandleAttachmentWin(const HANDLE& handle,
                                          HandleWin::Permissions permissions)
-    : handle_(handle), permissions_(permissions) {}
+    : handle_(handle), permissions_(permissions), owns_handle_(true) {}
 
 HandleAttachmentWin::HandleAttachmentWin(const WireFormat& wire_format)
     : BrokerableAttachment(wire_format.attachment_id),
       handle_(LongToHandle(wire_format.handle)),
-      permissions_(wire_format.permissions) {}
+      permissions_(wire_format.permissions), owns_handle_(false) {}
 
 HandleAttachmentWin::HandleAttachmentWin(
     const BrokerableAttachment::AttachmentId& id)
     : BrokerableAttachment(id),
       handle_(INVALID_HANDLE_VALUE),
-      permissions_(HandleWin::INVALID) {}
+      permissions_(HandleWin::INVALID), owns_handle_(false) {}
 
 HandleAttachmentWin::~HandleAttachmentWin() {
+  if (handle_ != INVALID_HANDLE_VALUE && owns_handle_)
+    ::CloseHandle(handle_);
 }
 
 HandleAttachmentWin::BrokerableType HandleAttachmentWin::GetBrokerableType()

ipc/handle_attachment_win.h

@@ -41,7 +41,7 @@ class IPC_EXPORT HandleAttachmentWin : public BrokerableAttachment {
     // The type is int32_t instead of HANDLE because HANDLE gets typedefed to
     // void*, whose size varies between 32 and 64-bit processes. Using a
     // int32_t means that 64-bit processes will need to perform both up-casting
-    // and down-casting. This is performed using the appropriate Windows apis.
+    // and down-casting. This is performed using the appropriate Windows APIs.
     // A value of 0 is equivalent to an invalid handle.
     int32_t handle;
 
@@ -54,7 +54,12 @@ class IPC_EXPORT HandleAttachmentWin : public BrokerableAttachment {
     AttachmentId attachment_id;
   };
 
+  // This constructor makes a copy of |handle| and takes ownership of the
+  // result. Should only be called by the sender of a Chrome IPC message.
   HandleAttachmentWin(const HANDLE& handle, HandleWin::Permissions permissions);
+
+  // These constructors do not take ownership of the HANDLE, and should only be
+  // called by the receiver of a Chrome IPC message.
   explicit HandleAttachmentWin(const WireFormat& wire_format);
   explicit HandleAttachmentWin(const BrokerableAttachment::AttachmentId& id);
 
@@ -65,10 +70,21 @@ class IPC_EXPORT HandleAttachmentWin : public BrokerableAttachment {
 
   HANDLE get_handle() const { return handle_; }
 
+  // The caller of this method has taken ownership of |handle_|.
+  void reset_handle_ownership() { owns_handle_ = false; }
+
  private:
   ~HandleAttachmentWin() override;
   HANDLE handle_;
   HandleWin::Permissions permissions_;
+
+  // In the sender process, the attachment owns the HANDLE of a newly created
+  // message. The attachment broker will eventually take ownership, and set
+  // this member to |false|.
+  // In the destination process, the attachment never owns the Mach port. The
+  // client code that receives the Chrome IPC message is always expected to take
+  // ownership.
+  bool owns_handle_;
 };
 
 }  // namespace internal

ipc/handle_win.h

@@ -23,6 +23,10 @@ class Message;
 // HandleWin is a wrapper around a Windows HANDLE that can be transported
 // across Chrome IPC channels that support attachment brokering. The HANDLE will
 // be duplicated into the destination process.
+//
+// The ownership semantics for the underlying |handle_| are complex. See
+// ipc/mach_port_mac.h (the OSX analog of this class) for an extensive
+// discussion.
 class IPC_EXPORT HandleWin {
  public:
   enum Permissions {

tools/metrics/histograms/histograms.xml

@@ -66685,6 +66685,13 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   <int value="12" label="ERROR_SOURCE_NOT_FOUND">
     Broker didn't have a channel of communication with the source process.
   </int>
+  <int value="13" label="ERROR_COULD_NOT_OPEN_SOURCE_OR_DEST">
+    Broker could not open the source or destination process with extra
+    privileges.
+  </int>
+  <int value="14" label="ERROR_INVALID_PERMISSIONS">
+    Broker was asked to transfer a HANDLE with invalid permissions.
+  </int>
 </enum>
 
 <enum name="IPCAttachmentBrokerUnprivilegedBrokerAttachmentError" type="int">