From 190624301755d366ccc464b2223eeeec5c7ccd32 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 30 Jul 2018 18:49:03 -0700 Subject: [PATCH 1/5] adb: move list-forward, kill-forward back into handle_forward_request. The daemon-side reverse functions depended on handle_forward_request: move them back instead of duplicating the logic we had in handle_host_request. Accomplish what we originally wanted to do in this change by changing the transport argument of handle_forward_request to a std::function that acquires a transport, either via acquire_one_transport or immediately returning a value that we already have. As a side effect, fix a bug where we would emit spurious errors for host service requests. Bug: http://b/112009742 Test: echo "001chost:connect:127.0.0.1:5555" | nc localhost 5037 Test: python test_device.py Test: python test_adb.py Change-Id: Iccc555575df6dbd7de10382854c4ea2c6f4beeaa --- adb/adb.cpp | 84 +++++++++++++++++++++++------------------ adb/adb.h | 5 ++- adb/daemon/services.cpp | 2 +- 3 files changed, 53 insertions(+), 38 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index 19300f660..38c6f62c9 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -920,13 +920,45 @@ int launch_server(const std::string& socket_spec) { } #endif /* ADB_HOST */ +bool handle_forward_request(const char* service, atransport* transport, int reply_fd) { + return handle_forward_request(service, [transport](std::string*) { return transport; }, + reply_fd); +} + // Try to handle a network forwarding request. -// This returns 1 on success, 0 on failure, and -1 to indicate this is not -// a forwarding-related request. -int handle_forward_request(const char* service, atransport* transport, int reply_fd) { +bool handle_forward_request(const char* service, + std::function transport_acquirer, + int reply_fd) { + if (!strcmp(service, "list-forward")) { + // Create the list of forward redirections. + std::string listeners = format_listeners(); +#if ADB_HOST + SendOkay(reply_fd); +#endif + SendProtocolString(reply_fd, listeners); + return true; + } + + if (!strcmp(service, "killforward-all")) { + remove_all_listeners(); +#if ADB_HOST + /* On the host: 1st OKAY is connect, 2nd OKAY is status */ + SendOkay(reply_fd); +#endif + SendOkay(reply_fd); + return true; + } + if (!strncmp(service, "forward:", 8) || !strncmp(service, "killforward:", 12)) { // killforward:local // forward:(norebind:)?local;remote + std::string error; + atransport* transport = transport_acquirer(&error); + if (!transport) { + SendFail(reply_fd, error); + return true; + } + bool kill_forward = false; bool no_rebind = false; if (android::base::StartsWith(service, "killforward:")) { @@ -946,17 +978,16 @@ int handle_forward_request(const char* service, atransport* transport, int reply // Check killforward: parameter format: '' if (pieces.size() != 1 || pieces[0].empty()) { SendFail(reply_fd, android::base::StringPrintf("bad killforward: %s", service)); - return 1; + return true; } } else { // Check forward: parameter format: ';' if (pieces.size() != 2 || pieces[0].empty() || pieces[1].empty() || pieces[1][0] == '*') { SendFail(reply_fd, android::base::StringPrintf("bad forward: %s", service)); - return 1; + return true; } } - std::string error; InstallStatus r; int resolved_tcp_port = 0; if (kill_forward) { @@ -977,7 +1008,7 @@ int handle_forward_request(const char* service, atransport* transport, int reply SendProtocolString(reply_fd, android::base::StringPrintf("%d", resolved_tcp_port)); } - return 1; + return true; } std::string message; @@ -996,9 +1027,10 @@ int handle_forward_request(const char* service, atransport* transport, int reply break; } SendFail(reply_fd, message); - return 1; + return true; } - return 0; + + return false; } #if ADB_HOST @@ -1186,35 +1218,15 @@ int handle_host_request(const char* service, TransportType type, const char* ser return SendOkay(reply_fd, response); } - if (!strcmp(service, "list-forward")) { - // Create the list of forward redirections. - std::string listeners = format_listeners(); -#if ADB_HOST - SendOkay(reply_fd); -#endif - return SendProtocolString(reply_fd, listeners); + if (handle_forward_request(service, + [=](std::string* error) { + return acquire_one_transport(type, serial, transport_id, nullptr, + error); + }, + reply_fd)) { + return 0; } - if (!strcmp(service, "killforward-all")) { - remove_all_listeners(); -#if ADB_HOST - /* On the host: 1st OKAY is connect, 2nd OKAY is status */ - SendOkay(reply_fd); -#endif - SendOkay(reply_fd); - return 1; - } - - std::string error; - atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &error); - if (!t) { - SendFail(reply_fd, error); - return 1; - } - - int ret = handle_forward_request(service, t, reply_fd); - if (ret >= 0) - return ret - 1; return -1; } diff --git a/adb/adb.h b/adb/adb.h index 13ca4d7e0..e6af780c4 100644 --- a/adb/adb.h +++ b/adb/adb.h @@ -158,7 +158,10 @@ asocket* create_jdwp_tracker_service_socket(); unique_fd create_jdwp_connection_fd(int jdwp_pid); #endif -int handle_forward_request(const char* service, atransport* transport, int reply_fd); +bool handle_forward_request(const char* service, atransport* transport, int reply_fd); +bool handle_forward_request(const char* service, + std::function transport_acquirer, + int reply_fd); /* packet allocator */ apacket* get_apacket(void); diff --git a/adb/daemon/services.cpp b/adb/daemon/services.cpp index 25024b05a..1f59d6446 100644 --- a/adb/daemon/services.cpp +++ b/adb/daemon/services.cpp @@ -157,7 +157,7 @@ unique_fd reverse_service(const char* command, atransport* transport) { return unique_fd{}; } VLOG(SERVICES) << "service socketpair: " << s[0] << ", " << s[1]; - if (handle_forward_request(command, transport, s[1]) < 0) { + if (!handle_forward_request(command, transport, s[1])) { SendFail(s[1], "not a reverse forwarding command"); } adb_close(s[1]); From 95af641bd65f6f02318ebc66bf5faaeb370108c8 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 30 Jul 2018 18:51:55 -0700 Subject: [PATCH 2/5] adb: don't pass time_point::max to condition_variable::wait_until. libstdc++ implements wait_until by calculating the offset between its default clock and the clock that it's given by calling now() on each and subtracting, and then adds that offset to the time_point argument. When time_point::max is used, this overflows, resulting in the reconnection thread spinning. Test: wine adb.exe server nodaemon Change-Id: Ife58f0aad14bc44c0804483d3ff2351c28b3d576 --- adb/transport.cpp | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/adb/transport.cpp b/adb/transport.cpp index 793c283fc..b08c7f9bb 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -102,7 +102,7 @@ class ReconnectHandler { // Tracks a reconnection attempt. struct ReconnectAttempt { atransport* transport; - std::chrono::system_clock::time_point deadline; + std::chrono::steady_clock::time_point reconnect_time; size_t attempts_left; }; @@ -149,8 +149,8 @@ void ReconnectHandler::TrackTransport(atransport* transport) { std::lock_guard lock(reconnect_mutex_); if (!running_) return; reconnect_queue_.emplace(ReconnectAttempt{ - transport, std::chrono::system_clock::now() + ReconnectHandler::kDefaultTimeout, - ReconnectHandler::kMaxAttempts}); + transport, std::chrono::steady_clock::now() + ReconnectHandler::kDefaultTimeout, + ReconnectHandler::kMaxAttempts}); } reconnect_cv_.notify_one(); } @@ -162,14 +162,26 @@ void ReconnectHandler::Run() { std::unique_lock lock(reconnect_mutex_); ScopedAssumeLocked assume_lock(reconnect_mutex_); - auto deadline = std::chrono::time_point::max(); - if (!reconnect_queue_.empty()) deadline = reconnect_queue_.front().deadline; - reconnect_cv_.wait_until(lock, deadline, [&]() REQUIRES(reconnect_mutex_) { - return !running_ || - (!reconnect_queue_.empty() && reconnect_queue_.front().deadline < deadline); - }); + if (!reconnect_queue_.empty()) { + // FIXME: libstdc++ (used on Windows) implements condition_variable with + // system_clock as its clock, so we're probably hosed if the clock changes, + // even if we use steady_clock throughout. This problem goes away once we + // switch to libc++. + reconnect_cv_.wait_until(lock, reconnect_queue_.front().reconnect_time); + } else { + reconnect_cv_.wait(lock); + } if (!running_) return; + if (reconnect_queue_.empty()) continue; + + // Go back to sleep in case |reconnect_cv_| woke up spuriously and we still + // have more time to wait for the current attempt. + auto now = std::chrono::steady_clock::now(); + if (reconnect_queue_.front().reconnect_time > now) { + continue; + } + attempt = reconnect_queue_.front(); reconnect_queue_.pop(); if (attempt.transport->kicked()) { @@ -191,9 +203,9 @@ void ReconnectHandler::Run() { std::lock_guard lock(reconnect_mutex_); reconnect_queue_.emplace(ReconnectAttempt{ - attempt.transport, - std::chrono::system_clock::now() + ReconnectHandler::kDefaultTimeout, - attempt.attempts_left - 1}); + attempt.transport, + std::chrono::steady_clock::now() + ReconnectHandler::kDefaultTimeout, + attempt.attempts_left - 1}); continue; } From e445a6d8a23d456c0720c92bc7b31ce033bfc345 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Tue, 31 Jul 2018 14:12:59 -0700 Subject: [PATCH 3/5] adb: more immediately try to reconnect connections. Most disconnects we're likely to encounter are cases where either we notice immediately and can start reconnecting almost immediately (adbd restarting because of `adb root`, etc.), or where we won't notice for a while anyway, so a 10 second sleep is somewhat meaningless. Test: adb root; time adb wait-for-device shell Change-Id: I18e9213dc4e84d735e9240118a368dcb38f21c78 --- adb/transport.cpp | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/adb/transport.cpp b/adb/transport.cpp index b08c7f9bb..b45c43f89 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -50,6 +50,7 @@ #include "adb_trace.h" #include "adb_utils.h" #include "fdevent.h" +#include "sysdeps/chrono.h" static void register_transport(atransport* transport); static void remove_transport(atransport* transport); @@ -104,10 +105,16 @@ class ReconnectHandler { atransport* transport; std::chrono::steady_clock::time_point reconnect_time; size_t attempts_left; + + bool operator<(const ReconnectAttempt& rhs) const { + // std::priority_queue returns the largest element first, so we want attempts that have + // less time remaining (i.e. smaller time_points) to compare greater. + return reconnect_time > rhs.reconnect_time; + } }; // Only retry for up to one minute. - static constexpr const std::chrono::seconds kDefaultTimeout = std::chrono::seconds(10); + static constexpr const std::chrono::seconds kDefaultTimeout = 10s; static constexpr const size_t kMaxAttempts = 6; // Protects all members. @@ -115,7 +122,7 @@ class ReconnectHandler { bool running_ GUARDED_BY(reconnect_mutex_) = true; std::thread handler_thread_; std::condition_variable reconnect_cv_; - std::queue reconnect_queue_ GUARDED_BY(reconnect_mutex_); + std::priority_queue reconnect_queue_ GUARDED_BY(reconnect_mutex_); DISALLOW_COPY_AND_ASSIGN(ReconnectHandler); }; @@ -137,7 +144,7 @@ void ReconnectHandler::Stop() { // Drain the queue to free all resources. std::lock_guard lock(reconnect_mutex_); while (!reconnect_queue_.empty()) { - ReconnectAttempt attempt = reconnect_queue_.front(); + ReconnectAttempt attempt = reconnect_queue_.top(); reconnect_queue_.pop(); remove_transport(attempt.transport); } @@ -148,9 +155,10 @@ void ReconnectHandler::TrackTransport(atransport* transport) { { std::lock_guard lock(reconnect_mutex_); if (!running_) return; - reconnect_queue_.emplace(ReconnectAttempt{ - transport, std::chrono::steady_clock::now() + ReconnectHandler::kDefaultTimeout, - ReconnectHandler::kMaxAttempts}); + // Arbitrary sleep to give adbd time to get ready, if we disconnected because it exited. + auto reconnect_time = std::chrono::steady_clock::now() + 250ms; + reconnect_queue_.emplace( + ReconnectAttempt{transport, reconnect_time, ReconnectHandler::kMaxAttempts}); } reconnect_cv_.notify_one(); } @@ -167,7 +175,7 @@ void ReconnectHandler::Run() { // system_clock as its clock, so we're probably hosed if the clock changes, // even if we use steady_clock throughout. This problem goes away once we // switch to libc++. - reconnect_cv_.wait_until(lock, reconnect_queue_.front().reconnect_time); + reconnect_cv_.wait_until(lock, reconnect_queue_.top().reconnect_time); } else { reconnect_cv_.wait(lock); } @@ -178,11 +186,11 @@ void ReconnectHandler::Run() { // Go back to sleep in case |reconnect_cv_| woke up spuriously and we still // have more time to wait for the current attempt. auto now = std::chrono::steady_clock::now(); - if (reconnect_queue_.front().reconnect_time > now) { + if (reconnect_queue_.top().reconnect_time > now) { continue; } - attempt = reconnect_queue_.front(); + attempt = reconnect_queue_.top(); reconnect_queue_.pop(); if (attempt.transport->kicked()) { D("transport %s was kicked. giving up on it.", attempt.transport->serial.c_str()); From 258d4a91c962b182a6119a69b631269ee7ffba58 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Tue, 31 Jul 2018 15:36:03 -0700 Subject: [PATCH 4/5] adb: fix error message for `adb {forward,reverse}` Test: `adb reverse` Change-Id: I6fa078ea7a0e019df5c6309df99ffdf2a3219194 --- adb/client/commandline.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adb/client/commandline.cpp b/adb/client/commandline.cpp index a7a94e775..3fb14f351 100644 --- a/adb/client/commandline.cpp +++ b/adb/client/commandline.cpp @@ -1614,9 +1614,9 @@ int adb_commandline(int argc, const char** argv) { return bugreport.DoIt(argc, argv); } else if (!strcmp(argv[0], "forward") || !strcmp(argv[0], "reverse")) { bool reverse = !strcmp(argv[0], "reverse"); - ++argv; --argc; if (argc < 1) return syntax_error("%s requires an argument", argv[0]); + ++argv; // Determine the for this command. std::string host_prefix; From def91c0abff84d6b51db507965cfb5f543538a80 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Tue, 31 Jul 2018 18:28:32 -0700 Subject: [PATCH 5/5] adb: disable ReconnectHandler in adbd. Previously, when a TCP connection was disconnected from adbd, we were registering it with ReconnectHandler, which led to the transport sticking around after the socket was closed. Due to the naming of TCP transports in adbd (host-), this results in incoming connections being immediately closed if their file descriptor number ends up being the same as a TCP transport that had previously disconnected. Guard all of the reconnect logic with ADB_HOST, to fix this. Bug: http://b/112054041 Test: while true; do adb connect ; adb connect ; adb shell true; done Change-Id: Ib55d304d7e07d6d744e8321d34671bb6d4b91afe --- adb/transport.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/adb/transport.cpp b/adb/transport.cpp index b45c43f89..922200826 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -81,6 +81,7 @@ class SCOPED_CAPABILITY ScopedAssumeLocked { ~ScopedAssumeLocked() RELEASE() {} }; +#if ADB_HOST // Tracks and handles atransport*s that are attempting reconnection. class ReconnectHandler { public: @@ -224,6 +225,8 @@ void ReconnectHandler::Run() { static auto& reconnect_handler = *new ReconnectHandler(); +#endif + } // namespace TransportId NextTransportId() { @@ -697,9 +700,11 @@ static void transport_registration_func(int _fd, unsigned ev, void*) { update_transports(); } +#if ADB_HOST void init_reconnect_handler(void) { reconnect_handler.Start(); } +#endif void init_transport_registration(void) { int s[2]; @@ -718,7 +723,9 @@ void init_transport_registration(void) { } void kick_all_transports() { +#if ADB_HOST reconnect_handler.Stop(); +#endif // To avoid only writing part of a packet to a transport after exit, kick all transports. std::lock_guard lock(transport_lock); for (auto t : transport_list) { @@ -756,13 +763,19 @@ static void transport_unref(atransport* t) { t->ref_count--; if (t->ref_count == 0) { t->connection()->Stop(); +#if ADB_HOST if (t->IsTcpDevice() && !t->kicked()) { - D("transport: %s unref (attempting reconnection) %d", t->serial.c_str(), t->kicked()); + D("transport: %s unref (attempting reconnection)", t->serial.c_str()); reconnect_handler.TrackTransport(t); } else { D("transport: %s unref (kicking and closing)", t->serial.c_str()); remove_transport(t); } +#else + D("transport: %s unref (kicking and closing)", t->serial.c_str()); + remove_transport(t); +#endif + } else { D("transport: %s unref (count=%zu)", t->serial.c_str(), t->ref_count); }