diff --git a/ipc/chromium/src/chrome/common/ipc_channel_win.cc b/ipc/chromium/src/chrome/common/ipc_channel_win.cc index ae7a254bc0ca..29fae1d76ed6 100644 --- a/ipc/chromium/src/chrome/common/ipc_channel_win.cc +++ b/ipc/chromium/src/chrome/common/ipc_channel_win.cc @@ -651,65 +651,46 @@ bool Channel::ChannelImpl::AcceptHandles(Message& msg) { return false; } - // Seek to the start of our handle payload. - mozilla::CheckedInt handles_payload_size(num_handles); - handles_payload_size *= sizeof(uint32_t); - if (!handles_payload_size.isValid() || - handles_payload_size.value() > msg.header()->payload_size) { - CHROMIUM_LOG(ERROR) << "invalid handle count " << num_handles - << " or payload size: " << msg.header()->payload_size; - return false; - } - uint32_t handles_offset = - msg.header()->payload_size - handles_payload_size.value(); - - PickleIterator handles_start{msg}; - if (!msg.IgnoreBytes(&handles_start, handles_offset)) { - CHROMIUM_LOG(ERROR) << "IgnoreBytes failed"; + // Read in the payload from the footer, truncating the message. + nsTArray payload; + payload.AppendElements(num_handles); + if (!msg.ReadFooter(payload.Elements(), num_handles * sizeof(uint32_t), + /* truncate */ true)) { + CHROMIUM_LOG(ERROR) << "failed to read handle payload from message"; return false; } + msg.header()->num_handles = 0; // Read in the handles themselves, transferring ownership as required. - nsTArray handles; - { - PickleIterator iter{handles_start}; - for (uint32_t i = 0; i < num_handles; ++i) { - uint32_t handleValue; - if (!msg.ReadUInt32(&iter, &handleValue)) { - CHROMIUM_LOG(ERROR) << "failed to read handle value"; + nsTArray handles(num_handles); + for (uint32_t handleValue : payload) { + HANDLE handle = Uint32ToHandle(handleValue); + + // If we're the privileged process, the remote process will have leaked + // the sent handles in its local address space, and be relying on us to + // duplicate them, otherwise the remote privileged side will have + // transferred the handles to us already. + if (privileged_) { + if (other_process_ == INVALID_HANDLE_VALUE) { + CHROMIUM_LOG(ERROR) << "other_process_ is invalid in AcceptHandles"; return false; } - HANDLE handle = Uint32ToHandle(handleValue); - - // If we're the privileged process, the remote process will have leaked - // the sent handles in its local address space, and be relying on us to - // duplicate them, otherwise the remote privileged side will have - // transferred the handles to us already. - if (privileged_) { - if (other_process_ == INVALID_HANDLE_VALUE) { - CHROMIUM_LOG(ERROR) << "other_process_ is invalid in AcceptHandles"; - return false; - } - if (!::DuplicateHandle( - other_process_, handle, GetCurrentProcess(), &handle, 0, FALSE, - DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE)) { - CHROMIUM_LOG(ERROR) << "DuplicateHandle failed for handle " << handle - << " in AcceptHandles"; - return false; - } + if (!::DuplicateHandle(other_process_, handle, GetCurrentProcess(), + &handle, 0, FALSE, + DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE)) { + CHROMIUM_LOG(ERROR) << "DuplicateHandle failed for handle " << handle + << " in AcceptHandles"; + return false; } - - // The handle is directly owned by this process now, and can be added to - // our `handles` array. - handles.AppendElement(mozilla::UniqueFileHandle(handle)); } + + // The handle is directly owned by this process now, and can be added to + // our `handles` array. + handles.AppendElement(mozilla::UniqueFileHandle(handle)); } // We're done with the handle footer, truncate the message at that point. - msg.Truncate(&handles_start); msg.SetAttachedFileHandles(std::move(handles)); - msg.header()->num_handles = 0; - MOZ_ASSERT(msg.header()->payload_size == handles_offset); MOZ_ASSERT(msg.num_handles() == num_handles); return true; } @@ -732,7 +713,7 @@ bool Channel::ChannelImpl::TransferHandles(Message& msg) { uint32_t handles_offset = msg.header()->payload_size; #endif - // Write handles from `attached_handles_` into the message payload. + nsTArray payload(num_handles); for (uint32_t i = 0; i < num_handles; ++i) { // Release ownership of the handle. It'll be cloned when the parent process // transfers it with DuplicateHandle either in this process or the remote @@ -756,14 +737,13 @@ bool Channel::ChannelImpl::TransferHandles(Message& msg) { } } - if (!msg.WriteUInt32(HandleToUint32(handle))) { - CHROMIUM_LOG(ERROR) << "failed to write handle value " << handle; - return false; - } + payload.AppendElement(HandleToUint32(handle)); } msg.attached_handles_.Clear(); + msg.WriteFooter(payload.Elements(), payload.Length() * sizeof(uint32_t)); msg.header()->num_handles = num_handles; + MOZ_ASSERT(msg.header()->payload_size == handles_offset + (sizeof(uint32_t) * num_handles), "Unexpected number of bytes written for handles footer?"); diff --git a/ipc/chromium/src/chrome/common/ipc_message.cc b/ipc/chromium/src/chrome/common/ipc_message.cc index d3ed3c686813..8a35ae1779af 100644 --- a/ipc/chromium/src/chrome/common/ipc_message.cc +++ b/ipc/chromium/src/chrome/common/ipc_message.cc @@ -44,7 +44,7 @@ Message::Message(int32_t routing_id, msgid_t type, uint32_t segment_capacity, #if defined(OS_MACOSX) header()->cookie = 0; #endif - header()->footer_offset = -1; + header()->event_footer_size = 0; if (recordWriteLatency) { create_time_ = mozilla::TimeStamp::Now(); } @@ -95,54 +95,41 @@ Message& Message::operator=(Message&& other) { } void Message::WriteFooter(const void* data, uint32_t data_len) { - MOZ_ASSERT(header()->footer_offset < 0, "Already wrote a footer!"); if (data_len == 0) { return; } - // Record the start of the footer. - header()->footer_offset = header()->payload_size; - WriteBytes(data, data_len); } bool Message::ReadFooter(void* buffer, uint32_t buffer_len, bool truncate) { - MOZ_ASSERT(buffer_len == FooterSize()); - MOZ_ASSERT(header()->footer_offset <= int64_t(header()->payload_size)); if (buffer_len == 0) { return true; } - // FIXME: This is a really inefficient way to seek to the end of the message - // for sufficiently large messages. - PickleIterator footer_iter(*this); - if (NS_WARN_IF(!IgnoreBytes(&footer_iter, header()->footer_offset))) { + if (NS_WARN_IF(AlignInt(header()->payload_size) != header()->payload_size) || + NS_WARN_IF(AlignInt(buffer_len) > header()->payload_size)) { + return false; + } + + // Seek to the start of the footer, and read it in. We read in with a + // duplicate of the iterator so we can use it to truncate later. + uint32_t offset = header()->payload_size - AlignInt(buffer_len); + PickleIterator footer_iter(*this); + if (NS_WARN_IF(!IgnoreBytes(&footer_iter, offset))) { return false; } - // Use a copy of the footer iterator for reading bytes so that we can use the - // previous iterator to truncate the message if requested. PickleIterator read_iter(footer_iter); bool ok = ReadBytesInto(&read_iter, buffer, buffer_len); - // If requested, truncate the buffer to the start of the footer, and clear our - // footer offset back to `-1`. + // If requested, truncate the buffer to the start of the footer. if (truncate) { - header()->footer_offset = -1; Truncate(&footer_iter); } - return ok; } -uint32_t Message::FooterSize() const { - if (header()->footer_offset >= 0 && - uint32_t(header()->footer_offset) < header()->payload_size) { - return header()->payload_size - header()->footer_offset; - } - return 0; -} - bool Message::WriteFileHandle(mozilla::UniqueFileHandle handle) { uint32_t handle_index = attached_handles_.Length(); WriteUInt32(handle_index); diff --git a/ipc/chromium/src/chrome/common/ipc_message.h b/ipc/chromium/src/chrome/common/ipc_message.h index 7362236618e9..f1581876a301 100644 --- a/ipc/chromium/src/chrome/common/ipc_message.h +++ b/ipc/chromium/src/chrome/common/ipc_message.h @@ -292,10 +292,20 @@ class Message : public mojo::core::ports::UserMessage, public Pickle { size_t GetSizeIfSerialized() const override { return size(); } bool WillBeRoutedExternally(mojo::core::ports::UserMessageEvent&) override; + // Write the given footer bytes to the end of the current message. The + // footer's `data_len` will be padded to a multiple of 4 bytes. void WriteFooter(const void* data, uint32_t data_len); + // Read a footer written with `WriteFooter` from the end of the message, given + // a buffer and the length of the footer. If `truncate` is true, the message + // will be truncated, removing the footer. [[nodiscard]] bool ReadFooter(void* buffer, uint32_t buffer_len, bool truncate); - uint32_t FooterSize() const; + + uint32_t event_footer_size() const { return header()->event_footer_size; } + + void set_event_footer_size(uint32_t size) { + header()->event_footer_size = size; + } // Used for async messages with no parameters. static void Log(const Message* msg, std::wstring* l) {} @@ -372,8 +382,8 @@ class Message : public mojo::core::ports::UserMessage, public Pickle { uint32_t interrupt_local_stack_depth; // Sequence number int32_t seqno; - // Offset of the message's footer in the payload, or -1 if invalid. - int32_t footer_offset; + // Size of the message's event footer + uint32_t event_footer_size; }; Header* header() { return headerT
(); } diff --git a/ipc/glue/NodeController.cpp b/ipc/glue/NodeController.cpp index dbbaf6762119..5724c9a1d698 100644 --- a/ipc/glue/NodeController.cpp +++ b/ipc/glue/NodeController.cpp @@ -197,12 +197,13 @@ auto NodeController::SerializeEventMessage(UniquePtr aEvent, } message->WriteFooter(buffer.begin(), buffer.length()); + message->set_event_footer_size(buffer.length()); #ifdef DEBUG // Debug-assert that we can read the same data back out of the buffer. - MOZ_ASSERT(message->FooterSize() == length); + MOZ_ASSERT(message->event_footer_size() == length); Vector buffer2; - (void)buffer2.initLengthUninitialized(message->FooterSize()); + (void)buffer2.initLengthUninitialized(message->event_footer_size()); MOZ_ASSERT(message->ReadFooter(buffer2.begin(), buffer2.length(), /* truncate */ false)); MOZ_ASSERT(!memcmp(buffer2.begin(), buffer.begin(), buffer.length())); @@ -220,7 +221,7 @@ auto NodeController::DeserializeEventMessage(UniquePtr aMessage, } Vector buffer; - (void)buffer.initLengthUninitialized(aMessage->FooterSize()); + (void)buffer.initLengthUninitialized(aMessage->event_footer_size()); // Truncate the message when reading the footer, so that the extra footer data // is no longer present in the message. This allows future code to eventually // send the same `IPC::Message` to another process. @@ -230,6 +231,7 @@ auto NodeController::DeserializeEventMessage(UniquePtr aMessage, aMessage->name()); return nullptr; } + aMessage->set_event_footer_size(0); UniquePtr event; if (aRelayTarget) {