Enforce/require MOJO_HANDLE_RIGHT_TRANSFER in sending handles via MojoWriteMessage().

R=azani@chromium.org

Review URL: https://codereview.chromium.org/2012613002 .
diff --git a/mojo/edk/system/core_unittest.cc b/mojo/edk/system/core_unittest.cc
index 005dede..b2a080b 100644
--- a/mojo/edk/system/core_unittest.cc
+++ b/mojo/edk/system/core_unittest.cc
@@ -1792,6 +1792,40 @@
   }
 }
 
+// Tests not having versus not having the transfer right.
+TEST_F(CoreTest, MessagePipeBasicLocalHandlePassing4) {
+  MojoHandle h0 = MOJO_HANDLE_INVALID;
+  MojoHandle h1 = MOJO_HANDLE_INVALID;
+  EXPECT_EQ(MOJO_RESULT_OK,
+            core()->CreateMessagePipe(NullUserPointer(), MakeUserPointer(&h0),
+                                      MakeUserPointer(&h1)));
+
+  MojoHandle h_transferrable = MOJO_HANDLE_INVALID;
+  EXPECT_EQ(MOJO_RESULT_OK,
+            core()->CreateSharedBuffer(NullUserPointer(), 100,
+                                       MakeUserPointer(&h_transferrable)));
+  MojoHandle h_not_transferrable = MOJO_HANDLE_INVALID;
+  EXPECT_EQ(MOJO_RESULT_OK, core()->DuplicateHandleWithReducedRights(
+                                h_transferrable, MOJO_HANDLE_RIGHT_TRANSFER,
+                                MakeUserPointer(&h_not_transferrable)));
+
+  // We can send |h_transferrable|.
+  EXPECT_EQ(MOJO_RESULT_OK,
+            core()->WriteMessage(h0, NullUserPointer(), 0,
+                                 MakeUserPointer(&h_transferrable), 1,
+                                 MOJO_WRITE_MESSAGE_FLAG_NONE));
+
+  // But not |h_not_transferrable|.
+  EXPECT_EQ(MOJO_RESULT_PERMISSION_DENIED,
+            core()->WriteMessage(h0, NullUserPointer(), 0,
+                                 MakeUserPointer(&h_not_transferrable), 1,
+                                 MOJO_WRITE_MESSAGE_FLAG_NONE));
+
+  EXPECT_EQ(MOJO_RESULT_OK, core()->Close(h0));
+  EXPECT_EQ(MOJO_RESULT_OK, core()->Close(h1));
+  EXPECT_EQ(MOJO_RESULT_OK, core()->Close(h_not_transferrable));
+}
+
 struct TestAsyncWaiter {
   TestAsyncWaiter() : result(MOJO_RESULT_UNKNOWN) {}
 
diff --git a/mojo/edk/system/handle_table.cc b/mojo/edk/system/handle_table.cc
index e123ae9..985e4ed 100644
--- a/mojo/edk/system/handle_table.cc
+++ b/mojo/edk/system/handle_table.cc
@@ -143,6 +143,11 @@
       error_result = MOJO_RESULT_BUSY;
       break;
     }
+    // Note: "Busy" is "preferred" over "permission denied".
+    if (!entries[i]->handle.has_all_rights(MOJO_HANDLE_RIGHT_TRANSFER)) {
+      error_result = MOJO_RESULT_PERMISSION_DENIED;
+      break;
+    }
     // Note: By marking the handle as busy here, we're also preventing the
     // same handle from being sent multiple times in the same message.
     entries[i]->busy = true;
diff --git a/mojo/edk/system/handle_table.h b/mojo/edk/system/handle_table.h
index 3b8e967..e1d91a0 100644
--- a/mojo/edk/system/handle_table.h
+++ b/mojo/edk/system/handle_table.h
@@ -75,8 +75,9 @@
   bool AddHandleVector(HandleVector* handles, MojoHandle* handle_values);
 
   // Tries to mark the given handle values as busy and start transport on them
