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