Commit 2f8cd83e authored by Ken Rockot's avatar Ken Rockot Committed by Commit Bot

Revert "Mojo EDK: Improve internal process handle ownership"

This reverts commit 27c99670.

Reason for revert: https://crbug.com/841565

Original change's description:
> Mojo EDK: Improve internal process handle ownership
> 
> Mojo passes around base::ProcessHandle values for various reasons. On
> most systems this is fine, but at least on Windows, a ProcessHandle
> refers to an owned reference to a system process object, and if not
> careful it's possible for a base::ProcessHandle value to inadvertently
> change meaning over time.
> 
> This CL introduces the concept of a move-only ScopedProcessHandle
> within Mojo, which on most platforms is just a base::PlatformHandle.
> On Windows, this represents an owned base::ProcessHandle which closes
> on destruction and clones correctly using DuplicateHandle rather than
> merely copying the raw handle value.
> 
> ScopedProcessHandle is used in a few places where process handle
> ownership semantics were previously weaker than necessary, or were
> correct but implemented ad hoc.
> 
> This also updates ScopedPlatformHandle (and supporting code like
> Channel::RewriteHandles) such that the |owning_process| field (if not
> the current process) is always an owned process handle. This ensures
> that when such handles are closed in unsent messages, they can be
> safely closed in the target process (from within the source process)
> without any risk of raciness against target process termination.
> 
> Bug: 837612
> Change-Id: I943bb5f70ede56351d52b2ecea7d76fcfdee46ce
> Reviewed-on: https://chromium-review.googlesource.com/1036459
> Reviewed-by: Jay Civelli <jcivelli@chromium.org>
> Commit-Queue: Ken Rockot <rockot@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#555117}