-  // (i.e., take their dispatcher locks); |transports| must be sized to contain
-  // |num_handles| elements. On failure, returns them to their original
+  // (i.e., take their dispatcher locks). The handles to be transported must all
+  // have the |MOJO_HANDLE_RIGHT_TRANSFER| right. |transports| must be sized to
+  // contain |num_handles| elements. On failure, returns them to their original
   // (non-busy, unlocked state).
   MojoResult MarkBusyAndStartTransport(
       MojoHandle disallowed_handle,
diff --git a/mojo/edk/system/handle_table_unittest.cc b/mojo/edk/system/handle_table_unittest.cc
index 92bffd7..19ef3ff 100644
--- a/mojo/edk/system/handle_table_unittest.cc
+++ b/mojo/edk/system/handle_table_unittest.cc
@@ -247,6 +247,8 @@
     EXPECT_EQ(MOJO_RESULT_OK, handles[i].dispatcher->Close()) << i;
 }
 
+// TODO(vtl): Figure out how to test |MarkBusyAndStartTransport()|.
+
 }  // namespace
 }  // namespace system
 }  // namespace mojo
diff --git a/mojo/public/c/system/message_pipe.h b/mojo/public/c/system/message_pipe.h
index ea3dcc3..166e08f 100644
--- a/mojo/public/c/system/message_pipe.h
+++ b/mojo/public/c/system/message_pipe.h
@@ -99,7 +99,8 @@
 //       |message_pipe_handle| is not a valid handle, or some of the
 //       requirements above are not satisfied).
 //   |MOJO_RESULT_PERMISSION_DENIED| if |message_pipe_handle| does not have the
-//       |MOJO_HANDLE_RIGHT_WRITE| right.
+//       |MOJO_HANDLE_RIGHT_WRITE| right or if one of the handles to be sent
+//       does not have the |MOJO_HANDLE_RIGHT_TRANSFER| right.
 //   |MOJO_RESULT_RESOURCE_EXHAUSTED| if some system limit has been reached, or
 //       the number of handles to send is too large (TODO(vtl): reconsider the
 //       latter case).
@@ -113,6 +114,15 @@
 //       being sent in a message), or if some handle to be sent is currently in
 //       use.
 //
+// Note: |MOJO_RESULT_BUSY| is generally "preferred" over
+// |MOJO_RESULT_PERMISSION_DENIED|. E.g., if a handle to be sent both is busy
+// and does not have the transfer right, then the result will be "busy".
+//
+// TODO(vtl): We'll also report |MOJO_RESULT_BUSY| if a (data pipe
+// producer/consumer) handle to be sent is in a two-phase write/read). But
+// should we? (For comparison, there's no such provision in |MojoClose()|.)
+// https://github.com/domokit/mojo/issues/782
+//
 // TODO(vtl): Add a notion of capacity for message pipes, and return
 // |MOJO_RESULT_SHOULD_WAIT| if the message pipe is full.
 MojoResult MojoWriteMessage(MojoHandle message_pipe_handle,  // In.
diff --git a/mojo/public/c/system/tests/core_unittest.cc b/mojo/public/c/system/tests/core_unittest.cc
index aeba1ee..9f02515 100644
--- a/mojo/public/c/system/tests/core_unittest.cc
+++ b/mojo/public/c/system/tests/core_unittest.cc
@@ -737,6 +737,36 @@
   EXPECT_EQ(MOJO_RESULT_OK, MojoClose(h2));
 }
 
+TEST(CoreTest, MessagePipeChecksTransferRight) {
+  MojoHandle h0 = MOJO_HANDLE_INVALID;
+  MojoHandle h1 = MOJO_HANDLE_INVALID;
+  EXPECT_EQ(MOJO_RESULT_OK, MojoCreateMessagePipe(nullptr, &h0, &h1));
+
+  // Create a shared buffer (which is transferrable and duplicatable).
+  MojoHandle h_transferrable = MOJO_HANDLE_INVALID;
+  EXPECT_EQ(MOJO_RESULT_OK,
+            MojoCreateSharedBuffer(nullptr, 100, &h_transferrable));
+
+  // Make a non-transferrable duplicate handle.
+  MojoHandle h_not_transferrable = MOJO_HANDLE_INVALID;
+  EXPECT_EQ(MOJO_RESULT_OK, MojoDuplicateHandleWithReducedRights(
+                                h_transferrable, MOJO_HANDLE_RIGHT_TRANSFER,
+                                &h_not_transferrable));
+
+  // |h_transferrable| can be transferred.
+  EXPECT_EQ(MOJO_RESULT_OK, MojoWriteMessage(h0, nullptr, 0u, &h_transferrable,
+                                             1u, MOJO_WRITE_MESSAGE_FLAG_NONE));
+
+  // |h_not_transferrable| can be transferred.
+  EXPECT_EQ(MOJO_RESULT_PERMISSION_DENIED,
+            MojoWriteMessage(h0, nullptr, 0u, &h_not_transferrable, 1u,
+                             MOJO_WRITE_MESSAGE_FLAG_NONE));
+
+  EXPECT_EQ(MOJO_RESULT_OK, MojoClose(h0));
+  EXPECT_EQ(MOJO_RESULT_OK, MojoClose(h1));
+  EXPECT_EQ(MOJO_RESULT_OK, MojoClose(h_not_transferrable));
+}
+
 // This checks that things actually work in C (not C++).
 TEST(CoreTest, MinimalCTest) {
   const char* failure = MinimalCTest();
diff --git a/mojo/public/platform/native/system_impl_private_unittest.cc b/mojo/public/platform/native/system_impl_private_unittest.cc
index 03fcafd..1c5bc8c 100644
--- a/mojo/public/platform/native/system_impl_private_unittest.cc
+++ b/mojo/public/platform/native/system_impl_private_unittest.cc
@@ -723,5 +723,43 @@
   // 2 SystemImpls are leaked...
 }
 
+TEST(CoreTest, MessagePipeChecksTransferRight) {
+  MojoSystemImpl sys = MojoSystemImplCreateImpl();
+
+  MojoHandle h0 = MOJO_HANDLE_INVALID;
+  MojoHandle h1 = MOJO_HANDLE_INVALID;
+  EXPECT_EQ(MOJO_RESULT_OK,
+            MojoSystemImplCreateMessagePipe(sys, nullptr, &h0, &h1));
+
+  // Create a shared buffer (which is transferrable and duplicatable).
+  MojoHandle h_transferrable = MOJO_HANDLE_INVALID;
+  EXPECT_EQ(MOJO_RESULT_OK, MojoSystemImplCreateSharedBuffer(sys, nullptr, 100,
+                                                             &h_transferrable));
+
+  // Make a non-transferrable duplicate handle.
+  MojoHandle h_not_transferrable = MOJO_HANDLE_INVALID;
+  EXPECT_EQ(MOJO_RESULT_OK,
+            MojoSystemImplDuplicateHandleWithReducedRights(
+                sys, h_transferrable, MOJO_HANDLE_RIGHT_TRANSFER,
+                &h_not_transferrable));
+
+  // |h_transferrable| can be transferred.
+  EXPECT_EQ(MOJO_RESULT_OK,
+            MojoSystemImplWriteMessage(sys, h0, nullptr, 0u, &h_transferrable,
+                                       1u, MOJO_WRITE_MESSAGE_FLAG_NONE));
+
+  // |h_not_transferrable| can be transferred.
+  EXPECT_EQ(
+      MOJO_RESULT_PERMISSION_DENIED,
+      MojoSystemImplWriteMessage(sys, h0, nullptr, 0u, &h_not_transferrable, 1u,
+                                 MOJO_WRITE_MESSAGE_FLAG_NONE));
+
+  EXPECT_EQ(MOJO_RESULT_OK, MojoSystemImplClose(sys, h0));
+  EXPECT_EQ(MOJO_RESULT_OK, MojoSystemImplClose(sys, h1));
+  EXPECT_EQ(MOJO_RESULT_OK, MojoSystemImplClose(sys, h_not_transferrable));
+
+  // 1 SystemImpl is leaked...
+}
+
 }  // namespace
 }  // namespace mojo