From 624faa76bebad4b0416c7a6a11dc62d646f7a52e Mon Sep 17 00:00:00 2001 From: erikchen Date: Tue, 1 Dec 2015 16:40:07 -0800 Subject: [PATCH] [Merge M48] ipc: Minor fixes to AttachmentBrokerPrivilegedMac. > Two small changes: > - Better cleanup when a process has gone away, but extractors or precursors are > still present. > - Remove an unused method. > > BUG=561105 > > Review URL: https://codereview.chromium.org/1473883002 > > Cr-Commit-Position: refs/heads/master@{#362057} (cherry picked from commit 28f0a585535c129e0303281ee4a58f6c07384c48) TBR=tsepez@chromium.org, isherman@chromium.org BUG=563762 Review URL: https://codereview.chromium.org/1491023002 . Cr-Commit-Position: refs/branch-heads/2564@{#189} Cr-Branched-From: 1283eca15bd9f772387f75241576cde7bdec7f54-refs/heads/master@{#359700} --- ipc/attachment_broker_privileged.h | 3 + ipc/attachment_broker_privileged_mac.cc | 88 ++++++++++++++++++------- ipc/attachment_broker_privileged_mac.h | 12 ++-- tools/metrics/histograms/histograms.xml | 3 + 4 files changed, 77 insertions(+), 29 deletions(-) diff --git a/ipc/attachment_broker_privileged.h b/ipc/attachment_broker_privileged.h index 5a55e67da7fa0..cbaaaf276d267 100644 --- a/ipc/attachment_broker_privileged.h +++ b/ipc/attachment_broker_privileged.h @@ -79,6 +79,9 @@ class IPC_EXPORT AttachmentBrokerPrivileged : public IPC::AttachmentBroker { ERROR_DECREASE_REF = 10, // Couldn't extract a right from the source. ERROR_EXTRACT_SOURCE_RIGHT = 11, + // The broker did not have a channel of communication with the source + // process. + ERROR_SOURCE_NOT_FOUND = 12, ERROR_MAX }; diff --git a/ipc/attachment_broker_privileged_mac.cc b/ipc/attachment_broker_privileged_mac.cc index 16dbff4d62cac..7ec9e1ad04ec6 100644 --- a/ipc/attachment_broker_privileged_mac.cc +++ b/ipc/attachment_broker_privileged_mac.cc @@ -96,6 +96,36 @@ bool AttachmentBrokerPrivilegedMac::SendAttachmentToProcess( return false; } +void AttachmentBrokerPrivilegedMac::DeregisterCommunicationChannel( + Endpoint* endpoint) { + AttachmentBrokerPrivileged::DeregisterCommunicationChannel(endpoint); + + if (!endpoint) + return; + + base::ProcessId pid = endpoint->GetPeerPID(); + if (pid == base::kNullProcessId) + return; + + { + base::AutoLock l(precursors_lock_); + auto it = precursors_.find(pid); + if (it != precursors_.end()) { + delete it->second; + precursors_.erase(pid); + } + } + + { + base::AutoLock l(extractors_lock_); + auto it = extractors_.find(pid); + if (it != extractors_.end()) { + delete it->second; + extractors_.erase(pid); + } + } +} + bool AttachmentBrokerPrivilegedMac::OnMessageReceived(const Message& msg) { bool handled = true; switch (msg.type()) { @@ -169,7 +199,7 @@ void AttachmentBrokerPrivilegedMac::RoutePrecursorToSelf( HandleReceivedAttachment(attachment); } -void AttachmentBrokerPrivilegedMac::RouteWireFormatToAnother( +bool AttachmentBrokerPrivilegedMac::RouteWireFormatToAnother( const MachPortWireFormat& wire_format) { DCHECK_NE(wire_format.destination_process, base::Process::Current().Pid()); @@ -183,11 +213,12 @@ void AttachmentBrokerPrivilegedMac::RouteWireFormatToAnother( LOG(ERROR) << "Failed to deliver brokerable attachment to process with id: " << dest; LogError(DESTINATION_NOT_FOUND); - return; + return false; } LogError(DESTINATION_FOUND); sender->Send(new AttachmentBrokerMsg_MachPortHasBeenDuplicated(wire_format)); + return true; } mach_port_name_t AttachmentBrokerPrivilegedMac::CreateIntermediateMachPort( @@ -245,21 +276,6 @@ mach_port_name_t AttachmentBrokerPrivilegedMac::CreateIntermediateMachPort( return endpoint; } -base::mac::ScopedMachSendRight AttachmentBrokerPrivilegedMac::AcquireSendRight( - base::ProcessId pid, - mach_port_name_t named_right) { - if (pid == base::GetCurrentProcId()) { - kern_return_t kr = mach_port_mod_refs(mach_task_self(), named_right, - MACH_PORT_RIGHT_SEND, 1); - if (kr != KERN_SUCCESS) - return base::mac::ScopedMachSendRight(MACH_PORT_NULL); - return base::mac::ScopedMachSendRight(named_right); - } - - mach_port_t task_port = port_provider_->TaskForPid(pid); - return ExtractNamedRight(task_port, named_right); -} - base::mac::ScopedMachSendRight AttachmentBrokerPrivilegedMac::ExtractNamedRight( mach_port_t task_port, mach_port_name_t named_right) { @@ -305,16 +321,32 @@ void AttachmentBrokerPrivilegedMac::SendPrecursorsForProcess( // Whether this process is the destination process. bool to_self = pid == base::GetCurrentProcId(); + if (!to_self) { + if (!GetSenderWithProcessId(pid)) { + // If there is no sender, then the destination process is no longer + // running, or never existed to begin with. + LogError(DESTINATION_NOT_FOUND); + delete it->second; + precursors_.erase(it); + return; + } + } + mach_port_t task_port = port_provider_->TaskForPid(pid); + + // It's possible that the destination process has not yet provided the + // privileged process with its task port. if (!to_self && task_port == MACH_PORT_NULL) return; while (!it->second->empty()) { auto precursor_it = it->second->begin(); - if (to_self) + if (to_self) { RoutePrecursorToSelf(*precursor_it); - else - SendPrecursor(*precursor_it, task_port); + } else { + if (!SendPrecursor(*precursor_it, task_port)) + break; + } it->second->erase(precursor_it); } @@ -322,7 +354,7 @@ void AttachmentBrokerPrivilegedMac::SendPrecursorsForProcess( precursors_.erase(it); } -void AttachmentBrokerPrivilegedMac::SendPrecursor( +bool AttachmentBrokerPrivilegedMac::SendPrecursor( AttachmentPrecursor* precursor, mach_port_t task_port) { DCHECK(task_port); @@ -334,7 +366,8 @@ void AttachmentBrokerPrivilegedMac::SendPrecursor( intermediate_port = CreateIntermediateMachPort( task_port, base::mac::ScopedMachSendRight(port_to_insert.release())); } - RouteWireFormatToAnother(CopyWireFormat(wire_format, intermediate_port)); + return RouteWireFormatToAnother( + CopyWireFormat(wire_format, intermediate_port)); } void AttachmentBrokerPrivilegedMac::AddPrecursor( @@ -357,7 +390,18 @@ void AttachmentBrokerPrivilegedMac::ProcessExtractorsForProcess( if (it == extractors_.end()) return; + if (!GetSenderWithProcessId(pid)) { + // If there is no sender, then the source process is no longer running. + LogError(ERROR_SOURCE_NOT_FOUND); + delete it->second; + extractors_.erase(it); + return; + } + mach_port_t task_port = port_provider_->TaskForPid(pid); + + // It's possible that the source process has not yet provided the privileged + // process with its task port. if (task_port == MACH_PORT_NULL) return; diff --git a/ipc/attachment_broker_privileged_mac.h b/ipc/attachment_broker_privileged_mac.h index 36abaf3ba03d2..86bfb6fd37e0c 100644 --- a/ipc/attachment_broker_privileged_mac.h +++ b/ipc/attachment_broker_privileged_mac.h @@ -64,6 +64,7 @@ class IPC_EXPORT AttachmentBrokerPrivilegedMac bool SendAttachmentToProcess( const scoped_refptr& attachment, base::ProcessId destination_process) override; + void DeregisterCommunicationChannel(Endpoint* endpoint) override; // IPC::Listener overrides. bool OnMessageReceived(const Message& message) override; @@ -150,11 +151,6 @@ class IPC_EXPORT AttachmentBrokerPrivilegedMac mach_port_t task_port, base::mac::ScopedMachSendRight port_to_insert); - // Acquire a send right to a named right in |pid|. - // Returns MACH_PORT_NULL on error. - base::mac::ScopedMachSendRight AcquireSendRight(base::ProcessId pid, - mach_port_name_t named_right); - // Extracts a copy of the send right to |named_right| from |task_port|. // Returns MACH_PORT_NULL on error. base::mac::ScopedMachSendRight ExtractNamedRight( @@ -176,14 +172,16 @@ class IPC_EXPORT AttachmentBrokerPrivilegedMac // |wire_format.mach_port| must be the intermediate Mach port. // Ownership of |wire_format.mach_port| is implicitly passed to the process // that receives the Chrome IPC message. - void RouteWireFormatToAnother(const MachPortWireFormat& wire_format); + // Returns |false| on irrecoverable error. + bool RouteWireFormatToAnother(const MachPortWireFormat& wire_format); // Atempts to broker all precursors whose destination is |pid|. Has no effect // if |port_provider_| does not have the task port for |pid|. void SendPrecursorsForProcess(base::ProcessId pid); // Brokers a single precursor into the task represented by |task_port|. - void SendPrecursor(AttachmentPrecursor* precursor, mach_port_t task_port); + // Returns |false| on irrecoverable error. + bool SendPrecursor(AttachmentPrecursor* precursor, mach_port_t task_port); // Add a precursor to |precursors_|. Takes ownership of |port|. void AddPrecursor(base::ProcessId pid, diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index a6cc9da8c6c04..f2b4a39573b0f 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -65067,6 +65067,9 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. Couldn't extract a right from the source process. + + Broker didn't have a channel of communication with the source process. +