summaryrefslogtreecommitdiffstats
path: root/adb
diff options
context:
space:
mode:
authorYabin Cui <yabinc@google.com>2015-09-29 12:25:33 -0700
committerYabin Cui <yabinc@google.com>2015-09-30 15:03:26 -0700
commitaa77e22d7384a97757e2d2194f4e265f7ad3b71a (patch)
tree9fbcc3dbd66c1cdae1a50656b0e15e831eb6fc99 /adb
parentd490db9824cdb4974da22ba80399f39a8b44ec08 (diff)
downloadsystem_core-aa77e22d7384a97757e2d2194f4e265f7ad3b71a.tar.gz
system_core-aa77e22d7384a97757e2d2194f4e265f7ad3b71a.tar.bz2
system_core-aa77e22d7384a97757e2d2194f4e265f7ad3b71a.zip
adb: detect sockets in CLOSE_WAIT state to prevent socket leak on linux.
It is possible that the adb server on host has many sockets in CLOSE_WAIT state. To prevent socket leak, always enable POLLRDHUP in fdevent.cpp to detect sockets in CLOSE_WAIT state. Update LocalSocketTest unit tests: Change half_close_with_packet to read_from_closing_socket, as reading from a SHUT_WR socket is not needed in adb. Change close_with_no_events_installed to close_socket_in_CLOSE_WAIT_state, as the latter is more close to the real situation in use. Bug: 23314034 Change-Id: Ice4f4036624e5584eab6ba5848e7f169c92f037f
Diffstat (limited to 'adb')
-rw-r--r--adb/adb.cpp5
-rw-r--r--adb/fdevent.cpp11
-rw-r--r--adb/socket_test.cpp87
3 files changed, 64 insertions, 39 deletions
diff --git a/adb/adb.cpp b/adb/adb.cpp
index 0e1859cdd..1c1683e73 100644
--- a/adb/adb.cpp
+++ b/adb/adb.cpp
@@ -395,6 +395,11 @@ void handle_packet(apacket *p, atransport *t)
D("Invalid A_OKAY(%d,%d), expected A_OKAY(%d,%d) on transport %s",
p->msg.arg0, p->msg.arg1, s->peer->id, p->msg.arg1, t->serial);
}
+ } else {
+ // When receiving A_OKAY from device for A_OPEN request, the host server may
+ // have closed the local socket because of client disconnection. Then we need
+ // to send A_CLSE back to device to close the service on device.
+ send_close(p->msg.arg1, p->msg.arg0, t);
}
}
break;
diff --git a/adb/fdevent.cpp b/adb/fdevent.cpp
index 2de9386fd..2ffd908d4 100644
--- a/adb/fdevent.cpp
+++ b/adb/fdevent.cpp
@@ -58,6 +58,12 @@ struct PollNode {
PollNode(fdevent* fde) : fde(fde) {
memset(&pollfd, 0, sizeof(pollfd));
pollfd.fd = fde->fd;
+
+#if defined(__linux__)
+ // Always enable POLLRDHUP, so the host server can take action when some clients disconnect.
+ // Then we can avoid leaving many sockets in CLOSE_WAIT state. See http://b/23314034.
+ pollfd.events = POLLRDHUP;
+#endif
}
};
@@ -234,6 +240,11 @@ static void fdevent_process() {
// be detected at that point.
events |= FDE_READ | FDE_ERROR;
}
+#if defined(__linux__)
+ if (pollfd.revents & POLLRDHUP) {
+ events |= FDE_READ | FDE_ERROR;
+ }
+#endif
if (events != 0) {
auto it = g_poll_node_map.find(pollfd.fd);
CHECK(it != g_poll_node_map.end());
diff --git a/adb/socket_test.cpp b/adb/socket_test.cpp
index 8f395d1e3..03cab64a5 100644
--- a/adb/socket_test.cpp
+++ b/adb/socket_test.cpp
@@ -168,7 +168,7 @@ static void CloseWithPacketThreadFunc(CloseWithPacketArg* arg) {
// The socket is closing but having some packets, so it is not closed. Then
// some write error happens in the socket's file handler, e.g., the file
// handler is closed.
-TEST_F(LocalSocketTest, close_with_packet) {
+TEST_F(LocalSocketTest, close_socket_with_packet) {
int socket_fd[2];
ASSERT_EQ(0, adb_socketpair(socket_fd));
int cause_close_fd[2];
@@ -193,13 +193,8 @@ TEST_F(LocalSocketTest, close_with_packet) {
ASSERT_EQ(0, pthread_join(thread, nullptr));
}
-#undef shutdown
-
// This test checks if we can read packets from a closing local socket.
-// The socket's file handler may be non readable if the other side has
-// called shutdown(SHUT_WR). But we should always write packets
-// successfully to the other side.
-TEST_F(LocalSocketTest, half_close_with_packet) {
+TEST_F(LocalSocketTest, read_from_closing_socket) {
int socket_fd[2];
ASSERT_EQ(0, adb_socketpair(socket_fd));
int cause_close_fd[2];
@@ -217,7 +212,6 @@ TEST_F(LocalSocketTest, half_close_with_packet) {
ASSERT_EQ(0, adb_close(cause_close_fd[0]));
sleep(1);
ASSERT_EQ(2u, fdevent_installed_count());
- ASSERT_EQ(0, shutdown(socket_fd[0], SHUT_WR));
// Verify if we can read successfully.
std::vector<char> buf(arg.bytes_written);
@@ -260,41 +254,56 @@ TEST_F(LocalSocketTest, write_error_when_having_packets) {
ASSERT_EQ(0, pthread_join(thread, nullptr));
}
-struct CloseNoEventsArg {
- int socket_fd;
-};
+#if defined(__linux__)
-static void CloseNoEventsThreadFunc(CloseNoEventsArg* arg) {
- asocket* s = create_local_socket(arg->socket_fd);
- ASSERT_TRUE(s != nullptr);
-
- InstallDummySocket();
- fdevent_loop();
+static void ClientThreadFunc() {
+ std::string error;
+ int fd = network_loopback_client(5038, SOCK_STREAM, &error);
+ ASSERT_GE(fd, 0) << error;
+ sleep(2);
+ ASSERT_EQ(0, adb_close(fd));
}
-// This test checks when a local socket doesn't enable FDE_READ/FDE_WRITE/FDE_ERROR, it
-// can still be closed when some error happens on its file handler.
-// This test successes on linux but fails on mac because of different implementation of
-// poll(). I think the function tested here is useful to make adb server more stable on
-// linux.
-TEST_F(LocalSocketTest, close_with_no_events_installed) {
- int socket_fd[2];
- ASSERT_EQ(0, adb_socketpair(socket_fd));
+struct CloseRdHupSocketArg {
+ int socket_fd;
+};
- CloseNoEventsArg arg;
- arg.socket_fd = socket_fd[1];
- pthread_t thread;
- ASSERT_EQ(0, pthread_create(&thread, nullptr,
- reinterpret_cast<void* (*)(void*)>(CloseNoEventsThreadFunc),
- &arg));
- // Wait until the fdevent_loop() starts.
- sleep(1);
- ASSERT_EQ(2u, fdevent_installed_count());
- ASSERT_EQ(0, adb_close(socket_fd[0]));
+static void CloseRdHupSocketThreadFunc(CloseRdHupSocketArg* arg) {
+ asocket* s = create_local_socket(arg->socket_fd);
+ ASSERT_TRUE(s != nullptr);
- // Wait until the socket is closed.
- sleep(1);
+ InstallDummySocket();
+ fdevent_loop();
+}
- ASSERT_EQ(0, pthread_kill(thread, SIGUSR1));
- ASSERT_EQ(0, pthread_join(thread, nullptr));
+// This test checks if we can close sockets in CLOSE_WAIT state.
+TEST_F(LocalSocketTest, close_socket_in_CLOSE_WAIT_state) {
+ std::string error;
+ int listen_fd = network_inaddr_any_server(5038, SOCK_STREAM, &error);
+ ASSERT_GE(listen_fd, 0);
+ pthread_t client_thread;
+ ASSERT_EQ(0, pthread_create(&client_thread, nullptr,
+ reinterpret_cast<void* (*)(void*)>(ClientThreadFunc), nullptr));
+
+ struct sockaddr addr;
+ socklen_t alen;
+ alen = sizeof(addr);
+ int accept_fd = adb_socket_accept(listen_fd, &addr, &alen);
+ ASSERT_GE(accept_fd, 0);
+ CloseRdHupSocketArg arg;
+ arg.socket_fd = accept_fd;
+ pthread_t thread;
+ ASSERT_EQ(0, pthread_create(&thread, nullptr,
+ reinterpret_cast<void* (*)(void*)>(CloseRdHupSocketThreadFunc),
+ &arg));
+ // Wait until the fdevent_loop() starts.
+ sleep(1);
+ ASSERT_EQ(2u, fdevent_installed_count());
+ // Wait until the client closes its socket.
+ ASSERT_EQ(0, pthread_join(client_thread, nullptr));
+ sleep(2);
+ ASSERT_EQ(0, pthread_kill(thread, SIGUSR1));
+ ASSERT_EQ(0, pthread_join(thread, nullptr));
}
+
+#endif // defined(__linux__)