Mojo: Ensure partially serialized message handles don't leak

For the sake of efficiency, we don't fully serialize handles in a message until we're ready to freeze the message contents for transmission. Unfortunately we were not properly closing these partially serialized handles in the event that the message was never fully serialized before destruction. This fixes that. [email protected] [email protected] (cherry picked from commit 7d450d1) Bug: 775592 Change-Id: Idac3b11359dc2fcec0699cef779de809575ea163 Reviewed-on: https://chromium-review.googlesource.com/723771 Reviewed-by: Jay Civelli <[email protected]> Commit-Queue: Ken Rockot <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#509600} Reviewed-on: https://chromium-review.googlesource.com/727403 Reviewed-by: Ken Rockot <[email protected]> Cr-Commit-Position: refs/branch-heads/3239@{#60} Cr-Branched-From: adb61db-refs/heads/master@{#508578}
nwjs · Oct 19, 2017 · b050aad · b050aad
1 parent c2853e6
commit b050aad
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 1 deletion.
diff --git a/mojo/edk/system/message_unittest.cc b/mojo/edk/system/message_unittest.cc
@@ -899,6 +899,35 @@ DEFINE_TEST_CLIENT_TEST_WITH_PIPE(ReadMessageAndCheckPipe, MessageTest, h) {
 
 #endif  // !defined(OS_IOS)
 
+TEST_F(MessageTest, PartiallySerializedMessagesDontLeakHandles) {
+  MojoMessageHandle message;
+  EXPECT_EQ(MOJO_RESULT_OK, MojoCreateMessage(&message));
+
+  MojoHandle handles[2];
+  CreateMessagePipe(&handles[0], &handles[1]);
+
+  const std::string kTestMessagePart1("hello i am message.");
+  void* buffer;
+  uint32_t buffer_size;
+  EXPECT_EQ(MOJO_RESULT_OK,
+            MojoAttachSerializedMessageBuffer(
+                message, static_cast<uint32_t>(kTestMessagePart1.size()),
+                nullptr, 0, &buffer, &buffer_size));
+  ASSERT_GE(buffer_size, static_cast<uint32_t>(kTestMessagePart1.size()));
+  memcpy(buffer, kTestMessagePart1.data(), kTestMessagePart1.size());
+
+  EXPECT_EQ(MOJO_RESULT_OK,
+            MojoExtendSerializedMessagePayload(
+                message, static_cast<uint32_t>(kTestMessagePart1.size()),
+                handles, 1, &buffer, &buffer_size));
+
+  // This must close |handles[0]|, which we can detect by observing the
+  // signal state of |handles[1].
+  EXPECT_EQ(MOJO_RESULT_OK, MojoDestroyMessage(message));
+  EXPECT_EQ(MOJO_RESULT_OK,
+            WaitForSignals(handles[1], MOJO_HANDLE_SIGNAL_PEER_CLOSED));
+}
+
 }  // namespace
 }  // namespace edk
 }  // namespace mojo
diff --git a/mojo/edk/system/user_message_impl.cc b/mojo/edk/system/user_message_impl.cc
@@ -279,7 +279,9 @@ UserMessageImpl::~UserMessageImpl() {
 
     if (!pending_handle_attachments_.empty()) {
       internal::g_core->ReleaseDispatchersForTransit(
-          pending_handle_attachments_, true);
+          pending_handle_attachments_, false);
+      for (const auto& dispatcher : pending_handle_attachments_)
+        MojoClose(dispatcher.local_handle);
     }
   }
 }