From cff1de9d81338681be3687697038553691b52c88 Mon Sep 17 00:00:00 2001 From: horo Date: Tue, 15 Nov 2016 17:52:54 -0800 Subject: [PATCH] Revert of Remove IPC::BrokerableAttachment. (patchset #1 id:120001 of https://codereview.chromium.org/2494943002/ ) Reason for revert: Caused crash in NewlibPackagedAppTest.SuccessfulLoad browser_tests. See http://crbug.com/665678#c1 BUG=665678 Original issue's description: > Remove IPC::BrokerableAttachment. > > With only ChannelMojo in use, the distinction between brokerable and > non-brokerable attachments no longer makes sense. This CL removes that > distinction by removing BrokerableAttachment and flattening the > hierarchy of attachment types. > > This also trims some POSIX-specific parts of IPC::MessageAttachmentSet. > > BUG=659448 > > Committed: https://crrev.com/f6e03ce56c4d2370b79d0c3dd4ceb89cf5528e56 > Cr-Commit-Position: refs/heads/master@{#432153} TBR=rockot@chromium.org,dcheng@chromium.org,mseaborn@chromium.org,raymes@chromium.org,erikchen@chromium.org,sammc@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=659448 Review-Url: https://codereview.chromium.org/2504063002 Cr-Commit-Position: refs/heads/master@{#432351} --- base/pickle.cc | 1 + components/nacl/loader/nacl_ipc_adapter.cc | 1 + ipc/BUILD.gn | 2 + ipc/brokerable_attachment.cc | 34 +++++ ipc/brokerable_attachment.h | 54 ++++++++ ipc/handle_attachment_win.cc | 15 +- ipc/handle_attachment_win.h | 45 +++++- ipc/handle_win.cc | 10 +- ipc/ipc_channel_mojo.cc | 27 +++- ipc/ipc_channel_mojo_unittest.cc | 11 +- ipc/ipc_channel_nacl.cc | 41 +++--- ipc/ipc_channel_nacl.h | 9 +- ipc/ipc_channel_reader.cc | 2 +- ipc/ipc_channel_reader.h | 5 +- ipc/ipc_channel_reader_unittest.cc | 3 +- ipc/ipc_message.cc | 23 ++- ipc/ipc_message.h | 5 + ipc/ipc_message_attachment.h | 11 +- ipc/ipc_message_attachment_set.cc | 131 +++++++++++++++--- ipc/ipc_message_attachment_set.h | 92 ++++++++++-- ...c_message_attachment_set_posix_unittest.cc | 80 ++++++++--- ipc/ipc_message_utils.cc | 8 +- ipc/ipc_message_utils.h | 1 + ipc/ipc_mojo_handle_attachment.cc | 9 +- ipc/ipc_mojo_handle_attachment.h | 5 + ipc/ipc_mojo_message_helper.cc | 2 +- ipc/ipc_platform_file_attachment_posix.cc | 4 +- ipc/ipc_platform_file_attachment_posix.h | 2 +- ipc/mach_port_attachment_mac.cc | 14 +- ipc/mach_port_attachment_mac.h | 34 ++++- ipc/mach_port_mac.cc | 10 +- ppapi/proxy/nacl_message_scanner.cc | 1 + 32 files changed, 566 insertions(+), 126 deletions(-) create mode 100644 ipc/brokerable_attachment.cc create mode 100644 ipc/brokerable_attachment.h diff --git a/base/pickle.cc b/base/pickle.cc index 02f39b57b7bb81..cfb316c13e5697 100644 --- a/base/pickle.cc +++ b/base/pickle.cc @@ -233,6 +233,7 @@ void PickleSizer::AddBytes(int length) { void PickleSizer::AddAttachment() { // From IPC::Message::WriteAttachment + AddBool(); AddInt(); } diff --git a/components/nacl/loader/nacl_ipc_adapter.cc b/components/nacl/loader/nacl_ipc_adapter.cc index 7418696b7dd81b..33f74f8c1a13aa 100644 --- a/components/nacl/loader/nacl_ipc_adapter.cc +++ b/components/nacl/loader/nacl_ipc_adapter.cc @@ -626,6 +626,7 @@ std::unique_ptr CreateOpenResourceReply( ppapi::proxy::SerializedHandle::WriteHeader(sh.header(), new_msg.get()); new_msg->WriteBool(true); // valid == true + new_msg->WriteBool(false); // brokerable == false // The file descriptor is at index 0. There's only ever one file // descriptor provided for this message type, so this will be correct. new_msg->WriteInt(0); diff --git a/ipc/BUILD.gn b/ipc/BUILD.gn index fa3f81c87d7ef8..cd1f365befe6b2 100644 --- a/ipc/BUILD.gn +++ b/ipc/BUILD.gn @@ -9,6 +9,8 @@ import("//tools/ipc_fuzzer/ipc_fuzzer.gni") component("ipc") { sources = [ + "brokerable_attachment.cc", + "brokerable_attachment.h", "export_template.h", "handle_attachment_win.cc", "handle_attachment_win.h", diff --git a/ipc/brokerable_attachment.cc b/ipc/brokerable_attachment.cc new file mode 100644 index 00000000000000..a0746e122b24f7 --- /dev/null +++ b/ipc/brokerable_attachment.cc @@ -0,0 +1,34 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "ipc/brokerable_attachment.h" + +#include + +#include "build/build_config.h" + +namespace IPC { + +// BrokerableAttachment::BrokerableAttachment ---------------------------------- + +BrokerableAttachment::BrokerableAttachment() = default; + +BrokerableAttachment::~BrokerableAttachment() = default; + +bool BrokerableAttachment::NeedsBrokering() const { + return GetBrokerableType() == PLACEHOLDER; +} + +BrokerableAttachment::Type BrokerableAttachment::GetType() const { + return TYPE_BROKERABLE_ATTACHMENT; +} + +#if defined(OS_POSIX) +base::PlatformFile BrokerableAttachment::TakePlatformFile() { + NOTREACHED(); + return base::PlatformFile(); +} +#endif // OS_POSIX + +} // namespace IPC diff --git a/ipc/brokerable_attachment.h b/ipc/brokerable_attachment.h new file mode 100644 index 00000000000000..a926d5bfe67089 --- /dev/null +++ b/ipc/brokerable_attachment.h @@ -0,0 +1,54 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef IPC_BROKERABLE_ATTACHMENT_H_ +#define IPC_BROKERABLE_ATTACHMENT_H_ + +#include +#include + +#include + +#include "base/macros.h" +#include "build/build_config.h" +#include "ipc/ipc_export.h" +#include "ipc/ipc_message_attachment.h" + +namespace IPC { + +// This subclass of MessageAttachment requires an AttachmentBroker to be +// attached to a Chrome IPC message. +class IPC_EXPORT BrokerableAttachment : public MessageAttachment { + public: + enum BrokerableType { + PLACEHOLDER, + WIN_HANDLE, + MACH_PORT, + }; + + // Whether the attachment still needs information from the broker before it + // can be used. + bool NeedsBrokering() const; + + // Returns TYPE_BROKERABLE_ATTACHMENT + Type GetType() const override; + + virtual BrokerableType GetBrokerableType() const = 0; + +// MessageAttachment override. +#if defined(OS_POSIX) + base::PlatformFile TakePlatformFile() override; +#endif // OS_POSIX + + protected: + BrokerableAttachment(); + ~BrokerableAttachment() override; + + private: + DISALLOW_COPY_AND_ASSIGN(BrokerableAttachment); +}; + +} // namespace IPC + +#endif // IPC_BROKERABLE_ATTACHMENT_H_ diff --git a/ipc/handle_attachment_win.cc b/ipc/handle_attachment_win.cc index 9249a5a069c658..53d589bc8f182e 100644 --- a/ipc/handle_attachment_win.cc +++ b/ipc/handle_attachment_win.cc @@ -28,13 +28,24 @@ HandleAttachmentWin::HandleAttachmentWin(const HANDLE& handle, FromWire from_wire) : handle_(handle), permissions_(HandleWin::INVALID), owns_handle_(true) {} +HandleAttachmentWin::HandleAttachmentWin(const WireFormat& wire_format) + : handle_(LongToHandle(wire_format.handle)), + permissions_(wire_format.permissions), + owns_handle_(true) {} + HandleAttachmentWin::~HandleAttachmentWin() { if (handle_ != INVALID_HANDLE_VALUE && owns_handle_) ::CloseHandle(handle_); } -MessageAttachment::Type HandleAttachmentWin::GetType() const { - return Type::WIN_HANDLE; +HandleAttachmentWin::BrokerableType HandleAttachmentWin::GetBrokerableType() + const { + return WIN_HANDLE; +} + +HandleAttachmentWin::WireFormat HandleAttachmentWin::GetWireFormat( + const base::ProcessId& destination) const { + return WireFormat(HandleToLong(handle_), destination, permissions_); } } // namespace internal diff --git a/ipc/handle_attachment_win.h b/ipc/handle_attachment_win.h index ef3b89652caf8c..cfe56a36bd81c9 100644 --- a/ipc/handle_attachment_win.h +++ b/ipc/handle_attachment_win.h @@ -8,16 +8,48 @@ #include #include "base/process/process_handle.h" +#include "ipc/brokerable_attachment.h" #include "ipc/handle_win.h" #include "ipc/ipc_export.h" -#include "ipc/ipc_message_attachment.h" namespace IPC { namespace internal { // This class represents a Windows HANDLE attached to a Chrome IPC message. -class IPC_EXPORT HandleAttachmentWin : public MessageAttachment { +class IPC_EXPORT HandleAttachmentWin : public BrokerableAttachment { public: + // The wire format for this handle. + struct IPC_EXPORT WireFormat { + // IPC translation requires that classes passed through IPC have a default + // constructor. + WireFormat() + : handle(0), + destination_process(0), + permissions(HandleWin::INVALID) {} + + WireFormat(int32_t handle, + const base::ProcessId& destination_process, + HandleWin::Permissions permissions) + : handle(handle), + destination_process(destination_process), + permissions(permissions) {} + + // The HANDLE that is intended for duplication, or the HANDLE that has been + // duplicated, depending on context. + // The type is int32_t instead of HANDLE because HANDLE gets typedefed to + // void*, whose size varies between 32 and 64-bit processes. Using a + // int32_t means that 64-bit processes will need to perform both up-casting + // and down-casting. This is performed using the appropriate Windows APIs. + // A value of 0 is equivalent to an invalid handle. + int32_t handle; + + // The id of the destination process that the handle is duplicated into. + base::ProcessId destination_process; + + // The permissions to use when duplicating the handle. + HandleWin::Permissions permissions; + }; + // This constructor makes a copy of |handle| and takes ownership of the // result. Should only be called by the sender of a Chrome IPC message. HandleAttachmentWin(const HANDLE& handle, HandleWin::Permissions permissions); @@ -29,7 +61,14 @@ class IPC_EXPORT HandleAttachmentWin : public MessageAttachment { // receiver of a Chrome IPC message. HandleAttachmentWin(const HANDLE& handle, FromWire from_wire); - Type GetType() const override; + // This constructor takes ownership of |wire_format.handle| without making a + // copy. Should only be called by the receiver of a Chrome IPC message. + explicit HandleAttachmentWin(const WireFormat& wire_format); + + BrokerableType GetBrokerableType() const override; + + // Returns the wire format of this attachment. + WireFormat GetWireFormat(const base::ProcessId& destination) const; HANDLE get_handle() const { return handle_; } diff --git a/ipc/handle_win.cc b/ipc/handle_win.cc index c8511e2017387a..60fa54c7cd1643 100644 --- a/ipc/handle_win.cc +++ b/ipc/handle_win.cc @@ -38,10 +38,16 @@ bool ParamTraits::Read(const base::Pickle* m, return false; MessageAttachment* attachment = static_cast(base_attachment.get()); - if (attachment->GetType() != MessageAttachment::Type::WIN_HANDLE) + if (attachment->GetType() != MessageAttachment::TYPE_BROKERABLE_ATTACHMENT) return false; + BrokerableAttachment* brokerable_attachment = + static_cast(attachment); + if (brokerable_attachment->GetBrokerableType() != + BrokerableAttachment::WIN_HANDLE) { + return false; + } IPC::internal::HandleAttachmentWin* handle_attachment = - static_cast(attachment); + static_cast(brokerable_attachment); r->set_handle(handle_attachment->get_handle()); handle_attachment->reset_handle_ownership(); return true; diff --git a/ipc/ipc_channel_mojo.cc b/ipc/ipc_channel_mojo.cc index 4189b9c06f1a58..13af1e3dff818a 100644 --- a/ipc/ipc_channel_mojo.cc +++ b/ipc/ipc_channel_mojo.cc @@ -124,14 +124,14 @@ base::ScopedFD TakeOrDupFile(internal::PlatformFileAttachment* attachment) { MojoResult WrapAttachmentImpl(MessageAttachment* attachment, mojom::SerializedHandlePtr* serialized) { - if (attachment->GetType() == MessageAttachment::Type::MOJO_HANDLE) { + if (attachment->GetType() == MessageAttachment::TYPE_MOJO_HANDLE) { *serialized = CreateSerializedHandle( static_cast(*attachment).TakeHandle(), mojom::SerializedHandle::Type::MOJO_HANDLE); return MOJO_RESULT_OK; } #if defined(OS_POSIX) - if (attachment->GetType() == MessageAttachment::Type::PLATFORM_FILE) { + if (attachment->GetType() == MessageAttachment::TYPE_PLATFORM_FILE) { // We dup() the handles in IPC::Message to transmit. // IPC::MessageAttachmentSet has intricate lifecycle semantics // of FDs, so just to dup()-and-own them is the safest option. @@ -148,7 +148,10 @@ MojoResult WrapAttachmentImpl(MessageAttachment* attachment, } #endif #if defined(OS_MACOSX) - DCHECK_EQ(attachment->GetType(), MessageAttachment::Type::MACH_PORT); + DCHECK_EQ(attachment->GetType(), + MessageAttachment::TYPE_BROKERABLE_ATTACHMENT); + DCHECK_EQ(static_cast(*attachment).GetBrokerableType(), + BrokerableAttachment::MACH_PORT); internal::MachPortAttachmentMac& mach_port_attachment = static_cast(*attachment); MojoResult result = WrapMachPort(mach_port_attachment.get_mach_port(), @@ -156,7 +159,10 @@ MojoResult WrapAttachmentImpl(MessageAttachment* attachment, mach_port_attachment.reset_mach_port_ownership(); return result; #elif defined(OS_WIN) - DCHECK_EQ(attachment->GetType(), MessageAttachment::Type::WIN_HANDLE); + DCHECK_EQ(attachment->GetType(), + MessageAttachment::TYPE_BROKERABLE_ATTACHMENT); + DCHECK_EQ(static_cast(*attachment).GetBrokerableType(), + BrokerableAttachment::WIN_HANDLE); internal::HandleAttachmentWin& handle_attachment = static_cast(*attachment); MojoResult result = WrapPlatformHandle( @@ -403,9 +409,18 @@ MojoResult ChannelMojo::ReadFromMessageAttachmentSet( std::vector output_handles; MessageAttachmentSet* set = message->attachment_set(); - for (unsigned i = 0; result == MOJO_RESULT_OK && i < set->size(); ++i) { - result = WrapAttachment(set->GetAttachmentAt(i).get(), &output_handles); + for (unsigned i = 0; + result == MOJO_RESULT_OK && i < set->num_non_brokerable_attachments(); + ++i) { + result = WrapAttachment(set->GetNonBrokerableAttachmentAt(i).get(), + &output_handles); } + for (unsigned i = 0; + result == MOJO_RESULT_OK && i < set->num_brokerable_attachments(); ++i) { + result = WrapAttachment(set->GetBrokerableAttachmentAt(i).get(), + &output_handles); + } + set->CommitAllDescriptors(); if (!output_handles.empty()) diff --git a/ipc/ipc_channel_mojo_unittest.cc b/ipc/ipc_channel_mojo_unittest.cc index 5c23ea3823c6b4..229716edc0dbae 100644 --- a/ipc/ipc_channel_mojo_unittest.cc +++ b/ipc/ipc_channel_mojo_unittest.cc @@ -289,12 +289,11 @@ class HandleSendingHelper { base::ScopedFD fd; scoped_refptr attachment; EXPECT_TRUE(message.ReadAttachment(iter, &attachment)); - EXPECT_EQ( - IPC::MessageAttachment::Type::PLATFORM_FILE, - static_cast(attachment.get())->GetType()); - base::File file( - static_cast(attachment.get()) - ->TakePlatformFile()); + EXPECT_EQ(IPC::MessageAttachment::TYPE_PLATFORM_FILE, + static_cast(attachment.get()) + ->GetType()); + base::File file(static_cast(attachment.get()) + ->TakePlatformFile()); std::string content(GetSendingFileContent().size(), ' '); file.Read(0, &content[0], content.size()); EXPECT_EQ(content, GetSendingFileContent()); diff --git a/ipc/ipc_channel_nacl.cc b/ipc/ipc_channel_nacl.cc index 1fa8e691459097..5243bc88d725cb 100644 --- a/ipc/ipc_channel_nacl.cc +++ b/ipc/ipc_channel_nacl.cc @@ -23,7 +23,6 @@ #include "ipc/ipc_listener.h" #include "ipc/ipc_logging.h" #include "ipc/ipc_message_attachment_set.h" -#include "ipc/ipc_platform_file_attachment_posix.h" #include "native_client/src/public/imc_syscalls.h" #include "native_client/src/public/imc_types.h" @@ -224,11 +223,8 @@ void ChannelNacl::DidRecvMsg(std::unique_ptr contents) { data->swap(contents->data); read_queue_.push_back(data); - input_attachments_.reserve(contents->fds.size()); - for (int fd : contents->fds) { - input_attachments_.push_back( - new internal::PlatformFileAttachment(base::ScopedFD(fd))); - } + input_fds_.insert(input_fds_.end(), + contents->fds.begin(), contents->fds.end()); contents->fds.clear(); // In POSIX, we would be told when there are bytes to read by implementing @@ -276,22 +272,16 @@ bool ChannelNacl::ProcessOutgoingMessages() { linked_ptr msg = output_queue_.front(); output_queue_.pop_front(); - const size_t num_fds = msg->attachment_set()->size(); + int fds[MessageAttachmentSet::kMaxDescriptorsPerMessage]; + const size_t num_fds = + msg->attachment_set()->num_non_brokerable_attachments(); DCHECK(num_fds <= MessageAttachmentSet::kMaxDescriptorsPerMessage); - std::vector fds; - fds.reserve(num_fds); - for (size_t i = 0; i < num_fds; i++) { - scoped_refptr attachment = - msg->attachment_set()->GetAttachmentAt(i); - DCHECK_EQ(MessageAttachment::Type::PLATFORM_FILE, attachment->GetType()); - fds.push_back(static_cast(*attachment) - .TakePlatformFile()); - } + msg->attachment_set()->PeekDescriptors(fds); NaClAbiNaClImcMsgIoVec iov = { const_cast(msg->data()), msg->size() }; - NaClAbiNaClImcMsgHdr msgh = {&iov, 1, fds.data(), num_fds}; + NaClAbiNaClImcMsgHdr msgh = { &iov, 1, fds, num_fds }; ssize_t bytes_written = imc_sendmsg(pipe_, &msgh, 0); DCHECK(bytes_written); // The trusted side shouldn't return 0. @@ -355,22 +345,23 @@ bool ChannelNacl::ShouldDispatchInputMessage(Message* msg) { return true; } -bool ChannelNacl::GetAttachments(Message* msg) { +bool ChannelNacl::GetNonBrokeredAttachments(Message* msg) { uint16_t header_fds = msg->header()->num_fds; - CHECK(header_fds == input_attachments_.size()); + CHECK(header_fds == input_fds_.size()); if (header_fds == 0) return true; // Nothing to do. - for (auto& attachment : input_attachments_) { - msg->attachment_set()->AddAttachment(std::move(attachment)); - } - input_attachments_.clear(); + // The shenaniganery below with &foo.front() requires input_fds_ to have + // contiguous underlying storage (such as a simple array or a std::vector). + // This is why the header warns not to make input_fds_ a deque<>. + msg->attachment_set()->AddDescriptorsToOwn(&input_fds_.front(), header_fds); + input_fds_.clear(); return true; } bool ChannelNacl::DidEmptyInputBuffers() { - // When the input data buffer is empty, the attachments should be too. - return input_attachments_.empty(); + // When the input data buffer is empty, the fds should be too. + return input_fds_.empty(); } void ChannelNacl::HandleInternalMessage(const Message& msg) { diff --git a/ipc/ipc_channel_nacl.h b/ipc/ipc_channel_nacl.h index 34afe6edd9e055..2e4d42fbe046e1 100644 --- a/ipc/ipc_channel_nacl.h +++ b/ipc/ipc_channel_nacl.h @@ -62,7 +62,7 @@ class ChannelNacl : public Channel, int buffer_len, int* bytes_read) override; bool ShouldDispatchInputMessage(Message* msg) override; - bool GetAttachments(Message* msg) override; + bool GetNonBrokeredAttachments(Message* msg) override; bool DidEmptyInputBuffers() override; void HandleInternalMessage(const Message& msg) override; @@ -96,8 +96,11 @@ class ChannelNacl : public Channel, // 2 above in NaCl eventually. // When ReadData is called, it pulls the bytes out of this queue in order. std::deque > > read_queue_; - // Queue of file descriptor attachments extracted from imc_recvmsg messages. - std::vector> input_attachments_; + // Queue of file descriptors extracted from imc_recvmsg messages. + // NOTE: The implementation assumes underlying storage here is contiguous, so + // don't change to something like std::deque<> without changing the + // implementation! + std::vector input_fds_; // This queue is used when a message is sent prior to Connect having been // called. Normally after we're connected, the queue is empty. diff --git a/ipc/ipc_channel_reader.cc b/ipc/ipc_channel_reader.cc index fa26734c314c8a..69079797d4ee51 100644 --- a/ipc/ipc_channel_reader.cc +++ b/ipc/ipc_channel_reader.cc @@ -192,7 +192,7 @@ bool ChannelReader::HandleTranslatedMessage(Message* translated_message) { } bool ChannelReader::HandleExternalMessage(Message* external_message) { - if (!GetAttachments(external_message)) + if (!GetNonBrokeredAttachments(external_message)) return false; DispatchMessage(external_message); diff --git a/ipc/ipc_channel_reader.h b/ipc/ipc_channel_reader.h index 04a95dd260b755..73cea04cca071f 100644 --- a/ipc/ipc_channel_reader.h +++ b/ipc/ipc_channel_reader.h @@ -12,6 +12,7 @@ #include "base/gtest_prod_util.h" #include "base/macros.h" #include "base/memory/scoped_vector.h" +#include "ipc/brokerable_attachment.h" #include "ipc/ipc_channel.h" #include "ipc/ipc_export.h" @@ -100,9 +101,9 @@ class IPC_EXPORT ChannelReader { virtual bool ShouldDispatchInputMessage(Message* msg) = 0; // Overridden by subclasses to get attachments that are sent alongside the IPC - // channel. + // channel (as opposed to through a broker). // Returns true on success. False means a fatal channel error. - virtual bool GetAttachments(Message* msg) = 0; + virtual bool GetNonBrokeredAttachments(Message* msg) = 0; // Performs post-dispatch checks. Called when all input buffers are empty, // though there could be more data ready to be read from the OS. diff --git a/ipc/ipc_channel_reader_unittest.cc b/ipc/ipc_channel_reader_unittest.cc index a3d80dc922e763..16c6649c3a1f16 100644 --- a/ipc/ipc_channel_reader_unittest.cc +++ b/ipc/ipc_channel_reader_unittest.cc @@ -12,6 +12,7 @@ #include #include "base/run_loop.h" +#include "ipc/brokerable_attachment.h" #include "ipc/ipc_channel_reader.h" #include "testing/gtest/include/gtest/gtest.h" @@ -38,7 +39,7 @@ class MockChannelReader : public ChannelReader { bool ShouldDispatchInputMessage(Message* msg) override { return true; } - bool GetAttachments(Message* msg) override { return true; } + bool GetNonBrokeredAttachments(Message* msg) override { return true; } bool DidEmptyInputBuffers() override { return true; } diff --git a/ipc/ipc_message.cc b/ipc/ipc_message.cc index f5e9ac7a8b807c..008401f14f358c 100644 --- a/ipc/ipc_message.cc +++ b/ipc/ipc_message.cc @@ -166,15 +166,19 @@ void Message::FindNext(const char* range_start, bool Message::WriteAttachment( scoped_refptr attachment) { + bool brokerable; size_t index; bool success = attachment_set()->AddAttachment( make_scoped_refptr(static_cast(attachment.get())), - &index); + &index, &brokerable); DCHECK(success); // NOTE: If you add more data to the pickle, make sure to update // PickleSizer::AddAttachment. + // Write the type of descriptor. + WriteBool(brokerable); + // Write the index of the descriptor so that we don't have to // keep the current descriptor as extra decoding state when deserialising. WriteInt(static_cast(index)); @@ -185,6 +189,10 @@ bool Message::WriteAttachment( bool Message::ReadAttachment( base::PickleIterator* iter, scoped_refptr* attachment) const { + bool brokerable; + if (!iter->ReadBool(&brokerable)) + return false; + int index; if (!iter->ReadInt(&index)) return false; @@ -193,7 +201,9 @@ bool Message::ReadAttachment( if (!attachment_set) return false; - *attachment = attachment_set->GetAttachmentAt(index); + *attachment = brokerable + ? attachment_set->GetBrokerableAttachmentAt(index) + : attachment_set->GetNonBrokerableAttachmentAt(index); return nullptr != attachment->get(); } @@ -202,4 +212,13 @@ bool Message::HasAttachments() const { return attachment_set_.get() && !attachment_set_->empty(); } +bool Message::HasMojoHandles() const { + return attachment_set_.get() && attachment_set_->num_mojo_handles() > 0; +} + +bool Message::HasBrokerableAttachments() const { + return attachment_set_.get() && + attachment_set_->num_brokerable_attachments() > 0; +} + } // namespace IPC diff --git a/ipc/ipc_message.h b/ipc/ipc_message.h index 41f57afc31ce13..6bf18dec425454 100644 --- a/ipc/ipc_message.h +++ b/ipc/ipc_message.h @@ -15,6 +15,7 @@ #include "base/pickle.h" #include "base/trace_event/trace_event.h" #include "build/build_config.h" +#include "ipc/brokerable_attachment.h" #include "ipc/ipc_export.h" #if !defined(NDEBUG) @@ -207,6 +208,10 @@ class IPC_EXPORT Message : public base::Pickle { scoped_refptr* attachment) const override; // Returns true if there are any attachment in this message. bool HasAttachments() const override; + // Returns true if there are any MojoHandleAttachments in this message. + bool HasMojoHandles() const; + // Whether the message has any brokerable attachments. + bool HasBrokerableAttachments() const; #ifdef IPC_MESSAGE_LOG_ENABLED // Adds the outgoing time from Time::Now() at the end of the message and sets diff --git a/ipc/ipc_message_attachment.h b/ipc/ipc_message_attachment.h index 9ff1de8c32f53b..7f7137d87b20f7 100644 --- a/ipc/ipc_message_attachment.h +++ b/ipc/ipc_message_attachment.h @@ -10,7 +10,6 @@ #include "base/memory/ref_counted.h" #include "base/pickle.h" #include "build/build_config.h" -#include "ipc/ipc.mojom.h" #include "ipc/ipc_export.h" namespace IPC { @@ -19,10 +18,18 @@ namespace IPC { // or a mojo |MessagePipe|. |GetType()| returns the type of the subclass. class IPC_EXPORT MessageAttachment : public base::Pickle::Attachment { public: - using Type = mojom::SerializedHandle::Type; + enum Type { + TYPE_PLATFORM_FILE, // The instance is |PlatformFileAttachment|. + TYPE_MOJO_HANDLE, // The instance is |MojoHandleAttachment|. + TYPE_BROKERABLE_ATTACHMENT, // The instance is |BrokerableAttachment|. + }; virtual Type GetType() const = 0; +#if defined(OS_POSIX) + virtual base::PlatformFile TakePlatformFile() = 0; +#endif // OS_POSIX + protected: friend class base::RefCountedThreadSafe; MessageAttachment(); diff --git a/ipc/ipc_message_attachment_set.cc b/ipc/ipc_message_attachment_set.cc index b9a990da8bb6af..068e9d29c8f326 100644 --- a/ipc/ipc_message_attachment_set.cc +++ b/ipc/ipc_message_attachment_set.cc @@ -11,8 +11,16 @@ #include "base/logging.h" #include "base/posix/eintr_wrapper.h" #include "build/build_config.h" +#include "ipc/brokerable_attachment.h" #include "ipc/ipc_message_attachment.h" +#if defined(OS_POSIX) +#include +#include +#include +#include "ipc/ipc_platform_file_attachment_posix.h" +#endif // OS_POSIX + namespace IPC { namespace { @@ -35,7 +43,7 @@ MessageAttachmentSet::MessageAttachmentSet() } MessageAttachmentSet::~MessageAttachmentSet() { - if (consumed_descriptor_highwater_ == size()) + if (consumed_descriptor_highwater_ == num_non_brokerable_attachments()) return; // We close all the owning descriptors. If this message should have @@ -46,24 +54,39 @@ MessageAttachmentSet::~MessageAttachmentSet() { // (which could a DOS against the browser by a rogue renderer) then all // the descriptors have their close flag set and we free all the extra // kernel resources. - LOG(WARNING) << "MessageAttachmentSet destroyed with unconsumed attachments: " - << consumed_descriptor_highwater_ << "/" << size(); + LOG(WARNING) << "MessageAttachmentSet destroyed with unconsumed descriptors: " + << consumed_descriptor_highwater_ << "/" << num_descriptors(); } unsigned MessageAttachmentSet::num_descriptors() const { return count_attachments_of_type(attachments_, - MessageAttachment::Type::PLATFORM_FILE); + MessageAttachment::TYPE_PLATFORM_FILE); } -unsigned MessageAttachmentSet::size() const { +unsigned MessageAttachmentSet::num_mojo_handles() const { + return count_attachments_of_type(attachments_, + MessageAttachment::TYPE_MOJO_HANDLE); +} + +unsigned MessageAttachmentSet::num_brokerable_attachments() const { + return static_cast(brokerable_attachments_.size()); +} + +unsigned MessageAttachmentSet::num_non_brokerable_attachments() const { return static_cast(attachments_.size()); } +unsigned MessageAttachmentSet::size() const { + return static_cast(attachments_.size() + + brokerable_attachments_.size()); +} + bool MessageAttachmentSet::AddAttachment( scoped_refptr attachment, - size_t* index) { + size_t* index, + bool* brokerable) { #if defined(OS_POSIX) - if (attachment->GetType() == MessageAttachment::Type::PLATFORM_FILE && + if (attachment->GetType() == MessageAttachment::TYPE_PLATFORM_FILE && num_descriptors() == kMaxDescriptorsPerMessage) { DLOG(WARNING) << "Cannot add file descriptor. MessageAttachmentSet full."; return false; @@ -71,12 +94,19 @@ bool MessageAttachmentSet::AddAttachment( #endif switch (attachment->GetType()) { - case MessageAttachment::Type::PLATFORM_FILE: - case MessageAttachment::Type::MOJO_HANDLE: - case MessageAttachment::Type::WIN_HANDLE: - case MessageAttachment::Type::MACH_PORT: + case MessageAttachment::TYPE_PLATFORM_FILE: + case MessageAttachment::TYPE_MOJO_HANDLE: attachments_.push_back(attachment); *index = attachments_.size() - 1; + *brokerable = false; + return true; + case MessageAttachment::TYPE_BROKERABLE_ATTACHMENT: + BrokerableAttachment* brokerable_attachment = + static_cast(attachment.get()); + scoped_refptr a(brokerable_attachment); + brokerable_attachments_.push_back(a); + *index = brokerable_attachments_.size() - 1; + *brokerable = true; return true; } return false; @@ -84,14 +114,16 @@ bool MessageAttachmentSet::AddAttachment( bool MessageAttachmentSet::AddAttachment( scoped_refptr attachment) { + bool brokerable; size_t index; - return AddAttachment(attachment, &index); + return AddAttachment(attachment, &index, &brokerable); } -scoped_refptr MessageAttachmentSet::GetAttachmentAt( - unsigned index) { - if (index >= size()) { - DLOG(WARNING) << "Accessing out of bound index:" << index << "/" << size(); +scoped_refptr +MessageAttachmentSet::GetNonBrokerableAttachmentAt(unsigned index) { + if (index >= num_non_brokerable_attachments()) { + DLOG(WARNING) << "Accessing out of bound index:" << index << "/" + << num_non_brokerable_attachments(); return scoped_refptr(); } @@ -116,7 +148,8 @@ scoped_refptr MessageAttachmentSet::GetAttachmentAt( // end of the array and index 0 is requested, we reset the highwater value. // TODO(morrita): This is absurd. This "wringle" disallow to introduce clearer // ownership model. Only client is NaclIPCAdapter. See crbug.com/415294 - if (index == 0 && consumed_descriptor_highwater_ == size()) { + if (index == 0 && + consumed_descriptor_highwater_ == num_non_brokerable_attachments()) { consumed_descriptor_highwater_ = 0; } @@ -128,9 +161,73 @@ scoped_refptr MessageAttachmentSet::GetAttachmentAt( return attachments_[index]; } +scoped_refptr +MessageAttachmentSet::GetBrokerableAttachmentAt(unsigned index) { + if (index >= num_brokerable_attachments()) { + DLOG(WARNING) << "Accessing out of bound index:" << index << "/" + << num_brokerable_attachments(); + return scoped_refptr(); + } + + scoped_refptr brokerable_attachment( + brokerable_attachments_[index]); + return scoped_refptr(brokerable_attachment.get()); +} + void MessageAttachmentSet::CommitAllDescriptors() { attachments_.clear(); consumed_descriptor_highwater_ = 0; } +std::vector> +MessageAttachmentSet::GetBrokerableAttachments() const { + return brokerable_attachments_; +} + +#if defined(OS_POSIX) + +void MessageAttachmentSet::PeekDescriptors(base::PlatformFile* buffer) const { + for (size_t i = 0; i != attachments_.size(); ++i) + buffer[i] = internal::GetPlatformFile(attachments_[i]); +} + +bool MessageAttachmentSet::ContainsDirectoryDescriptor() const { + struct stat st; + + for (auto i = attachments_.begin(); i != attachments_.end(); ++i) { + if (fstat(internal::GetPlatformFile(*i), &st) == 0 && S_ISDIR(st.st_mode)) + return true; + } + + return false; +} + +void MessageAttachmentSet::ReleaseFDsToClose( + std::vector* fds) { + for (size_t i = 0; i < attachments_.size(); ++i) { + internal::PlatformFileAttachment* file = + static_cast(attachments_[i].get()); + if (file->Owns()) + fds->push_back(file->TakePlatformFile()); + } + + CommitAllDescriptors(); +} + +void MessageAttachmentSet::AddDescriptorsToOwn(const base::PlatformFile* buffer, + unsigned count) { + DCHECK(count <= kMaxDescriptorsPerMessage); + DCHECK_EQ(num_descriptors(), 0u); + DCHECK_EQ(consumed_descriptor_highwater_, 0u); + + attachments_.reserve(count); + for (unsigned i = 0; i < count; ++i) + AddAttachment( + new internal::PlatformFileAttachment(base::ScopedFD(buffer[i]))); +} + +#endif // OS_POSIX + } // namespace IPC + + diff --git a/ipc/ipc_message_attachment_set.h b/ipc/ipc_message_attachment_set.h index de37211435867e..09ff2e3f651518 100644 --- a/ipc/ipc_message_attachment_set.h +++ b/ipc/ipc_message_attachment_set.h @@ -14,17 +14,31 @@ #include "build/build_config.h" #include "ipc/ipc_export.h" +#if defined(OS_POSIX) +#include "base/files/file.h" +#endif + namespace IPC { +class BrokerableAttachment; class MessageAttachment; // ----------------------------------------------------------------------------- // A MessageAttachmentSet is an ordered set of MessageAttachment objects -// associated with an IPC message. All attachments are wrapped in a mojo handle -// if necessary and sent over the mojo message pipe. +// associated with an IPC message. There are three types of MessageAttachments: +// 1) TYPE_PLATFORM_FILE is transmitted over the Channel's underlying +// UNIX domain socket +// 2) TYPE_MOJO_HANDLE is transmitted over the Mojo MessagePipe. +// 3) TYPE_BROKERABLE_ATTACHMENT is transmitted by the Attachment Broker. +// Any given IPC Message can have attachments of type (1) or (2), but not both. +// These are stored in |attachments_|. Attachments of type (3) are stored in +// |brokerable_attachments_|. +// +// To produce a deterministic ordering, all attachments in |attachments_| are +// considered to come before those in |brokerable_attachments_|. These +// attachments are transmitted across different communication channels, and +// multiplexed by the receiver, so ordering between them cannot be guaranteed. // -// For ChannelNacl under SFI NaCl, only Type::PLATFORM_FILE is supported. In -// that case, the FD is sent over socket. // ----------------------------------------------------------------------------- class IPC_EXPORT MessageAttachmentSet : public base::RefCountedThreadSafe { @@ -33,31 +47,52 @@ class IPC_EXPORT MessageAttachmentSet // Return the number of attachments unsigned size() const; + // Return the number of file descriptors + unsigned num_descriptors() const; + // Return the number of mojo handles in the attachment set + unsigned num_mojo_handles() const; + // Return the number of brokerable attachments in the attachment set. + unsigned num_brokerable_attachments() const; + // Return the number of non-brokerable attachments in the attachment set. + unsigned num_non_brokerable_attachments() const; // Return true if no unconsumed descriptors remain - bool empty() const { return attachments_.empty(); } + bool empty() const { return 0 == size(); } // Returns whether the attachment was successfully added. // |index| is an output variable. On success, it contains the index of the // newly added attachment. + // |brokerable| is an output variable. On success, it describes which vector + // the attachment was added to. bool AddAttachment(scoped_refptr attachment, - size_t* index); + size_t* index, + bool* brokerable); // Similar to the above method, but without output variables. bool AddAttachment(scoped_refptr attachment); - // Take the nth from the beginning of the vector, Code using this /must/ - // access the attachments in order, and must do it at most once. + // Take the nth non-brokerable attachment from the beginning of the vector, + // Code using this /must/ access the attachments in order, and must do it at + // most once. // // This interface is designed for the deserialising code as it doesn't // support close flags. // returns: an attachment, or nullptr on error - scoped_refptr GetAttachmentAt(unsigned index); + scoped_refptr GetNonBrokerableAttachmentAt(unsigned index); - // Marks all the descriptors as consumed and closes those which are - // auto-close. + // Similar to GetNonBrokerableAttachmentAt, but there are no ordering + // requirements. + scoped_refptr GetBrokerableAttachmentAt(unsigned index); + + // This must be called after transmitting the descriptors returned by + // PeekDescriptors. It marks all the non-brokerable descriptors as consumed + // and closes those which are auto-close. void CommitAllDescriptors(); + // Returns a vector of all brokerable attachments. + std::vector> + GetBrokerableAttachments() const; + #if defined(OS_POSIX) // This is the maximum number of descriptors per message. We need to know this // because the control message kernel interface has to be given a buffer which @@ -68,6 +103,32 @@ class IPC_EXPORT MessageAttachmentSet // In debugging mode, it's a fatal error to try and add more than this number // of descriptors to a MessageAttachmentSet. static const size_t kMaxDescriptorsPerMessage = 7; + + // --------------------------------------------------------------------------- + // Interfaces for transmission... + + // Fill an array with file descriptors without 'consuming' them. + // CommitAllDescriptors must be called after these descriptors have been + // transmitted. + // buffer: (output) a buffer of, at least, size() integers. + void PeekDescriptors(base::PlatformFile* buffer) const; + // Returns true if any contained file descriptors appear to be handles to a + // directory. + bool ContainsDirectoryDescriptor() const; + // Fetch all filedescriptors with the "auto close" property. Used instead of + // CommitAllDescriptors() when closing must be handled manually. + void ReleaseFDsToClose(std::vector* fds); + + // --------------------------------------------------------------------------- + + // --------------------------------------------------------------------------- + // Interfaces for receiving... + + // Set the contents of the set from the given buffer. This set must be empty + // before calling. The auto-close flag is set on all the descriptors so that + // unconsumed descriptors are closed on destruction. + void AddDescriptorsToOwn(const base::PlatformFile* buffer, unsigned count); + #endif // OS_POSIX // --------------------------------------------------------------------------- @@ -77,16 +138,17 @@ class IPC_EXPORT MessageAttachmentSet ~MessageAttachmentSet(); - // Return the number of file descriptors - unsigned num_descriptors() const; - + // All elements either have type TYPE_PLATFORM_FILE or TYPE_MOJO_HANDLE. std::vector> attachments_; + // All elements have type TYPE_BROKERABLE_ATTACHMENT. + std::vector> brokerable_attachments_; + // This contains the index of the next descriptor which should be consumed. // It's used in a couple of ways. Firstly, at destruction we can check that // all the descriptors have been read (with GetNthDescriptor). Secondly, we // can check that they are read in order. - unsigned consumed_descriptor_highwater_; + mutable unsigned consumed_descriptor_highwater_; DISALLOW_COPY_AND_ASSIGN(MessageAttachmentSet); }; diff --git a/ipc/ipc_message_attachment_set_posix_unittest.cc b/ipc/ipc_message_attachment_set_posix_unittest.cc index 0bd143df4dee9e..5ebed84070ce37 100644 --- a/ipc/ipc_message_attachment_set_posix_unittest.cc +++ b/ipc/ipc_message_attachment_set_posix_unittest.cc @@ -34,12 +34,6 @@ bool VerifyClosed(int fd) { return true; } -int GetFdAt(MessageAttachmentSet* set, int id) { - return static_cast( - *set->GetAttachmentAt(id)) - .TakePlatformFile(); -} - // The MessageAttachmentSet will try and close some of the descriptor numbers // which we given it. This is the base descriptor value. It's great enough such // that no real descriptor will accidently be closed. @@ -88,6 +82,44 @@ TEST(MessageAttachmentSet, MaxSize) { set->CommitAllDescriptors(); } +#if defined(OS_ANDROID) +#define MAYBE_SetDescriptors DISABLED_SetDescriptors +#else +#define MAYBE_SetDescriptors SetDescriptors +#endif +TEST(MessageAttachmentSet, MAYBE_SetDescriptors) { + scoped_refptr set(new MessageAttachmentSet); + + ASSERT_TRUE(set->empty()); + set->AddDescriptorsToOwn(NULL, 0); + ASSERT_TRUE(set->empty()); + + const int fd = GetSafeFd(); + static const int fds[] = {fd}; + set->AddDescriptorsToOwn(fds, 1); + ASSERT_TRUE(!set->empty()); + ASSERT_EQ(set->size(), 1u); + + set->CommitAllDescriptors(); + + ASSERT_TRUE(VerifyClosed(fd)); +} + +TEST(MessageAttachmentSet, PeekDescriptors) { + scoped_refptr set(new MessageAttachmentSet); + + set->PeekDescriptors(NULL); + ASSERT_TRUE( + set->AddAttachment(new internal::PlatformFileAttachment(kFDBase))); + + int fds[1]; + fds[0] = 0; + set->PeekDescriptors(fds); + ASSERT_EQ(fds[0], kFDBase); + set->CommitAllDescriptors(); + ASSERT_TRUE(set->empty()); +} + TEST(MessageAttachmentSet, WalkInOrder) { scoped_refptr set(new MessageAttachmentSet); @@ -100,9 +132,11 @@ TEST(MessageAttachmentSet, WalkInOrder) { ASSERT_TRUE( set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 2))); - ASSERT_EQ(GetFdAt(set.get(), 0), kFDBase); - ASSERT_EQ(GetFdAt(set.get(), 1), kFDBase + 1); - ASSERT_EQ(GetFdAt(set.get(), 2), kFDBase + 2); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(0)->TakePlatformFile(), kFDBase); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(1)->TakePlatformFile(), + kFDBase + 1); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(2)->TakePlatformFile(), + kFDBase + 2); set->CommitAllDescriptors(); } @@ -119,8 +153,8 @@ TEST(MessageAttachmentSet, WalkWrongOrder) { ASSERT_TRUE( set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 2))); - ASSERT_EQ(GetFdAt(set.get(), 0), kFDBase); - ASSERT_FALSE(set->GetAttachmentAt(2)); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(0)->TakePlatformFile(), kFDBase); + ASSERT_FALSE(set->GetNonBrokerableAttachmentAt(2)); set->CommitAllDescriptors(); } @@ -137,15 +171,21 @@ TEST(MessageAttachmentSet, WalkCycle) { ASSERT_TRUE( set->AddAttachment(new internal::PlatformFileAttachment(kFDBase + 2))); - ASSERT_EQ(GetFdAt(set.get(), 0), kFDBase); - ASSERT_EQ(GetFdAt(set.get(), 1), kFDBase + 1); - ASSERT_EQ(GetFdAt(set.get(), 2), kFDBase + 2); - ASSERT_EQ(GetFdAt(set.get(), 0), kFDBase); - ASSERT_EQ(GetFdAt(set.get(), 1), kFDBase + 1); - ASSERT_EQ(GetFdAt(set.get(), 2), kFDBase + 2); - ASSERT_EQ(GetFdAt(set.get(), 0), kFDBase); - ASSERT_EQ(GetFdAt(set.get(), 1), kFDBase + 1); - ASSERT_EQ(GetFdAt(set.get(), 2), kFDBase + 2); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(0)->TakePlatformFile(), kFDBase); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(1)->TakePlatformFile(), + kFDBase + 1); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(2)->TakePlatformFile(), + kFDBase + 2); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(0)->TakePlatformFile(), kFDBase); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(1)->TakePlatformFile(), + kFDBase + 1); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(2)->TakePlatformFile(), + kFDBase + 2); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(0)->TakePlatformFile(), kFDBase); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(1)->TakePlatformFile(), + kFDBase + 1); + ASSERT_EQ(set->GetNonBrokerableAttachmentAt(2)->TakePlatformFile(), + kFDBase + 2); set->CommitAllDescriptors(); } diff --git a/ipc/ipc_message_utils.cc b/ipc/ipc_message_utils.cc index be83e5a236bb23..f814727ee2b81f 100644 --- a/ipc/ipc_message_utils.cc +++ b/ipc/ipc_message_utils.cc @@ -658,14 +658,8 @@ bool ParamTraits::Read(const base::Pickle* m, if (!m->ReadAttachment(iter, &attachment)) return false; - if (static_cast(attachment.get())->GetType() != - MessageAttachment::Type::PLATFORM_FILE) { - return false; - } - *r = base::FileDescriptor( - static_cast(attachment.get()) - ->TakePlatformFile(), + static_cast(attachment.get())->TakePlatformFile(), true); return true; } diff --git a/ipc/ipc_message_utils.h b/ipc/ipc_message_utils.h index 58d6aa26da34fb..28cc7225be5750 100644 --- a/ipc/ipc_message_utils.h +++ b/ipc/ipc_message_utils.h @@ -27,6 +27,7 @@ #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "build/build_config.h" +#include "ipc/brokerable_attachment.h" #include "ipc/ipc_message_start.h" #include "ipc/ipc_param_traits.h" #include "ipc/ipc_sync_message.h" diff --git a/ipc/ipc_mojo_handle_attachment.cc b/ipc/ipc_mojo_handle_attachment.cc index e3421c3e88bc31..819a12b890ad1c 100644 --- a/ipc/ipc_mojo_handle_attachment.cc +++ b/ipc/ipc_mojo_handle_attachment.cc @@ -18,9 +18,16 @@ MojoHandleAttachment::~MojoHandleAttachment() { } MessageAttachment::Type MojoHandleAttachment::GetType() const { - return Type::MOJO_HANDLE; + return TYPE_MOJO_HANDLE; } +#if defined(OS_POSIX) +base::PlatformFile MojoHandleAttachment::TakePlatformFile() { + NOTREACHED(); + return base::kInvalidPlatformFile; +} +#endif // OS_POSIX + mojo::ScopedHandle MojoHandleAttachment::TakeHandle() { return std::move(handle_); } diff --git a/ipc/ipc_mojo_handle_attachment.h b/ipc/ipc_mojo_handle_attachment.h index d6152769efd59e..6aa1888c02d49c 100644 --- a/ipc/ipc_mojo_handle_attachment.h +++ b/ipc/ipc_mojo_handle_attachment.h @@ -26,6 +26,11 @@ class IPC_EXPORT MojoHandleAttachment : public MessageAttachment { Type GetType() const override; +#if defined(OS_POSIX) + // Should not be called. + base::PlatformFile TakePlatformFile() override; +#endif // OS_POSIX + // Returns the owning handle transferring the ownership. mojo::ScopedHandle TakeHandle(); diff --git a/ipc/ipc_mojo_message_helper.cc b/ipc/ipc_mojo_message_helper.cc index a87a2d694d5f97..8f869455ba2e2f 100644 --- a/ipc/ipc_mojo_message_helper.cc +++ b/ipc/ipc_mojo_message_helper.cc @@ -32,7 +32,7 @@ bool MojoMessageHelper::ReadMessagePipeFrom( MessageAttachment::Type type = static_cast(attachment.get())->GetType(); - if (type != MessageAttachment::Type::MOJO_HANDLE) { + if (type != MessageAttachment::TYPE_MOJO_HANDLE) { LOG(ERROR) << "Unxpected attachment type:" << type; return false; } diff --git a/ipc/ipc_platform_file_attachment_posix.cc b/ipc/ipc_platform_file_attachment_posix.cc index 7111cfa0bbb497..b130ab26eb7db6 100644 --- a/ipc/ipc_platform_file_attachment_posix.cc +++ b/ipc/ipc_platform_file_attachment_posix.cc @@ -20,7 +20,7 @@ PlatformFileAttachment::~PlatformFileAttachment() { } MessageAttachment::Type PlatformFileAttachment::GetType() const { - return Type::PLATFORM_FILE; + return TYPE_PLATFORM_FILE; } base::PlatformFile PlatformFileAttachment::TakePlatformFile() { @@ -30,7 +30,7 @@ base::PlatformFile PlatformFileAttachment::TakePlatformFile() { base::PlatformFile GetPlatformFile( scoped_refptr attachment) { - DCHECK_EQ(attachment->GetType(), MessageAttachment::Type::PLATFORM_FILE); + DCHECK_EQ(attachment->GetType(), MessageAttachment::TYPE_PLATFORM_FILE); return static_cast(attachment.get())->file(); } diff --git a/ipc/ipc_platform_file_attachment_posix.h b/ipc/ipc_platform_file_attachment_posix.h index 9b0790093b0608..d1eff609c5340b 100644 --- a/ipc/ipc_platform_file_attachment_posix.h +++ b/ipc/ipc_platform_file_attachment_posix.h @@ -23,7 +23,7 @@ class IPC_EXPORT PlatformFileAttachment : public MessageAttachment { explicit PlatformFileAttachment(base::ScopedFD file); Type GetType() const override; - base::PlatformFile TakePlatformFile(); + base::PlatformFile TakePlatformFile() override; base::PlatformFile file() const { return file_; } bool Owns() const { return owning_.is_valid(); } diff --git a/ipc/mach_port_attachment_mac.cc b/ipc/mach_port_attachment_mac.cc index 55a85cba7d04e4..215afa967adeb0 100644 --- a/ipc/mach_port_attachment_mac.cc +++ b/ipc/mach_port_attachment_mac.cc @@ -24,6 +24,10 @@ MachPortAttachmentMac::MachPortAttachmentMac(mach_port_t mach_port, FromWire from_wire) : mach_port_(mach_port), owns_mach_port_(true) {} +MachPortAttachmentMac::MachPortAttachmentMac(const WireFormat& wire_format) + : mach_port_(static_cast(wire_format.mach_port)), + owns_mach_port_(true) {} + MachPortAttachmentMac::~MachPortAttachmentMac() { if (mach_port_ != MACH_PORT_NULL && owns_mach_port_) { kern_return_t kr = mach_port_mod_refs(mach_task_self(), mach_port_, @@ -33,8 +37,14 @@ MachPortAttachmentMac::~MachPortAttachmentMac() { } } -MessageAttachment::Type MachPortAttachmentMac::GetType() const { - return Type::MACH_PORT; +MachPortAttachmentMac::BrokerableType MachPortAttachmentMac::GetBrokerableType() + const { + return MACH_PORT; +} + +MachPortAttachmentMac::WireFormat MachPortAttachmentMac::GetWireFormat( + const base::ProcessId& destination) const { + return WireFormat(static_cast(mach_port_), destination); } } // namespace internal diff --git a/ipc/mach_port_attachment_mac.h b/ipc/mach_port_attachment_mac.h index 861c4e30656351..efc2c11a784a74 100644 --- a/ipc/mach_port_attachment_mac.h +++ b/ipc/mach_port_attachment_mac.h @@ -10,16 +10,36 @@ #include "base/macros.h" #include "base/process/process_handle.h" +#include "ipc/brokerable_attachment.h" #include "ipc/ipc_export.h" -#include "ipc/ipc_message_attachment.h" #include "ipc/mach_port_mac.h" namespace IPC { namespace internal { // This class represents an OSX mach_port_t attached to a Chrome IPC message. -class IPC_EXPORT MachPortAttachmentMac : public MessageAttachment { +class IPC_EXPORT MachPortAttachmentMac : public BrokerableAttachment { public: + struct IPC_EXPORT WireFormat { + // IPC translation requires that classes passed through IPC have a default + // constructor. + WireFormat() : mach_port(0), destination_process(0) {} + + WireFormat(uint32_t mach_port, const base::ProcessId& destination_process) + : mach_port(mach_port), destination_process(destination_process) {} + + // The mach port that is intended for duplication, or the mach port that has + // been duplicated, depending on context. + // The type is uint32_t instead of mach_port_t to ensure that the wire + // format stays consistent. + uint32_t mach_port; + static_assert(sizeof(mach_port_t) <= sizeof(uint32_t), + "mach_port_t must be smaller than uint32_t"); + + // The id of the destination process that the handle is duplicated into. + base::ProcessId destination_process; + }; + // This constructor increments the ref count of |mach_port_| and takes // ownership of the result. Should only be called by the sender of a Chrome // IPC message. @@ -32,7 +52,15 @@ class IPC_EXPORT MachPortAttachmentMac : public MessageAttachment { // ref count. Should only be called by the receiver of a Chrome IPC message. MachPortAttachmentMac(mach_port_t mach_port, FromWire from_wire); - Type GetType() const override; + // This constructor takes ownership of |wire_format.mach_port|, but does not + // modify its ref count. Should only be called by the receiver of a Chrome IPC + // message. + explicit MachPortAttachmentMac(const WireFormat& wire_format); + + BrokerableType GetBrokerableType() const override; + + // Returns the wire format of this attachment. + WireFormat GetWireFormat(const base::ProcessId& destination) const; mach_port_t get_mach_port() const { return mach_port_; } diff --git a/ipc/mach_port_mac.cc b/ipc/mach_port_mac.cc index 6d3045a560660f..a482d24d7f03ee 100644 --- a/ipc/mach_port_mac.cc +++ b/ipc/mach_port_mac.cc @@ -34,10 +34,16 @@ bool ParamTraits::Read(const base::Pickle* m, return false; MessageAttachment* attachment = static_cast(base_attachment.get()); - if (attachment->GetType() != MessageAttachment::Type::MACH_PORT) + if (attachment->GetType() != MessageAttachment::TYPE_BROKERABLE_ATTACHMENT) return false; + BrokerableAttachment* brokerable_attachment = + static_cast(attachment); + if (brokerable_attachment->GetBrokerableType() != + BrokerableAttachment::MACH_PORT) { + return false; + } IPC::internal::MachPortAttachmentMac* mach_port_attachment = - static_cast(attachment); + static_cast(brokerable_attachment); r->set_mach_port(mach_port_attachment->get_mach_port()); mach_port_attachment->reset_mach_port_ownership(); return true; diff --git a/ppapi/proxy/nacl_message_scanner.cc b/ppapi/proxy/nacl_message_scanner.cc index 19edce26768e4e..e31ee38bf739fb 100644 --- a/ppapi/proxy/nacl_message_scanner.cc +++ b/ppapi/proxy/nacl_message_scanner.cc @@ -63,6 +63,7 @@ void WriteHandle(int handle_index, // Now write the handle itself in POSIX style. // See ParamTraits::Read for where these values are read. msg->WriteBool(true); // valid == true + msg->WriteBool(false); // brokerable == false msg->WriteInt(handle_index); } }