Bug 1838906 - Part 3: Clean up ConnectNamedPipe logic, r=ipc-reviewers,jld

This call is no longer required after the other changes in part 1 and 2,
and adds substantial complexity to the type. This just removes all of
the ConnectNamedPipe handling logic from ipc_channel_win now that it is
unnecessary.

Differential Revision: https://phabricator.services.mozilla.com/D181803
This commit is contained in:
Nika Layzell 2023-06-27 19:59:47 +00:00
parent 4a72a442c0
commit 1a1cb945e4
2 changed files with 14 additions and 134 deletions

View File

@ -54,8 +54,6 @@ Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode,
return;
}
shared_secret_ = 0;
waiting_for_shared_secret_ = false;
pipe_ = pipe.release();
EnqueueHelloMessage();
}
@ -99,11 +97,6 @@ void Channel::ChannelImpl::Close() {
void Channel::ChannelImpl::CloseLocked() {
chan_cap_.NoteExclusiveAccess();
if (connect_timeout_) {
connect_timeout_->Cancel();
connect_timeout_ = nullptr;
}
// If we still have pending I/O, cancel it. The references inside
// `input_state_` and `output_state_` will keep the buffers alive until they
// complete.
@ -177,13 +170,8 @@ bool Channel::ChannelImpl::EnqueueHelloMessage() {
auto m = mozilla::MakeUnique<Message>(MSG_ROUTING_NONE, HELLO_MESSAGE_TYPE);
// If we're waiting for our shared secret from the other end's hello message
// then don't give the game away by sending it in ours.
int32_t secret = waiting_for_shared_secret_ ? 0 : shared_secret_;
// Also, don't send if the value is zero (for IPC backwards compatability).
if (!m->WriteInt(GetCurrentProcessId()) ||
(secret && !m->WriteUInt32(secret))) {
if (!m->WriteInt(GetCurrentProcessId())) {
CloseHandle(pipe_);
pipe_ = INVALID_HANDLE_VALUE;
return false;
@ -201,88 +189,21 @@ bool Channel::ChannelImpl::Connect() {
if (pipe_ == INVALID_HANDLE_VALUE) return false;
MessageLoopForIO::current()->RegisterIOHandler(pipe_, this);
// Check to see if there is a client connected to our pipe...
if (mode_ == MODE_SERVER) {
DCHECK(!input_state_.is_pending);
if (!ProcessConnection()) {
return false;
}
} else {
waiting_connect_ = false;
}
if (!input_state_.is_pending) {
// Complete setup asynchronously. By not setting input_state_.is_pending
// to `this`, we indicate to OnIOCompleted that this is the special
// initialization signal, while keeping a reference through the
// `RunnableMethod`.
IOThread().Dispatch(
mozilla::NewRunnableMethod<MessageLoopForIO::IOContext*, DWORD, DWORD>(
"ContinueConnect", this, &ChannelImpl::OnIOCompleted,
&input_state_.context, 0, 0));
}
if (!waiting_connect_) {
DCHECK(!output_state_.is_pending);
ProcessOutgoingMessages(NULL, 0, false);
}
return true;
}
bool Channel::ChannelImpl::ProcessConnection() {
chan_cap_.NoteExclusiveAccess();
waiting_connect_ = false;
DCHECK(!input_state_.is_pending);
// Do we have a client connected to our pipe?
if (INVALID_HANDLE_VALUE == pipe_) return false;
BOOL ok = ConnectNamedPipe(pipe_, &input_state_.context.overlapped);
DWORD err = GetLastError();
if (ok) {
// Uhm, the API documentation says that this function should never
// return success when used in overlapped mode.
NOTREACHED();
return false;
}
switch (err) {
case ERROR_IO_PENDING: {
static bool kExtendedTimeout =
#ifdef DEBUG
true;
#else
!!PR_GetEnv("MOZ_RUN_GTEST");
#endif
input_state_.is_pending = this;
NS_NewTimerWithCallback(
getter_AddRefs(connect_timeout_),
[self = RefPtr{this}](nsITimer* timer) {
CHROMIUM_LOG(ERROR) << "ConnectNamedPipe timed out!";
self->IOThread().AssertOnCurrentThread();
self->Close();
self->listener_->OnChannelError();
},
kExtendedTimeout ? 60000 : 10000, nsITimer::TYPE_ONE_SHOT,
"ChannelImpl::ProcessConnection", IOThread().GetEventTarget());
} break;
case ERROR_PIPE_CONNECTED:
waiting_connect_ = false;
if (connect_timeout_) {
connect_timeout_->Cancel();
connect_timeout_ = nullptr;
}
break;
case ERROR_NO_DATA:
// The pipe is being closed.
return false;
default:
NOTREACHED();
return false;
}
// Complete setup asynchronously. By not setting input_state_.is_pending
// to `this`, we indicate to OnIOCompleted that this is the special
// initialization signal, while keeping a reference through the
// `RunnableMethod`.
IOThread().Dispatch(
mozilla::NewRunnableMethod<MessageLoopForIO::IOContext*, DWORD, DWORD>(
"ContinueConnect", this, &ChannelImpl::OnIOCompleted,
&input_state_.context, 0, 0));
DCHECK(!output_state_.is_pending);
ProcessOutgoingMessages(NULL, 0, false);
return true;
}
@ -427,13 +348,6 @@ bool Channel::ChannelImpl::ProcessIncomingMessages(
MessageIterator it = MessageIterator(m);
int32_t other_pid = it.NextInt();
SetOtherPid(other_pid);
if (waiting_for_shared_secret_ && (it.NextInt() != shared_secret_)) {
NOTREACHED();
// Something went wrong. Abort connection.
// NOTE: Caller will `Close()` and notify `OnChannelError`.
return false;
}
waiting_for_shared_secret_ = false;
listener_->OnChannelConnected(other_pid);
} else {
@ -552,26 +466,10 @@ void Channel::ChannelImpl::OnIOCompleted(MessageLoopForIO::IOContext* context,
bool ok;
if (context == &input_state_.context) {
was_pending = input_state_.is_pending.forget();
bool was_waiting_connect = waiting_connect_;
if (was_waiting_connect) {
mozilla::MutexAutoLock lock(SendMutex());
if (!ProcessConnection()) {
return;
}
// We may have some messages queued up to send...
if (!output_queue_.IsEmpty() && !output_state_.is_pending) {
ProcessOutgoingMessages(NULL, 0, false);
}
if (input_state_.is_pending) {
return;
}
// else, fall-through and look for incoming messages...
}
// we don't support recursion through OnMessageReceived yet!
DCHECK(!processing_incoming_);
processing_incoming_ = true;
ok = ProcessIncomingMessages(context, bytes_transfered,
was_pending && !was_waiting_connect);
ok = ProcessIncomingMessages(context, bytes_transfered, was_pending);
processing_incoming_ = false;
} else {
mozilla::MutexAutoLock lock(SendMutex());

View File

@ -83,7 +83,6 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
bool EnqueueHelloMessage() MOZ_REQUIRES(SendMutex(), IOThread());
void CloseLocked() MOZ_REQUIRES(SendMutex(), IOThread());
bool ProcessConnection() MOZ_REQUIRES(SendMutex(), IOThread());
bool ProcessIncomingMessages(MessageLoopForIO::IOContext* context,
DWORD bytes_read, bool was_pending)
MOZ_REQUIRES(IOThread());
@ -150,14 +149,7 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
// buffers of this message.
mozilla::UniquePtr<Message> incoming_message_ MOZ_GUARDED_BY(IOThread());
// Timer started when a MODE_SERVER channel begins waiting for a connection,
// and cancelled when the connection completes. Will produce an error if no
// connection occurs before the timeout.
nsCOMPtr<nsITimer> connect_timeout_ MOZ_GUARDED_BY(IOThread());
// Will be set to `true` until `Connect()` has been called, and, if in
// server-mode, the client has connected. The `input_state_` is used to wait
// for the client to connect in overlapped mode.
// Will be set to `true` until `Connect()` has been called.
bool waiting_connect_ MOZ_GUARDED_BY(chan_cap_) = true;
// This flag is set when processing incoming messages. It is used to
@ -169,16 +161,6 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
// record this when generating logs of IPC messages.
int32_t other_pid_ MOZ_GUARDED_BY(chan_cap_) = -1;
// This is a unique per-channel value used to authenticate the client end of
// a connection. If the value is non-zero, the client passes it in the hello
// and the host validates. (We don't send the zero value to preserve IPC
// compatibility with existing clients that don't validate the channel.)
int32_t shared_secret_ MOZ_GUARDED_BY(IOThread()) = 0;
// In server-mode, we wait for the channel at the other side of the pipe to
// send us back our shared secret, if we are using one.
bool waiting_for_shared_secret_ MOZ_GUARDED_BY(IOThread()) = false;
// Whether or not to accept handles from a remote process, and whether this
// process is the privileged side of a IPC::Channel which can transfer
// handles.