diff options
author | Yabin Cui <yabinc@google.com> | 2015-09-29 12:25:33 -0700 |
---|---|---|
committer | Yabin Cui <yabinc@google.com> | 2015-09-30 15:03:26 -0700 |
commit | aa77e22d7384a97757e2d2194f4e265f7ad3b71a (patch) | |
tree | 9fbcc3dbd66c1cdae1a50656b0e15e831eb6fc99 /adb | |
parent | d490db9824cdb4974da22ba80399f39a8b44ec08 (diff) | |
download | system_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.cpp | 5 | ||||
-rw-r--r-- | adb/fdevent.cpp | 11 | ||||
-rw-r--r-- | adb/socket_test.cpp | 87 |
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__) |