[mojo] Release sent handles earlier on Windows

This releases HANDLE ownership *before* sending any handle in a
message to a remote process, rather than after. Avoids an extremely
subtle race between DuplicateHandle+DUPLICATE_CLOSE_SOURCE in the
receiving process racing with ScopedHandle release in the sending
process. The trade-off is the introduction of potential leaks which
turn out to not really matter in any practical scenarios.

Bug: 900655
Change-Id: Ieec82bb062bc66cdb3c732c920a220ea2aa2f8d0
Reviewed-on: https://chromium-review.googlesource.com/c/1310505
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604510}

mojo/core/channel.cc

@@ -447,10 +447,6 @@ std::vector<PlatformHandleInTransit> Channel::Message::TakeHandles() {
     mach_ports_header_->num_ports = 0;
   }
 #endif
-  if (is_legacy_message())
-    legacy_header()->num_handles = 0;
-  else
-    header()->num_handles = 0;
   return std::move(handle_vector_);
 }
 

mojo/core/channel_win.cc

@@ -81,7 +81,7 @@ class ChannelWin : public Channel,
 
       bool write_now = !delay_writes_ && outgoing_messages_.empty();
       outgoing_messages_.emplace_back(std::move(message));
-      if (write_now && !WriteNoLock(outgoing_messages_.front()))
+      if (write_now && !WriteNoLock(outgoing_messages_.front().get()))
         reject_writes_ = write_error = true;
     }
     if (write_error) {
@@ -265,11 +265,6 @@ class ChannelWin : public Channel,
       Channel::MessagePtr message = std::move(outgoing_messages_.front());
       outgoing_messages_.pop_front();
 
-      // Invalidate all the scoped handles so we don't attempt to close them.
-      std::vector<PlatformHandleInTransit> handles = message->TakeHandles();
-      for (auto& handle : handles)
-        handle.CompleteTransit();
-
       // Overlapped WriteFile() to a pipe should always fully complete.
       if (message->data_num_bytes() != bytes_written)
         reject_writes_ = write_error = true;
@@ -298,10 +293,24 @@ class ChannelWin : public Channel,
     }
   }
 
-  // Attempts to write a message directly to the channel. If the full message
-  // cannot be written, it's queued and a wait is initiated to write the message
-  // ASAP on the I/O thread.
-  bool WriteNoLock(const Channel::MessagePtr& message) {
+  bool WriteNoLock(Channel::Message* message) {
+    // We can release all the handles immediately now that we're attempting an
+    // actual write to the remote process.
+    //
+    // If the HANDLE values are locally owned, that means we're sending them
+    // to a broker who will duplicate-and-close them. If the broker never
+    // receives that message (and thus we effectively leak these handles),
+    // either it died (and our total dysfunction is imminent) or we died; in
+    // either case the handle leak doesn't matter.
+    //
+    // If the handles have already been transferred and are therefore remotely
+    // owned, the only way they won't eventually be managed by the remote
+    // process is if the remote process dies before receiving this message. At
+    // that point, again, potential handle leaks don't matter.
+    std::vector<PlatformHandleInTransit> handles = message->TakeHandles();
+    for (auto& handle : handles)
+      handle.CompleteTransit();
+
     BOOL ok = WriteFile(handle_.Get(), message->data(),
                         static_cast<DWORD>(message->data_num_bytes()), NULL,
                         &write_context_.overlapped);
@@ -316,7 +325,7 @@ class ChannelWin : public Channel,
   bool WriteNextNoLock() {
     if (outgoing_messages_.empty())
       return true;
-    return WriteNoLock(outgoing_messages_.front());
+    return WriteNoLock(outgoing_messages_.front().get());
   }
 
   void OnWriteError(Error error) {