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