TBR=jcivelli@chromium.org,rockot@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 837612
Change-Id: Ief6e1d6d6f2f96dc7420e06d8438cc06cbf17490
Reviewed-on: https://chromium-review.googlesource.com/1057699Reviewed-by: default avatarJay Civelli <jcivelli@chromium.org>
Reviewed-by: default avatarKen Rockot <rockot@chromium.org>
Commit-Queue: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558388}
parent 7ca50812
...@@ -82,7 +82,6 @@ template("core_impl_source_set") { ...@@ -82,7 +82,6 @@ template("core_impl_source_set") {
"system/platform_handle_dispatcher.h", "system/platform_handle_dispatcher.h",
"system/platform_shared_memory_mapping.h", "system/platform_shared_memory_mapping.h",
"system/request_context.h", "system/request_context.h",
"system/scoped_process_handle.h",
"system/shared_buffer_dispatcher.h", "system/shared_buffer_dispatcher.h",
"system/user_message_impl.h", "system/user_message_impl.h",
] ]
...@@ -123,7 +122,6 @@ template("core_impl_source_set") { ...@@ -123,7 +122,6 @@ template("core_impl_source_set") {
"system/platform_handle_dispatcher.cc", "system/platform_handle_dispatcher.cc",
"system/platform_shared_memory_mapping.cc", "system/platform_shared_memory_mapping.cc",
"system/request_context.cc", "system/request_context.cc",
"system/scoped_process_handle.cc",
"system/shared_buffer_dispatcher.cc", "system/shared_buffer_dispatcher.cc",
"system/user_message_impl.cc", "system/user_message_impl.cc",
"system/watch.cc", "system/watch.cc",
......
...@@ -39,9 +39,7 @@ void PlatformHandle::CloseIfNecessary() { ...@@ -39,9 +39,7 @@ void PlatformHandle::CloseIfNecessary() {
// * Set hSourceProcessHandle to the target process from the // * Set hSourceProcessHandle to the target process from the
// call that created the handle. // call that created the handle.
// * Set hSourceHandle to the duplicated handle to close. // * Set hSourceHandle to the duplicated handle to close.
// * Set lpTargetHandle [sic] to NULL. (N.B.: This appears to be a // * Set lpTargetHandle to NULL.
// documentation bug; what matters is that hTargetProcessHandle is
// NULL.)
// * Set dwOptions to DUPLICATE_CLOSE_SOURCE. // * Set dwOptions to DUPLICATE_CLOSE_SOURCE.
// //
// [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms724251 // [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms724251
...@@ -49,9 +47,8 @@ void PlatformHandle::CloseIfNecessary() { ...@@ -49,9 +47,8 @@ void PlatformHandle::CloseIfNecessary() {
// NOTE: It's possible for this operation to fail if the owning process // NOTE: It's possible for this operation to fail if the owning process
// was terminated or is in the process of being terminated. Either way, // was terminated or is in the process of being terminated. Either way,
// there is nothing we can reasonably do about failure, so we ignore it. // there is nothing we can reasonably do about failure, so we ignore it.
::DuplicateHandle(owning_process, handle, NULL, &handle, 0, FALSE, DuplicateHandle(owning_process, handle, NULL, &handle, 0, FALSE,
DUPLICATE_CLOSE_SOURCE); DUPLICATE_CLOSE_SOURCE);
::CloseHandle(owning_process);
return; return;
} }
......
...@@ -26,7 +26,7 @@ BrokerHost::BrokerHost(base::ProcessHandle client_process, ...@@ -26,7 +26,7 @@ BrokerHost::BrokerHost(base::ProcessHandle client_process,
: process_error_callback_(process_error_callback) : process_error_callback_(process_error_callback)
#if defined(OS_WIN) #if defined(OS_WIN)
, ,
client_process_(ScopedProcessHandle::CloneFrom(client_process)) client_process_(client_process)
#endif #endif
{ {
CHECK(platform_handle.is_valid()); CHECK(platform_handle.is_valid());
...@@ -52,7 +52,7 @@ bool BrokerHost::PrepareHandlesForClient( ...@@ -52,7 +52,7 @@ bool BrokerHost::PrepareHandlesForClient(
std::vector<ScopedPlatformHandle>* handles) { std::vector<ScopedPlatformHandle>* handles) {
#if defined(OS_WIN) #if defined(OS_WIN)
if (!Channel::Message::RewriteHandles(base::GetCurrentProcessHandle(), if (!Channel::Message::RewriteHandles(base::GetCurrentProcessHandle(),
client_process_.get(), handles)) { client_process_, handles)) {
// NOTE: We only log an error here. We do not signal a logical error or // NOTE: We only log an error here. We do not signal a logical error or
// prevent any message from being sent. The client should handle unexpected // prevent any message from being sent. The client should handle unexpected
// invalid handles appropriately. // invalid handles appropriately.
......
...@@ -15,7 +15,6 @@ ...@@ -15,7 +15,6 @@
#include "mojo/edk/embedder/process_error_callback.h" #include "mojo/edk/embedder/process_error_callback.h"
#include "mojo/edk/embedder/scoped_platform_handle.h" #include "mojo/edk/embedder/scoped_platform_handle.h"
#include "mojo/edk/system/channel.h" #include "mojo/edk/system/channel.h"
#include "mojo/edk/system/scoped_process_handle.h"
namespace mojo { namespace mojo {
namespace edk { namespace edk {
...@@ -56,7 +55,7 @@ class BrokerHost : public Channel::Delegate, ...@@ -56,7 +55,7 @@ class BrokerHost : public Channel::Delegate,
const ProcessErrorCallback process_error_callback_; const ProcessErrorCallback process_error_callback_;
#if defined(OS_WIN) #if defined(OS_WIN)
ScopedProcessHandle client_process_; base::ProcessHandle client_process_;
#endif #endif
scoped_refptr<Channel> channel_; scoped_refptr<Channel> channel_;
......
...@@ -476,14 +476,7 @@ bool Channel::Message::RewriteHandles( ...@@ -476,14 +476,7 @@ bool Channel::Message::RewriteHandles(
&(*handles)[i].get().handle, 0, FALSE, &(*handles)[i].get().handle, 0, FALSE,
DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE); DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE);
if (result) { if (result) {
if (to_process == base::GetCurrentProcessHandle()) { (*handles)[i].get().owning_process = to_process;
(*handles)[i].get().owning_process = to_process;
} else {
// If this handle is bound for an external process, make sure it owns
// its own copy of the target process handle.
(*handles)[i].get().owning_process =
ScopedProcessHandle::CloneFrom(to_process).release();
}
} else { } else {
success = false; success = false;
......
...@@ -232,24 +232,41 @@ void NodeChannel::NotifyBadMessage(const std::string& error) { ...@@ -232,24 +232,41 @@ void NodeChannel::NotifyBadMessage(const std::string& error) {
process_error_callback_.Run("Received bad user message: " + error); process_error_callback_.Run("Received bad user message: " + error);
} }
void NodeChannel::SetRemoteProcessHandle(ScopedProcessHandle process_handle) { void NodeChannel::SetRemoteProcessHandle(base::ProcessHandle process_handle) {
DCHECK(io_task_runner_->RunsTasksInCurrentSequence()); DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
base::AutoLock lock(remote_process_handle_lock_); base::AutoLock lock(remote_process_handle_lock_);
DCHECK(!remote_process_handle_.is_valid()); DCHECK_EQ(base::kNullProcessHandle, remote_process_handle_);
CHECK_NE(remote_process_handle_.get(), base::GetCurrentProcessHandle()); CHECK_NE(remote_process_handle_, base::GetCurrentProcessHandle());
remote_process_handle_ = std::move(process_handle); remote_process_handle_ = process_handle;
#if defined(OS_WIN)
DCHECK(!scoped_remote_process_handle_.is_valid());
scoped_remote_process_handle_.reset(PlatformHandle(process_handle));
#endif
} }
bool NodeChannel::HasRemoteProcessHandle() { bool NodeChannel::HasRemoteProcessHandle() {
base::AutoLock lock(remote_process_handle_lock_); base::AutoLock lock(remote_process_handle_lock_);
return remote_process_handle_.is_valid(); return remote_process_handle_ != base::kNullProcessHandle;
} }
ScopedProcessHandle NodeChannel::CloneRemoteProcessHandle() { base::ProcessHandle NodeChannel::CopyRemoteProcessHandle() {
base::AutoLock lock(remote_process_handle_lock_); base::AutoLock lock(remote_process_handle_lock_);
if (!remote_process_handle_.is_valid()) #if defined(OS_WIN)
return ScopedProcessHandle(); if (remote_process_handle_ != base::kNullProcessHandle) {
return remote_process_handle_.Clone(); // Privileged nodes use this to pass their invitees' process handles to the
// broker on launch.
HANDLE handle = remote_process_handle_;
BOOL result =
DuplicateHandle(base::GetCurrentProcessHandle(), remote_process_handle_,
base::GetCurrentProcessHandle(), &handle, 0, FALSE,
DUPLICATE_SAME_ACCESS);
DPCHECK(result);
return handle;
}
return base::kNullProcessHandle;
#else
return remote_process_handle_;
#endif
} }
void NodeChannel::SetRemoteNodeName(const ports::NodeName& name) { void NodeChannel::SetRemoteNodeName(const ports::NodeName& name) {
...@@ -290,12 +307,11 @@ void NodeChannel::AcceptPeer(const ports::NodeName& sender_name, ...@@ -290,12 +307,11 @@ void NodeChannel::AcceptPeer(const ports::NodeName& sender_name,
} }
void NodeChannel::AddBrokerClient(const ports::NodeName& client_name, void NodeChannel::AddBrokerClient(const ports::NodeName& client_name,
ScopedProcessHandle process_handle) { base::ProcessHandle process_handle) {
AddBrokerClientData* data; AddBrokerClientData* data;
std::vector<ScopedPlatformHandle> handles; std::vector<ScopedPlatformHandle> handles;
#if defined(OS_WIN) #if defined(OS_WIN)
handles.emplace_back( handles.emplace_back(ScopedPlatformHandle(PlatformHandle(process_handle)));
ScopedPlatformHandle(PlatformHandle(process_handle.release())));
#endif #endif
Channel::MessagePtr message = Channel::MessagePtr message =
CreateMessage(MessageType::ADD_BROKER_CLIENT, sizeof(AddBrokerClientData), CreateMessage(MessageType::ADD_BROKER_CLIENT, sizeof(AddBrokerClientData),
...@@ -303,7 +319,7 @@ void NodeChannel::AddBrokerClient(const ports::NodeName& client_name, ...@@ -303,7 +319,7 @@ void NodeChannel::AddBrokerClient(const ports::NodeName& client_name,
message->SetHandles(std::move(handles)); message->SetHandles(std::move(handles));
data->client_name = client_name; data->client_name = client_name;
#if !defined(OS_WIN) #if !defined(OS_WIN)
data->process_handle = process_handle.get(); data->process_handle = process_handle;
data->padding = 0; data->padding = 0;
#endif #endif
WriteChannelMessage(std::move(message)); WriteChannelMessage(std::move(message));
...@@ -485,13 +501,14 @@ void NodeChannel::OnChannelMessage(const void* payload, ...@@ -485,13 +501,14 @@ void NodeChannel::OnChannelMessage(const void* payload,
// from a privileged descendant. // from a privileged descendant.
{ {
base::AutoLock lock(remote_process_handle_lock_); base::AutoLock lock(remote_process_handle_lock_);
if (!handles.empty() && remote_process_handle_.is_valid()) { if (!handles.empty() &&
remote_process_handle_ != base::kNullProcessHandle) {
// Note that we explicitly mark the handles as being owned by the sending // Note that we explicitly mark the handles as being owned by the sending
// process before rewriting them, in order to accommodate RewriteHandles' // process before rewriting them, in order to accommodate RewriteHandles'
// internal sanity checks. // internal sanity checks.
for (auto& handle : handles) for (auto& handle : handles)
handle.get().owning_process = remote_process_handle_.get(); handle.get().owning_process = remote_process_handle_;
if (!Channel::Message::RewriteHandles(remote_process_handle_.get(), if (!Channel::Message::RewriteHandles(remote_process_handle_,
base::GetCurrentProcessHandle(), base::GetCurrentProcessHandle(),
&handles)) { &handles)) {
DLOG(ERROR) << "Received one or more invalid handles."; DLOG(ERROR) << "Received one or more invalid handles.";
...@@ -652,10 +669,7 @@ void NodeChannel::OnChannelMessage(const void* payload, ...@@ -652,10 +669,7 @@ void NodeChannel::OnChannelMessage(const void* payload,
base::ProcessHandle from_process; base::ProcessHandle from_process;
{ {
base::AutoLock lock(remote_process_handle_lock_); base::AutoLock lock(remote_process_handle_lock_);
// NOTE: It's safe to retain a weak reference to this process handle from_process = remote_process_handle_;
// through the extent of this call because |this| is kept alive and
// |remote_process_handle_| is never reset once set.
from_process = remote_process_handle_.get();
} }
const RelayEventMessageData* data; const RelayEventMessageData* data;
if (GetMessagePayload(payload, payload_size, &data)) { if (GetMessagePayload(payload, payload_size, &data)) {
...@@ -786,7 +800,7 @@ void NodeChannel::ProcessPendingMessagesWithMachPorts() { ...@@ -786,7 +800,7 @@ void NodeChannel::ProcessPendingMessagesWithMachPorts() {
base::ProcessHandle remote_process_handle; base::ProcessHandle remote_process_handle;
{ {
base::AutoLock lock(remote_process_handle_lock_); base::AutoLock lock(remote_process_handle_lock_);
remote_process_handle = remote_process_handle_.get(); remote_process_handle = remote_process_handle_;
} }
PendingMessageQueue pending_writes; PendingMessageQueue pending_writes;
PendingRelayMessageQueue pending_relays; PendingRelayMessageQueue pending_relays;
...@@ -832,14 +846,17 @@ void NodeChannel::WriteChannelMessage(Channel::MessagePtr message) { ...@@ -832,14 +846,17 @@ void NodeChannel::WriteChannelMessage(Channel::MessagePtr message) {
// here (they'll be unpacked and duplicated by the broker). // here (they'll be unpacked and duplicated by the broker).
if (message->has_handles()) { if (message->has_handles()) {
base::AutoLock lock(remote_process_handle_lock_); base::ProcessHandle remote_process_handle;
{
base::AutoLock lock(remote_process_handle_lock_);
remote_process_handle = remote_process_handle_;
}
// Rewrite outgoing handles if we have a handle to the destination process. // Rewrite outgoing handles if we have a handle to the destination process.
if (remote_process_handle_.is_valid()) { if (remote_process_handle != base::kNullProcessHandle) {
std::vector<ScopedPlatformHandle> handles = message->TakeHandles(); std::vector<ScopedPlatformHandle> handles = message->TakeHandles();
if (!Channel::Message::RewriteHandles(base::GetCurrentProcessHandle(), if (!Channel::Message::RewriteHandles(base::GetCurrentProcessHandle(),
remote_process_handle_.get(), remote_process_handle, &handles)) {
&handles)) {
DLOG(ERROR) << "Failed to duplicate one or more outgoing handles."; DLOG(ERROR) << "Failed to duplicate one or more outgoing handles.";
} }
message->SetHandles(std::move(handles)); message->SetHandles(std::move(handles));
...@@ -851,11 +868,16 @@ void NodeChannel::WriteChannelMessage(Channel::MessagePtr message) { ...@@ -851,11 +868,16 @@ void NodeChannel::WriteChannelMessage(Channel::MessagePtr message) {
if (message->has_mach_ports()) { if (message->has_mach_ports()) {
MachPortRelay* relay = delegate_->GetMachPortRelay(); MachPortRelay* relay = delegate_->GetMachPortRelay();
if (relay) { if (relay) {
base::AutoLock lock(remote_process_handle_lock_); base::ProcessHandle remote_process_handle;
DCHECK(remote_process_handle_.is_valid()); {
base::AutoLock lock(remote_process_handle_lock_);
// Expect that the receiving node is a known process.
DCHECK(remote_process_handle_ != base::kNullProcessHandle);
remote_process_handle = remote_process_handle_;
}
{ {
base::AutoLock lock(pending_mach_messages_lock_); base::AutoLock lock(pending_mach_messages_lock_);
if (relay->port_provider()->TaskForPid(remote_process_handle_.get()) == if (relay->port_provider()->TaskForPid(remote_process_handle) ==
MACH_PORT_NULL) { MACH_PORT_NULL) {
// It is also possible for TaskForPid() to return MACH_PORT_NULL when // It is also possible for TaskForPid() to return MACH_PORT_NULL when
// the process has started, then died. In that case, the queued // the process has started, then died. In that case, the queued
...@@ -866,7 +888,7 @@ void NodeChannel::WriteChannelMessage(Channel::MessagePtr message) { ...@@ -866,7 +888,7 @@ void NodeChannel::WriteChannelMessage(Channel::MessagePtr message) {
} }
} }
relay->SendPortsToProcess(message.get(), remote_process_handle_.get()); relay->SendPortsToProcess(message.get(), remote_process_handle);
} }
} }
#endif #endif
......
...@@ -21,7 +21,6 @@ ...@@ -21,7 +21,6 @@
#include "mojo/edk/embedder/scoped_platform_handle.h" #include "mojo/edk/embedder/scoped_platform_handle.h"
#include "mojo/edk/system/channel.h" #include "mojo/edk/system/channel.h"
#include "mojo/edk/system/ports/name.h" #include "mojo/edk/system/ports/name.h"
#include "mojo/edk/system/scoped_process_handle.h"
#if defined(OS_MACOSX) && !defined(OS_IOS) #if defined(OS_MACOSX) && !defined(OS_IOS)
#include "mojo/edk/system/mach_port_relay.h" #include "mojo/edk/system/mach_port_relay.h"
...@@ -116,9 +115,12 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>, ...@@ -116,9 +115,12 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>,
// Invokes the bad message callback for this channel, if any. // Invokes the bad message callback for this channel, if any.
void NotifyBadMessage(const std::string& error); void NotifyBadMessage(const std::string& error);
void SetRemoteProcessHandle(ScopedProcessHandle process_handle); // Note: On Windows, we take ownership of the remote process handle.
void SetRemoteProcessHandle(base::ProcessHandle process_handle);
bool HasRemoteProcessHandle(); bool HasRemoteProcessHandle();
ScopedProcessHandle CloneRemoteProcessHandle(); // Note: The returned |ProcessHandle| is owned by the caller and should be
// freed if necessary.
base::ProcessHandle CopyRemoteProcessHandle();
// Used for context in Delegate calls (via |from_node| arguments.) // Used for context in Delegate calls (via |from_node| arguments.)
void SetRemoteNodeName(const ports::NodeName& name); void SetRemoteNodeName(const ports::NodeName& name);
...@@ -131,7 +133,7 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>, ...@@ -131,7 +133,7 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>,
const ports::NodeName& token, const ports::NodeName& token,
const ports::PortName& port_name); const ports::PortName& port_name);
void AddBrokerClient(const ports::NodeName& client_name, void AddBrokerClient(const ports::NodeName& client_name,
ScopedProcessHandle process_handle); base::ProcessHandle process_handle);
void BrokerClientAdded(const ports::NodeName& client_name, void BrokerClientAdded(const ports::NodeName& client_name,
ScopedPlatformHandle broker_channel); ScopedPlatformHandle broker_channel);
void AcceptBrokerClient(const ports::NodeName& broker_name, void AcceptBrokerClient(const ports::NodeName& broker_name,
...@@ -198,7 +200,10 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>, ...@@ -198,7 +200,10 @@ class NodeChannel : public base::RefCountedThreadSafe<NodeChannel>,
ports::NodeName remote_node_name_; ports::NodeName remote_node_name_;
base::Lock remote_process_handle_lock_; base::Lock remote_process_handle_lock_;
ScopedProcessHandle remote_process_handle_; base::ProcessHandle remote_process_handle_ = base::kNullProcessHandle;
#if defined(OS_WIN)
ScopedPlatformHandle scoped_remote_process_handle_;
#endif
#if defined(OS_MACOSX) && !defined(OS_IOS) #if defined(OS_MACOSX) && !defined(OS_IOS)
base::Lock pending_mach_messages_lock_; base::Lock pending_mach_messages_lock_;
......
...@@ -185,12 +185,22 @@ void NodeController::SendBrokerClientInvitation( ...@@ -185,12 +185,22 @@ void NodeController::SendBrokerClientInvitation(
} }
} }
ScopedProcessHandle scoped_target_process = #if defined(OS_WIN)
ScopedProcessHandle::CloneFrom(target_process); // On Windows, we need to duplicate the process handle because we have no
// control over its lifetime and it may become invalid by the time the posted
// task runs.
HANDLE dup_handle = INVALID_HANDLE_VALUE;
BOOL ok = ::DuplicateHandle(base::GetCurrentProcessHandle(), target_process,
base::GetCurrentProcessHandle(), &dup_handle, 0,
FALSE, DUPLICATE_SAME_ACCESS);
DPCHECK(ok);
target_process = dup_handle;
#endif
io_task_runner_->PostTask( io_task_runner_->PostTask(
FROM_HERE, FROM_HERE,
base::BindOnce(&NodeController::SendBrokerClientInvitationOnIOThread, base::BindOnce(&NodeController::SendBrokerClientInvitationOnIOThread,
base::Unretained(this), std::move(scoped_target_process), base::Unretained(this), target_process,
std::move(connection_params), temporary_node_name, std::move(connection_params), temporary_node_name,
process_error_callback)); process_error_callback));
} }
...@@ -322,7 +332,7 @@ void NodeController::NotifyBadMessageFrom(const ports::NodeName& source_node, ...@@ -322,7 +332,7 @@ void NodeController::NotifyBadMessageFrom(const ports::NodeName& source_node,
} }
void NodeController::SendBrokerClientInvitationOnIOThread( void NodeController::SendBrokerClientInvitationOnIOThread(
ScopedProcessHandle target_process, base::ProcessHandle target_process,
ConnectionParams connection_params, ConnectionParams connection_params,
ports::NodeName temporary_node_name, ports::NodeName temporary_node_name,
const ProcessErrorCallback& process_error_callback) { const ProcessErrorCallback& process_error_callback) {
...@@ -332,9 +342,9 @@ void NodeController::SendBrokerClientInvitationOnIOThread( ...@@ -332,9 +342,9 @@ void NodeController::SendBrokerClientInvitationOnIOThread(
PlatformChannelPair node_channel; PlatformChannelPair node_channel;
ScopedPlatformHandle server_handle = node_channel.PassServerHandle(); ScopedPlatformHandle server_handle = node_channel.PassServerHandle();
// BrokerHost owns itself. // BrokerHost owns itself.
BrokerHost* broker_host = new BrokerHost( BrokerHost* broker_host =
target_process.get(), connection_params.TakeChannelHandle(), new BrokerHost(target_process, connection_params.TakeChannelHandle(),
process_error_callback); process_error_callback);
bool channel_ok = broker_host->SendChannel(node_channel.PassClientHandle()); bool channel_ok = broker_host->SendChannel(node_channel.PassClientHandle());
#if defined(OS_WIN) #if defined(OS_WIN)
...@@ -368,7 +378,7 @@ void NodeController::SendBrokerClientInvitationOnIOThread( ...@@ -368,7 +378,7 @@ void NodeController::SendBrokerClientInvitationOnIOThread(
pending_invitations_.insert(std::make_pair(temporary_node_name, channel)); pending_invitations_.insert(std::make_pair(temporary_node_name, channel));
channel->SetRemoteNodeName(temporary_node_name); channel->SetRemoteNodeName(temporary_node_name);
channel->SetRemoteProcessHandle(std::move(target_process)); channel->SetRemoteProcessHandle(target_process);
channel->Start(); channel->Start();
channel->AcceptInvitee(name_, temporary_node_name); channel->AcceptInvitee(name_, temporary_node_name);
...@@ -786,7 +796,7 @@ void NodeController::OnAcceptInvitation(const ports::NodeName& from_node, ...@@ -786,7 +796,7 @@ void NodeController::OnAcceptInvitation(const ports::NodeName& from_node,
scoped_refptr<NodeChannel> broker = GetBrokerChannel(); scoped_refptr<NodeChannel> broker = GetBrokerChannel();
if (broker) { if (broker) {
// Inform the broker of this new client. // Inform the broker of this new client.
broker->AddBrokerClient(invitee_name, channel->CloneRemoteProcessHandle()); broker->AddBrokerClient(invitee_name, channel->CopyRemoteProcessHandle());
} else { } else {
// If we have no broker, either we need to wait for one, or we *are* the // If we have no broker, either we need to wait for one, or we *are* the
// broker. // broker.
...@@ -810,8 +820,11 @@ void NodeController::OnAcceptInvitation(const ports::NodeName& from_node, ...@@ -810,8 +820,11 @@ void NodeController::OnAcceptInvitation(const ports::NodeName& from_node,
void NodeController::OnAddBrokerClient(const ports::NodeName& from_node, void NodeController::OnAddBrokerClient(const ports::NodeName& from_node,
const ports::NodeName& client_name, const ports::NodeName& client_name,
base::ProcessHandle process_handle) { base::ProcessHandle process_handle) {
ScopedProcessHandle scoped_process_handle(process_handle); #if defined(OS_WIN)
// Scoped handle to avoid leaks on error.
ScopedPlatformHandle scoped_process_handle =
ScopedPlatformHandle(PlatformHandle(process_handle));
#endif
scoped_refptr<NodeChannel> sender = GetPeerChannel(from_node); scoped_refptr<NodeChannel> sender = GetPeerChannel(from_node);
if (!sender) { if (!sender) {
DLOG(ERROR) << "Ignoring AddBrokerClient from unknown sender."; DLOG(ERROR) << "Ignoring AddBrokerClient from unknown sender.";
...@@ -838,8 +851,10 @@ void NodeController::OnAddBrokerClient(const ports::NodeName& from_node, ...@@ -838,8 +851,10 @@ void NodeController::OnAddBrokerClient(const ports::NodeName& from_node,
DLOG(ERROR) << "Broker rejecting client with invalid process handle."; DLOG(ERROR) << "Broker rejecting client with invalid process handle.";
return; return;
} }
client->SetRemoteProcessHandle(scoped_process_handle.release().handle);
#else
client->SetRemoteProcessHandle(process_handle);
#endif #endif
client->SetRemoteProcessHandle(std::move(scoped_process_handle));
AddPeer(client_name, client, true /* start_channel */); AddPeer(client_name, client, true /* start_channel */);
...@@ -928,7 +943,7 @@ void NodeController::OnAcceptBrokerClient(const ports::NodeName& from_node, ...@@ -928,7 +943,7 @@ void NodeController::OnAcceptBrokerClient(const ports::NodeName& from_node,
auto it = pending_invitations_.find(invitee_name); auto it = pending_invitations_.find(invitee_name);
DCHECK(it != pending_invitations_.end()); DCHECK(it != pending_invitations_.end());
broker->AddBrokerClient(invitee_name, broker->AddBrokerClient(invitee_name,
it->second->CloneRemoteProcessHandle()); it->second->CopyRemoteProcessHandle());
pending_broker_clients.pop(); pending_broker_clients.pop();
} }
......
...@@ -28,7 +28,6 @@ ...@@ -28,7 +28,6 @@
#include "mojo/edk/system/ports/name.h" #include "mojo/edk/system/ports/name.h"
#include "mojo/edk/system/ports/node.h" #include "mojo/edk/system/ports/node.h"
#include "mojo/edk/system/ports/node_delegate.h" #include "mojo/edk/system/ports/node_delegate.h"
#include "mojo/edk/system/scoped_process_handle.h"
#include "mojo/edk/system/system_impl_export.h" #include "mojo/edk/system/system_impl_export.h"
namespace base { namespace base {
...@@ -160,7 +159,7 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate, ...@@ -160,7 +159,7 @@ class MOJO_SYSTEM_IMPL_EXPORT NodeController : public ports::NodeDelegate,
}; };
void SendBrokerClientInvitationOnIOThread( void SendBrokerClientInvitationOnIOThread(
ScopedProcessHandle target_process, base::ProcessHandle target_process,
ConnectionParams connection_params, ConnectionParams connection_params,
ports::NodeName token, ports::NodeName token,
const ProcessErrorCallback& process_error_callback); const ProcessErrorCallback& process_error_callback);
......
// Copyright 2018 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 "mojo/edk/system/scoped_process_handle.h"
#include "build/build_config.h"
#if defined(OS_WIN)
#include <windows.h>
#endif
namespace mojo {
namespace edk {
ScopedProcessHandle::ScopedProcessHandle() = default;
ScopedProcessHandle::ScopedProcessHandle(base::ProcessHandle handle)
: handle_(handle) {}
ScopedProcessHandle::ScopedProcessHandle(ScopedProcessHandle&&) = default;
ScopedProcessHandle::~ScopedProcessHandle() = default;
// static
ScopedProcessHandle ScopedProcessHandle::CloneFrom(base::ProcessHandle handle) {
#if defined(OS_WIN)
BOOL ok = ::DuplicateHandle(base::GetCurrentProcessHandle(), handle,
base::GetCurrentProcessHandle(), &handle, 0,
FALSE, DUPLICATE_SAME_ACCESS);
DCHECK(ok);
#endif
return ScopedProcessHandle(handle);
}
ScopedProcessHandle& ScopedProcessHandle::operator=(ScopedProcessHandle&&) =
default;
ScopedProcessHandle ScopedProcessHandle::Clone() const {
DCHECK(is_valid());
return CloneFrom(get());
}
} // namespace edk
} // namespace mojo
// Copyright 2018 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 MOJO_EDK_SYSTEM_SCOPED_PROCESS_HANDLE_H_
#define MOJO_EDK_SYSTEM_SCOPED_PROCESS_HANDLE_H_
#include "base/macros.h"
#include "base/process/process_handle.h"
#include "build/build_config.h"
#if defined(OS_WIN)
#include "base/win/scoped_handle.h"
#endif
namespace mojo {
namespace edk {
// Wraps a |base::ProcessHandle| with additional scoped lifetime semantics on
// applicable platforms. For platforms where process handles aren't ownable
// references, this is just a wrapper around |base::ProcessHandle|.
//
// This essentially exists to support passing around process handles internally
// in a generic way while also supporting Windows process handle ownership
// semantics.
class ScopedProcessHandle {
public:
ScopedProcessHandle();
// Assumes ownership of |handle|.
explicit ScopedProcessHandle(base::ProcessHandle handle);
ScopedProcessHandle(ScopedProcessHandle&&);
~ScopedProcessHandle();
// Creates a new ScopedProcessHandle from a clone of |handle|.
static ScopedProcessHandle CloneFrom(base::ProcessHandle handle);
ScopedProcessHandle& operator=(ScopedProcessHandle&&);
bool is_valid() const {
#if defined(OS_WIN)
return handle_.IsValid();
#else
return handle_ != base::kNullProcessHandle;
#endif
}
base::ProcessHandle get() const {
#if defined(OS_WIN)
return handle_.Get();
#else
return handle_;
#endif
}
base::ProcessHandle release() {
#if defined(OS_WIN)
return handle_.Take();
#else
return handle_;
#endif
}
ScopedProcessHandle Clone() const;
private:
#if defined(OS_WIN)
base::win::ScopedHandle handle_;
#else
base::ProcessHandle handle_ = base::kNullProcessHandle;
#endif
DISALLOW_COPY_AND_ASSIGN(ScopedProcessHandle);
};
} // namespace edk
} // namespace mojo
#endif // MOJO_EDK_SYSTEM_SCOPED_PROCESS_HANDLE_H_
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment