Add some bullet proofing to ipc_channel_posix.
Specifically make sure you can't connect after creating a bad channel.
BUG=NONE
TEST=Build and run tests
Review URL: http://codereview.chromium.org/6596093
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@76633 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: 6aad23f982580fadbe6f180aa46191102139b01e
diff --git a/ipc/ipc_channel_posix.cc b/ipc/ipc_channel_posix.cc
index a6b1cb4..5e09666 100644
--- a/ipc/ipc_channel_posix.cc
+++ b/ipc/ipc_channel_posix.cc
@@ -373,13 +373,14 @@
// 4a) Client side: Pull the pipe out of the GlobalDescriptors set.
// 4b) Server side: create the pipe.
+ int local_pipe = -1;
if (channel_handle.socket.fd != -1) {
// Case 1 from comment above.
- pipe_ = channel_handle.socket.fd;
+ local_pipe = channel_handle.socket.fd;
#if defined(IPC_USES_READWRITE)
// Test the socket passed into us to make sure it is nonblocking.
// We don't want to call read/write on a blocking socket.
- int value = fcntl(pipe_, F_GETFL);
+ int value = fcntl(local_pipe, F_GETFL);
if (value == -1) {
PLOG(ERROR) << "fcntl(F_GETFL) " << pipe_name_;
return false;
@@ -392,25 +393,25 @@
} else if (mode_ & MODE_NAMED_FLAG) {
// Case 2 from comment above.
if (mode_ & MODE_SERVER_FLAG) {
- if (!CreateServerUnixDomainSocket(pipe_name_, &pipe_)) {
+ if (!CreateServerUnixDomainSocket(pipe_name_, &local_pipe)) {
return false;
}
must_unlink_ = true;
} else if (mode_ & MODE_CLIENT_FLAG) {
- if (!CreateClientUnixDomainSocket(pipe_name_, &pipe_)) {
+ if (!CreateClientUnixDomainSocket(pipe_name_, &local_pipe)) {
return false;
}
} else {
- NOTREACHED();
+ LOG(ERROR) << "Bad mode: " << mode_;
return false;
}
} else {
- pipe_ = PipeMap::GetInstance()->Lookup(pipe_name_);
+ local_pipe = PipeMap::GetInstance()->Lookup(pipe_name_);
if (mode_ & MODE_CLIENT_FLAG) {
- if (pipe_ != -1) {
+ if (local_pipe != -1) {
// Case 3 from comment above.
// We only allow one connection.
- pipe_ = HANDLE_EINTR(dup(pipe_));
+ local_pipe = HANDLE_EINTR(dup(local_pipe));
PipeMap::GetInstance()->RemoveAndClose(pipe_name_);
} else {
// Case 4a from comment above.
@@ -425,28 +426,24 @@
}
used_initial_channel = true;
- pipe_ = base::GlobalDescriptors::GetInstance()->Get(kPrimaryIPCChannel);
+ local_pipe =
+ base::GlobalDescriptors::GetInstance()->Get(kPrimaryIPCChannel);
}
} else if (mode_ & MODE_SERVER_FLAG) {
// Case 4b from comment above.
- if (pipe_ != -1) {
+ if (local_pipe != -1) {
LOG(ERROR) << "Server already exists for " << pipe_name_;
return false;
}
- if (!SocketPair(&pipe_, &client_pipe_))
+ if (!SocketPair(&local_pipe, &client_pipe_))
return false;
PipeMap::GetInstance()->Insert(pipe_name_, client_pipe_);
} else {
- NOTREACHED();
+ LOG(ERROR) << "Bad mode: " << mode_;
return false;
}
}
- if ((mode_ & MODE_SERVER_FLAG) && (mode_ & MODE_NAMED_FLAG)) {
- server_listen_pipe_ = pipe_;
- pipe_ = -1;
- }
-
#if defined(IPC_USES_READWRITE)
// Create a dedicated socketpair() for exchanging file descriptors.
// See comments for IPC_USES_READWRITE for details.
@@ -457,12 +454,18 @@
}
#endif // IPC_USES_READWRITE
+ if ((mode_ & MODE_SERVER_FLAG) && (mode_ & MODE_NAMED_FLAG)) {
+ server_listen_pipe_ = local_pipe;
+ local_pipe = -1;
+ }
+
+ pipe_ = local_pipe;
return true;
}
bool Channel::ChannelImpl::Connect() {
if (server_listen_pipe_ == -1 && pipe_ == -1) {
- DLOG(INFO) << "Must call create on a channel before calling connect";
+ DLOG(INFO) << "Channel creation failed: " << pipe_name_;
return false;
}
diff --git a/ipc/ipc_channel_posix.h b/ipc/ipc_channel_posix.h
index cee613a..b1c4c3b 100644
--- a/ipc/ipc_channel_posix.h
+++ b/ipc/ipc_channel_posix.h
@@ -140,7 +140,7 @@
ScopedRunnableMethodFactory<ChannelImpl> factory_;
- DISALLOW_COPY_AND_ASSIGN(ChannelImpl);
+ DISALLOW_IMPLICIT_CONSTRUCTORS(ChannelImpl);
};
// The maximum length of the name of a pipe for MODE_NAMED_SERVER or
diff --git a/ipc/ipc_channel_posix_unittest.cc b/ipc/ipc_channel_posix_unittest.cc
index 459aafb..dcd924e 100644
--- a/ipc/ipc_channel_posix_unittest.cc
+++ b/ipc/ipc_channel_posix_unittest.cc
@@ -318,6 +318,25 @@
ASSERT_FALSE(channel.HasAcceptedConnection());
}
+TEST_F(IPCChannelPosixTest, DoubleServer) {
+ // Test setting up two servers with the same name.
+ IPCChannelPosixTestListener listener(false);
+ IPCChannelPosixTestListener listener2(false);
+ IPC::ChannelHandle chan_handle(kConnectionSocketTestName);
+ IPC::Channel channel(chan_handle, IPC::Channel::MODE_SERVER, &listener);
+ IPC::Channel channel2(chan_handle, IPC::Channel::MODE_SERVER, &listener2);
+ ASSERT_TRUE(channel.Connect());
+ ASSERT_FALSE(channel2.Connect());
+}
+
+TEST_F(IPCChannelPosixTest, BadMode) {
+ // Test setting up two servers with a bad mode.
+ IPCChannelPosixTestListener listener(false);
+ IPC::ChannelHandle chan_handle(kConnectionSocketTestName);
+ IPC::Channel channel(chan_handle, IPC::Channel::MODE_NONE, &listener);
+ ASSERT_FALSE(channel.Connect());
+}
+
// A long running process that connects to us
MULTIPROCESS_TEST_MAIN(IPCChannelPosixTestConnectionProc) {
MessageLoopForIO message_loop;