From aeca208385ba5a8307e1c297548eef0ef1999c6f Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 20 Feb 2019 19:46:26 -0800 Subject: [PATCH 01/12] adb: make ParseUint reject garbage at the end by default. Test: adb_test on host Change-Id: Ie63d89bd08f0b296a3d54ff66aae85fe52d8cd0f --- adb/adb_utils.h | 5 ++++- adb/adb_utils_test.cpp | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/adb/adb_utils.h b/adb/adb_utils.h index a85ca8c84..8b0dcee83 100644 --- a/adb/adb_utils.h +++ b/adb/adb_utils.h @@ -110,7 +110,7 @@ inline std::string_view StripTrailingNulls(std::string_view str) { // Base-10 stroll on a string_view. template -inline bool ParseUint(T* result, std::string_view str, std::string_view* remaining) { +inline bool ParseUint(T* result, std::string_view str, std::string_view* remaining = nullptr) { if (str.empty() || !isdigit(str[0])) { return false; } @@ -135,6 +135,9 @@ inline bool ParseUint(T* result, std::string_view str, std::string_view* remaini *result = value; if (remaining) { *remaining = str.substr(it - str.begin()); + } else { + return it == str.end(); } + return true; } diff --git a/adb/adb_utils_test.cpp b/adb/adb_utils_test.cpp index 8518e1782..bd676c2de 100644 --- a/adb/adb_utils_test.cpp +++ b/adb/adb_utils_test.cpp @@ -206,6 +206,14 @@ void TestParseUint(std::string_view string, bool expected_success, uint32_t expe EXPECT_EQ(remaining, "foo"); } } + + // With trailing text, without remaining. + { + std::string text = std::string(string) + "foo"; + uint32_t value; + bool success = ParseUint(&value, text, nullptr); + EXPECT_EQ(success, false); + } } TEST(adb_utils, ParseUint) { From ac8da2a7dfba9696fc7a6570b04a6cf069613183 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 20 Feb 2019 19:36:00 -0800 Subject: [PATCH 02/12] adb: add helper to consume a prefix on a string_view. It's error-prone to manually writing code of the following form: if (foo.starts_with("some_prefix:")) { foo.remove_prefix(strlen("some_prefix:")); } Add a helper to do that for us. Test: mma Change-Id: I5df391deba8b6c036fcbf17a1f1c79af8d9abd2b --- adb/adb_utils.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/adb/adb_utils.h b/adb/adb_utils.h index 8b0dcee83..5800a62f7 100644 --- a/adb/adb_utils.h +++ b/adb/adb_utils.h @@ -141,3 +141,11 @@ inline bool ParseUint(T* result, std::string_view str, std::string_view* remaini return true; } + +inline bool ConsumePrefix(std::string_view* str, std::string_view prefix) { + if (str->starts_with(prefix)) { + str->remove_prefix(prefix.size()); + return true; + } + return false; +} From 342b661e8112002e7c27e45f62a16d205033b963 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 20 Feb 2019 17:16:37 -0800 Subject: [PATCH 03/12] adb: switch adb_io.h to string_view. Test: mma Change-Id: Ifa16502bac18429491426c45ece562657cff1689 --- adb/adb_io.cpp | 4 ++-- adb/adb_io.h | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/adb/adb_io.cpp b/adb/adb_io.cpp index 91b0d1f62..f5cdcb558 100644 --- a/adb/adb_io.cpp +++ b/adb/adb_io.cpp @@ -34,7 +34,7 @@ #include "adb_utils.h" #include "sysdeps.h" -bool SendProtocolString(int fd, const std::string& s) { +bool SendProtocolString(int fd, std::string_view s) { unsigned int length = s.size(); if (length > MAX_PAYLOAD - 4) { errno = EMSGSIZE; @@ -69,7 +69,7 @@ bool SendOkay(int fd) { return WriteFdExactly(fd, "OKAY", 4); } -bool SendFail(int fd, const std::string& reason) { +bool SendFail(int fd, std::string_view reason) { return WriteFdExactly(fd, "FAIL", 4) && SendProtocolString(fd, reason); } diff --git a/adb/adb_io.h b/adb/adb_io.h index e2df1b1ef..d6e65d8de 100644 --- a/adb/adb_io.h +++ b/adb/adb_io.h @@ -20,6 +20,7 @@ #include #include +#include #include "adb_unique_fd.h" @@ -27,10 +28,10 @@ bool SendOkay(int fd); // Sends the protocol "FAIL" message, with the given failure reason. -bool SendFail(int fd, const std::string& reason); +bool SendFail(int fd, std::string_view reason); // Writes a protocol-format string; a four hex digit length followed by the string data. -bool SendProtocolString(int fd, const std::string& s); +bool SendProtocolString(int fd, std::string_view s); // Reads a protocol-format string; a four hex digit length followed by the string data. bool ReadProtocolString(int fd, std::string* s, std::string* error); From 769f853f965d3758af83659acaeefb0b2c91373b Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 20 Feb 2019 19:30:59 -0800 Subject: [PATCH 04/12] adb: switch handle_host_request to string_view. Test: test_adb.py Test: test_device.py Change-Id: Ideae5262a6dff2e27c50f666144d48f874837290 --- adb/adb.cpp | 87 ++++++++++++++++++++++++++----------------------- adb/adb.h | 2 +- adb/sockets.cpp | 3 +- 3 files changed, 48 insertions(+), 44 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index a5b2f7b84..32fbb6555 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -1018,9 +1018,9 @@ static int SendOkay(int fd, const std::string& s) { return 0; } -bool handle_host_request(const char* service, TransportType type, const char* serial, +bool handle_host_request(std::string_view service, TransportType type, const char* serial, TransportId transport_id, int reply_fd, asocket* s) { - if (strcmp(service, "kill") == 0) { + if (service == "kill") { fprintf(stderr, "adb server killed by remote request\n"); fflush(stdout); @@ -1036,25 +1036,25 @@ bool handle_host_request(const char* service, TransportType type, const char* se // "transport-usb:" is used for switching transport to the only USB transport // "transport-local:" is used for switching transport to the only local transport // "transport-any:" is used for switching transport to the only transport - if (!strncmp(service, "transport", strlen("transport"))) { + if (service.starts_with("transport")) { TransportType type = kTransportAny; - if (!strncmp(service, "transport-id:", strlen("transport-id:"))) { - service += strlen("transport-id:"); - transport_id = strtoll(service, const_cast(&service), 10); - if (*service != '\0') { + std::string serial_storage; + + if (ConsumePrefix(&service, "transport-id:")) { + if (!ParseUint(&transport_id, service)) { SendFail(reply_fd, "invalid transport id"); return true; } - } else if (!strncmp(service, "transport-usb", strlen("transport-usb"))) { + } else if (service == "transport-usb") { type = kTransportUsb; - } else if (!strncmp(service, "transport-local", strlen("transport-local"))) { + } else if (service == "transport-local") { type = kTransportLocal; - } else if (!strncmp(service, "transport-any", strlen("transport-any"))) { + } else if (service == "transport-any") { type = kTransportAny; - } else if (!strncmp(service, "transport:", strlen("transport:"))) { - service += strlen("transport:"); - serial = service; + } else if (ConsumePrefix(&service, "transport:")) { + serial_storage = service; + serial = serial_storage.c_str(); } std::string error; @@ -1072,18 +1072,16 @@ bool handle_host_request(const char* service, TransportType type, const char* se } // return a list of all connected devices - if (!strncmp(service, "devices", 7)) { - bool long_listing = (strcmp(service+7, "-l") == 0); - if (long_listing || service[7] == 0) { - D("Getting device list..."); - std::string device_list = list_transports(long_listing); - D("Sending device list..."); - SendOkay(reply_fd, device_list); - } + if (service == "devices" || service == "devices-l") { + bool long_listing = service == "devices-l"; + D("Getting device list..."); + std::string device_list = list_transports(long_listing); + D("Sending device list..."); + SendOkay(reply_fd, device_list); return true; } - if (!strcmp(service, "reconnect-offline")) { + if (service == "reconnect-offline") { std::string response; close_usb_devices([&response](const atransport* transport) { if (!ConnectionStateIsOnline(transport->GetConnectionState())) { @@ -1099,7 +1097,7 @@ bool handle_host_request(const char* service, TransportType type, const char* se return true; } - if (!strcmp(service, "features")) { + if (service == "features") { std::string error; atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &error); if (t != nullptr) { @@ -1110,7 +1108,7 @@ bool handle_host_request(const char* service, TransportType type, const char* se return true; } - if (!strcmp(service, "host-features")) { + if (service == "host-features") { FeatureSet features = supported_features(); // Abuse features to report libusb status. if (should_use_libusb()) { @@ -1122,8 +1120,8 @@ bool handle_host_request(const char* service, TransportType type, const char* se } // remove TCP transport - if (!strncmp(service, "disconnect:", 11)) { - const std::string address(service + 11); + if (service.starts_with("disconnect:")) { + std::string address(service.substr(11)); if (address.empty()) { kick_all_tcp_devices(); SendOkay(reply_fd, "disconnected everything"); @@ -1152,13 +1150,13 @@ bool handle_host_request(const char* service, TransportType type, const char* se } // Returns our value for ADB_SERVER_VERSION. - if (!strcmp(service, "version")) { + if (service == "version") { SendOkay(reply_fd, android::base::StringPrintf("%04x", ADB_SERVER_VERSION)); return true; } // These always report "unknown" rather than the actual error, for scripts. - if (!strcmp(service, "get-serialno")) { + if (service == "get-serialno") { std::string error; atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &error); if (t) { @@ -1168,7 +1166,7 @@ bool handle_host_request(const char* service, TransportType type, const char* se } return true; } - if (!strcmp(service, "get-devpath")) { + if (service == "get-devpath") { std::string error; atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &error); if (t) { @@ -1178,7 +1176,7 @@ bool handle_host_request(const char* service, TransportType type, const char* se } return true; } - if (!strcmp(service, "get-state")) { + if (service == "get-state") { std::string error; atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &error); if (t) { @@ -1190,31 +1188,38 @@ bool handle_host_request(const char* service, TransportType type, const char* se } // Indicates a new emulator instance has started. - if (!strncmp(service, "emulator:", 9)) { - int port = atoi(service+9); - local_connect(port); + if (ConsumePrefix(&service, "emulator:")) { + unsigned int port; + if (!ParseUint(&port, service)) { + LOG(ERROR) << "received invalid port for emulator: " << service; + } else { + local_connect(port); + } + /* we don't even need to send a reply */ return true; } - if (!strcmp(service, "reconnect")) { + if (service == "reconnect") { std::string response; atransport* t = acquire_one_transport(type, serial, transport_id, nullptr, &response, true); if (t != nullptr) { kick_transport(t); response = - "reconnecting " + t->serial_name() + " [" + t->connection_state_name() + "]\n"; + "reconnecting " + t->serial_name() + " [" + t->connection_state_name() + "]\n"; } SendOkay(reply_fd, response); return true; } - if (handle_forward_request(service, - [=](std::string* error) { - return acquire_one_transport(type, serial, transport_id, nullptr, - error); - }, - reply_fd)) { + // TODO: Switch handle_forward_request to string_view. + std::string service_str(service); + if (handle_forward_request( + service_str.c_str(), + [=](std::string* error) { + return acquire_one_transport(type, serial, transport_id, nullptr, error); + }, + reply_fd)) { return true; } diff --git a/adb/adb.h b/adb/adb.h index 3a8cb7b1c..89d7663b6 100644 --- a/adb/adb.h +++ b/adb/adb.h @@ -218,7 +218,7 @@ extern const char* adb_device_banner; #define USB_FFS_ADB_IN USB_FFS_ADB_EP(ep2) #endif -bool handle_host_request(const char* service, TransportType type, const char* serial, +bool handle_host_request(std::string_view service, TransportType type, const char* serial, TransportId transport_id, int reply_fd, asocket* s); void handle_online(atransport* t); diff --git a/adb/sockets.cpp b/adb/sockets.cpp index 420a6d5a3..df0104b66 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -808,8 +808,7 @@ static int smart_socket_enqueue(asocket* s, apacket::payload_type data) { // Some requests are handled immediately -- in that case the handle_host_request() routine // has sent the OKAY or FAIL message and all we have to do is clean up. - // TODO: Convert to string_view. - if (handle_host_request(std::string(service).c_str(), type, + if (handle_host_request(service, type, serial.empty() ? nullptr : std::string(serial).c_str(), transport_id, s->peer->fd, s)) { LOG(VERBOSE) << "SS(" << s->id << "): handled host service '" << service << "'"; From 0ecc4020d600fc5c0a9c4585fb651a7dad178ea1 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 22 Feb 2019 13:41:55 -0800 Subject: [PATCH 05/12] adb: switch host_service_to_socket to string_view. Test: manual Change-Id: I09b83cc82edb10c1a51ecc5ebb8d9fe5294222c3 --- adb/adb.h | 3 ++- adb/services.cpp | 43 ++++++++++++++++++++----------------------- adb/sockets.cpp | 19 +------------------ 3 files changed, 23 insertions(+), 42 deletions(-) diff --git a/adb/adb.h b/adb/adb.h index 89d7663b6..f575adb47 100644 --- a/adb/adb.h +++ b/adb/adb.h @@ -145,7 +145,8 @@ unique_fd daemon_service_to_fd(std::string_view name, atransport* transport); #endif #if ADB_HOST -asocket* host_service_to_socket(const char* name, const char* serial, TransportId transport_id); +asocket* host_service_to_socket(std::string_view name, std::string_view serial, + TransportId transport_id); #endif #if !ADB_HOST diff --git a/adb/services.cpp b/adb/services.cpp index 0061f0ead..1ed71823e 100644 --- a/adb/services.cpp +++ b/adb/services.cpp @@ -25,6 +25,7 @@ #include #include + #include #include #include @@ -63,11 +64,11 @@ unique_fd create_service_thread(const char* service_name, std::function sinfo = std::make_shared(); if (sinfo == nullptr) { fprintf(stderr, "couldn't allocate state_info: %s", strerror(errno)); return nullptr; } - if (serial) sinfo->serial = serial; + sinfo->serial = serial; sinfo->transport_id = transport_id; - if (android::base::StartsWith(name, "local")) { - name += strlen("local"); + if (ConsumePrefix(&name, "local")) { sinfo->transport_type = kTransportLocal; - } else if (android::base::StartsWith(name, "usb")) { - name += strlen("usb"); + } else if (ConsumePrefix(&name, "usb")) { sinfo->transport_type = kTransportUsb; - } else if (android::base::StartsWith(name, "any")) { - name += strlen("any"); + } else if (ConsumePrefix(&name, "any")) { sinfo->transport_type = kTransportAny; } else { return nullptr; } - if (!strcmp(name, "-device")) { + if (name == "-device") { sinfo->state = kCsDevice; - } else if (!strcmp(name, "-recovery")) { + } else if (name == "-recovery") { sinfo->state = kCsRecovery; - } else if (!strcmp(name, "-sideload")) { + } else if (name == "-sideload") { sinfo->state = kCsSideload; - } else if (!strcmp(name, "-bootloader")) { + } else if (name == "-bootloader") { sinfo->state = kCsBootloader; - } else if (!strcmp(name, "-any")) { + } else if (name == "-any") { sinfo->state = kCsAny; } else { return nullptr; @@ -235,8 +232,8 @@ asocket* host_service_to_socket(const char* name, const char* serial, TransportI wait_for_state(fd, sinfo.get()); }); return create_local_socket(std::move(fd)); - } else if (!strncmp(name, "connect:", 8)) { - std::string host(name + strlen("connect:")); + } else if (ConsumePrefix(&name, "connect:")) { + std::string host(name); unique_fd fd = create_service_thread( "connect", std::bind(connect_service, std::placeholders::_1, host)); return create_local_socket(std::move(fd)); diff --git a/adb/sockets.cpp b/adb/sockets.cpp index df0104b66..04d92db60 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -426,22 +426,6 @@ asocket* create_local_service_socket(std::string_view name, atransport* transpor return s; } -#if ADB_HOST -static asocket* create_host_service_socket(const char* name, const char* serial, - TransportId transport_id) { - asocket* s; - - s = host_service_to_socket(name, serial, transport_id); - - if (s != nullptr) { - D("LS(%d) bound to '%s'", s->id, name); - return s; - } - - return s; -} -#endif /* ADB_HOST */ - static int remote_socket_enqueue(asocket* s, apacket::payload_type data) { D("entered remote_socket_enqueue RS(%d) WRITE fd=%d peer.fd=%d", s->id, s->fd, s->peer->fd); apacket* p = get_apacket(); @@ -825,8 +809,7 @@ static int smart_socket_enqueue(asocket* s, apacket::payload_type data) { ** and tear down here. */ // TODO: Convert to string_view. - s2 = create_host_service_socket(std::string(service).c_str(), std::string(serial).c_str(), - transport_id); + s2 = host_service_to_socket(service, serial, transport_id); if (s2 == nullptr) { LOG(VERBOSE) << "SS(" << s->id << "): couldn't create host service '" << service << "'"; SendFail(s->peer->fd, "unknown host service"); From 2df76b7ee7e97cfeaedc649d3324e9de1ec09cd5 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 20 Feb 2019 20:00:27 -0800 Subject: [PATCH 06/12] adbd: switch daemon/services to ConsumePrefix. Test: mma Change-Id: Ib4bb7d3352219a9883a5089e08225fb17145c2ee --- adb/daemon/services.cpp | 46 ++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/adb/daemon/services.cpp b/adb/daemon/services.cpp index 362a987e0..b0cc4509d 100644 --- a/adb/daemon/services.cpp +++ b/adb/daemon/services.cpp @@ -223,17 +223,15 @@ asocket* daemon_service_to_socket(std::string_view name) { return create_jdwp_service_socket(); } else if (name == "track-jdwp") { return create_jdwp_tracker_service_socket(); - } else if (name.starts_with("sink:")) { - name.remove_prefix(strlen("sink:")); + } else if (ConsumePrefix(&name, "sink:")) { uint64_t byte_count = 0; - if (!android::base::ParseUint(name.data(), &byte_count)) { + if (!ParseUint(&byte_count, name)) { return nullptr; } return new SinkSocket(byte_count); - } else if (name.starts_with("source:")) { - name.remove_prefix(strlen("source:")); + } else if (ConsumePrefix(&name, "source:")) { uint64_t byte_count = 0; - if (!android::base::ParseUint(name.data(), &byte_count)) { + if (!ParseUint(&byte_count, name)) { return nullptr; } return new SourceSocket(byte_count); @@ -252,20 +250,19 @@ unique_fd daemon_service_to_fd(std::string_view name, atransport* transport) { #if defined(__ANDROID__) if (name.starts_with("framebuffer:")) { return create_service_thread("fb", framebuffer_service); - } else if (name.starts_with("remount:")) { - std::string arg(name.begin() + strlen("remount:"), name.end()); + } else if (ConsumePrefix(&name, "remount:")) { + std::string arg(name); return create_service_thread("remount", std::bind(remount_service, std::placeholders::_1, arg)); - } else if (name.starts_with("reboot:")) { - std::string arg(name.begin() + strlen("reboot:"), name.end()); + } else if (ConsumePrefix(&name, "reboot:")) { + std::string arg(name); return create_service_thread("reboot", std::bind(reboot_service, std::placeholders::_1, arg)); } else if (name.starts_with("root:")) { return create_service_thread("root", restart_root_service); } else if (name.starts_with("unroot:")) { return create_service_thread("unroot", restart_unroot_service); - } else if (name.starts_with("backup:")) { - name.remove_prefix(strlen("backup:")); + } else if (ConsumePrefix(&name, "backup:")) { std::string cmd = "/system/bin/bu backup "; cmd += name; return StartSubprocess(cmd, nullptr, SubprocessType::kRaw, SubprocessProtocol::kNone); @@ -278,8 +275,7 @@ unique_fd daemon_service_to_fd(std::string_view name, atransport* transport) { } else if (name.starts_with("enable-verity:")) { return create_service_thread("verity-off", std::bind(set_verity_enabled_state_service, std::placeholders::_1, true)); - } else if (name.starts_with("tcpip:")) { - name.remove_prefix(strlen("tcpip:")); + } else if (ConsumePrefix(&name, "tcpip:")) { std::string str(name); int port; @@ -293,24 +289,22 @@ unique_fd daemon_service_to_fd(std::string_view name, atransport* transport) { } #endif - if (name.starts_with("dev:")) { - name.remove_prefix(strlen("dev:")); + if (ConsumePrefix(&name, "dev:")) { return unique_fd{unix_open(name, O_RDWR | O_CLOEXEC)}; - } else if (name.starts_with("jdwp:")) { - name.remove_prefix(strlen("jdwp:")); - std::string str(name); - return create_jdwp_connection_fd(atoi(str.c_str())); - } else if (name.starts_with("shell")) { - name.remove_prefix(strlen("shell")); + } else if (ConsumePrefix(&name, "jdwp:")) { + pid_t pid; + if (!ParseUint(&pid, name)) { + return unique_fd{}; + } + return create_jdwp_connection_fd(pid); + } else if (ConsumePrefix(&name, "shell")) { return ShellService(name, transport); - } else if (name.starts_with("exec:")) { - name.remove_prefix(strlen("exec:")); + } else if (ConsumePrefix(&name, "exec:")) { return StartSubprocess(std::string(name), nullptr, SubprocessType::kRaw, SubprocessProtocol::kNone); } else if (name.starts_with("sync:")) { return create_service_thread("sync", file_sync_service); - } else if (name.starts_with("reverse:")) { - name.remove_prefix(strlen("reverse:")); + } else if (ConsumePrefix(&name, "reverse:")) { return reverse_service(name, transport); } else if (name == "reconnect") { return create_service_thread( From 79797ecbb1b099e94564505acb9f86bbea7e15e1 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 20 Feb 2019 20:37:26 -0800 Subject: [PATCH 07/12] adb: tell the client what transport it received. Prerequisite for making `adb root` wait for the device that it told to restart to disappear: the client needs to know which transport to wait on. Bug: http://b/124244488 Test: manual Change-Id: I474559838ad7c0e961e9d2a98c902bca3b60d6c8 --- adb/adb.cpp | 103 +++++++++++++++++++++++--------------- adb/adb.h | 11 +++- adb/client/adb_client.cpp | 75 +++++++++++++++++---------- adb/client/adb_client.h | 5 +- adb/sockets.cpp | 26 ++++++---- 5 files changed, 142 insertions(+), 78 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index 32fbb6555..3c0788254 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -1018,8 +1018,9 @@ static int SendOkay(int fd, const std::string& s) { return 0; } -bool handle_host_request(std::string_view service, TransportType type, const char* serial, - TransportId transport_id, int reply_fd, asocket* s) { +HostRequestResult handle_host_request(std::string_view service, TransportType type, + const char* serial, TransportId transport_id, int reply_fd, + asocket* s) { if (service == "kill") { fprintf(stderr, "adb server killed by remote request\n"); fflush(stdout); @@ -1032,29 +1033,49 @@ bool handle_host_request(std::string_view service, TransportType type, const cha exit(0); } - // "transport:" is used for switching transport with a specified serial number - // "transport-usb:" is used for switching transport to the only USB transport - // "transport-local:" is used for switching transport to the only local transport - // "transport-any:" is used for switching transport to the only transport - if (service.starts_with("transport")) { + LOG(DEBUG) << "handle_host_request(" << service << ")"; + + // Transport selection: + if (service.starts_with("transport") || service.starts_with("tport:")) { TransportType type = kTransportAny; std::string serial_storage; + bool legacy = true; - if (ConsumePrefix(&service, "transport-id:")) { - if (!ParseUint(&transport_id, service)) { - SendFail(reply_fd, "invalid transport id"); - return true; + // New transport selection protocol: + // This is essentially identical to the previous version, except it returns the selected + // transport id to the caller as well. + if (ConsumePrefix(&service, "tport:")) { + legacy = false; + if (ConsumePrefix(&service, "serial:")) { + serial_storage = service; + serial = serial_storage.c_str(); + } else if (service == "usb") { + type = kTransportUsb; + } else if (service == "local") { + type = kTransportLocal; + } else if (service == "any") { + type = kTransportAny; + } + + // Selection by id is unimplemented, since you obviously already know the transport id + // you're connecting to. + } else { + if (ConsumePrefix(&service, "transport-id:")) { + if (!ParseUint(&transport_id, service)) { + SendFail(reply_fd, "invalid transport id"); + return HostRequestResult::Handled; + } + } else if (service == "transport-usb") { + type = kTransportUsb; + } else if (service == "transport-local") { + type = kTransportLocal; + } else if (service == "transport-any") { + type = kTransportAny; + } else if (ConsumePrefix(&service, "transport:")) { + serial_storage = service; + serial = serial_storage.c_str(); } - } else if (service == "transport-usb") { - type = kTransportUsb; - } else if (service == "transport-local") { - type = kTransportLocal; - } else if (service == "transport-any") { - type = kTransportAny; - } else if (ConsumePrefix(&service, "transport:")) { - serial_storage = service; - serial = serial_storage.c_str(); } std::string error; @@ -1063,11 +1084,15 @@ bool handle_host_request(std::string_view service, TransportType type, const cha s->transport = t; SendOkay(reply_fd); - // We succesfully handled the device selection, but there's another request coming. - return false; + if (!legacy) { + // Nothing we can do if this fails. + WriteFdExactly(reply_fd, &t->id, sizeof(t->id)); + } + + return HostRequestResult::SwitchedTransport; } else { SendFail(reply_fd, error); - return true; + return HostRequestResult::Handled; } } @@ -1078,7 +1103,7 @@ bool handle_host_request(std::string_view service, TransportType type, const cha std::string device_list = list_transports(long_listing); D("Sending device list..."); SendOkay(reply_fd, device_list); - return true; + return HostRequestResult::Handled; } if (service == "reconnect-offline") { @@ -1094,7 +1119,7 @@ bool handle_host_request(std::string_view service, TransportType type, const cha response.resize(response.size() - 1); } SendOkay(reply_fd, response); - return true; + return HostRequestResult::Handled; } if (service == "features") { @@ -1105,7 +1130,7 @@ bool handle_host_request(std::string_view service, TransportType type, const cha } else { SendFail(reply_fd, error); } - return true; + return HostRequestResult::Handled; } if (service == "host-features") { @@ -1116,7 +1141,7 @@ bool handle_host_request(std::string_view service, TransportType type, const cha } features.insert(kFeaturePushSync); SendOkay(reply_fd, FeatureSetToString(features)); - return true; + return HostRequestResult::Handled; } // remove TCP transport @@ -1125,7 +1150,7 @@ bool handle_host_request(std::string_view service, TransportType type, const cha if (address.empty()) { kick_all_tcp_devices(); SendOkay(reply_fd, "disconnected everything"); - return true; + return HostRequestResult::Handled; } std::string serial; @@ -1137,22 +1162,22 @@ bool handle_host_request(std::string_view service, TransportType type, const cha } else if (!android::base::ParseNetAddress(address, &host, &port, &serial, &error)) { SendFail(reply_fd, android::base::StringPrintf("couldn't parse '%s': %s", address.c_str(), error.c_str())); - return true; + return HostRequestResult::Handled; } atransport* t = find_transport(serial.c_str()); if (t == nullptr) { SendFail(reply_fd, android::base::StringPrintf("no such device '%s'", serial.c_str())); - return true; + return HostRequestResult::Handled; } kick_transport(t); SendOkay(reply_fd, android::base::StringPrintf("disconnected %s", address.c_str())); - return true; + return HostRequestResult::Handled; } // Returns our value for ADB_SERVER_VERSION. if (service == "version") { SendOkay(reply_fd, android::base::StringPrintf("%04x", ADB_SERVER_VERSION)); - return true; + return HostRequestResult::Handled; } // These always report "unknown" rather than the actual error, for scripts. @@ -1164,7 +1189,7 @@ bool handle_host_request(std::string_view service, TransportType type, const cha } else { SendFail(reply_fd, error); } - return true; + return HostRequestResult::Handled; } if (service == "get-devpath") { std::string error; @@ -1174,7 +1199,7 @@ bool handle_host_request(std::string_view service, TransportType type, const cha } else { SendFail(reply_fd, error); } - return true; + return HostRequestResult::Handled; } if (service == "get-state") { std::string error; @@ -1184,7 +1209,7 @@ bool handle_host_request(std::string_view service, TransportType type, const cha } else { SendFail(reply_fd, error); } - return true; + return HostRequestResult::Handled; } // Indicates a new emulator instance has started. @@ -1197,7 +1222,7 @@ bool handle_host_request(std::string_view service, TransportType type, const cha } /* we don't even need to send a reply */ - return true; + return HostRequestResult::Handled; } if (service == "reconnect") { @@ -1209,7 +1234,7 @@ bool handle_host_request(std::string_view service, TransportType type, const cha "reconnecting " + t->serial_name() + " [" + t->connection_state_name() + "]\n"; } SendOkay(reply_fd, response); - return true; + return HostRequestResult::Handled; } // TODO: Switch handle_forward_request to string_view. @@ -1220,10 +1245,10 @@ bool handle_host_request(std::string_view service, TransportType type, const cha return acquire_one_transport(type, serial, transport_id, nullptr, error); }, reply_fd)) { - return true; + return HostRequestResult::Handled; } - return false; + return HostRequestResult::Unhandled; } static auto& init_mutex = *new std::mutex(); diff --git a/adb/adb.h b/adb/adb.h index f575adb47..5eea8bea7 100644 --- a/adb/adb.h +++ b/adb/adb.h @@ -219,8 +219,15 @@ extern const char* adb_device_banner; #define USB_FFS_ADB_IN USB_FFS_ADB_EP(ep2) #endif -bool handle_host_request(std::string_view service, TransportType type, const char* serial, - TransportId transport_id, int reply_fd, asocket* s); +enum class HostRequestResult { + Handled, + SwitchedTransport, + Unhandled, +}; + +HostRequestResult handle_host_request(std::string_view service, TransportType type, + const char* serial, TransportId transport_id, int reply_fd, + asocket* s); void handle_online(atransport* t); void handle_offline(atransport* t); diff --git a/adb/client/adb_client.cpp b/adb/client/adb_client.cpp index 0a09d1ee5..4cf3a743a 100644 --- a/adb/client/adb_client.cpp +++ b/adb/client/adb_client.cpp @@ -70,46 +70,60 @@ void adb_set_socket_spec(const char* socket_spec) { __adb_server_socket_spec = socket_spec; } -static int switch_socket_transport(int fd, std::string* error) { +static std::optional switch_socket_transport(int fd, std::string* error) { + TransportId result; + bool read_transport = true; + std::string service; if (__adb_transport_id) { + read_transport = false; service += "host:transport-id:"; service += std::to_string(__adb_transport_id); + result = __adb_transport_id; } else if (__adb_serial) { - service += "host:transport:"; + service += "host:tport:serial:"; service += __adb_serial; } else { const char* transport_type = "???"; switch (__adb_transport) { case kTransportUsb: - transport_type = "transport-usb"; - break; + transport_type = "usb"; + break; case kTransportLocal: - transport_type = "transport-local"; - break; + transport_type = "local"; + break; case kTransportAny: - transport_type = "transport-any"; - break; + transport_type = "any"; + break; case kTransportHost: // no switch necessary return 0; } - service += "host:"; + service += "host:tport:"; service += transport_type; } if (!SendProtocolString(fd, service)) { *error = perror_str("write failure during connection"); - return -1; + return std::nullopt; } - D("Switch transport in progress"); + + LOG(DEBUG) << "Switch transport in progress: " << service; if (!adb_status(fd, error)) { D("Switch transport failed: %s", error->c_str()); - return -1; + return std::nullopt; } + + if (read_transport) { + if (!ReadFdExactly(fd, &result, sizeof(result))) { + *error = "failed to read transport id from server"; + return std::nullopt; + } + } + D("Switch transport success"); - return 0; + return result; } bool adb_status(int fd, std::string* error) { @@ -133,11 +147,10 @@ bool adb_status(int fd, std::string* error) { return false; } -static int _adb_connect(const std::string& service, std::string* error) { - D("_adb_connect: %s", service.c_str()); +static int _adb_connect(std::string_view service, TransportId* transport, std::string* error) { + LOG(DEBUG) << "_adb_connect: " << service; if (service.empty() || service.size() > MAX_PAYLOAD) { - *error = android::base::StringPrintf("bad service name length (%zd)", - service.size()); + *error = android::base::StringPrintf("bad service name length (%zd)", service.size()); return -1; } @@ -149,8 +162,15 @@ static int _adb_connect(const std::string& service, std::string* error) { return -2; } - if (memcmp(&service[0], "host", 4) != 0 && switch_socket_transport(fd.get(), error)) { - return -1; + if (!service.starts_with("host")) { + std::optional transport_result = switch_socket_transport(fd.get(), error); + if (!transport_result) { + return -1; + } + + if (transport) { + *transport = *transport_result; + } } if (!SendProtocolString(fd.get(), service)) { @@ -190,11 +210,15 @@ bool adb_kill_server() { return true; } -int adb_connect(const std::string& service, std::string* error) { - // first query the adb server's version - unique_fd fd(_adb_connect("host:version", error)); +int adb_connect(std::string_view service, std::string* error) { + return adb_connect(nullptr, service, error); +} - D("adb_connect: service %s", service.c_str()); +int adb_connect(TransportId* transport, std::string_view service, std::string* error) { + // first query the adb server's version + unique_fd fd(_adb_connect("host:version", nullptr, error)); + + LOG(DEBUG) << "adb_connect: service: " << service; if (fd == -2 && !is_local_socket_spec(__adb_server_socket_spec)) { fprintf(stderr, "* cannot start server on remote host\n"); // error is the original network connection error @@ -216,7 +240,7 @@ int adb_connect(const std::string& service, std::string* error) { // Fall through to _adb_connect. } else { // If a server is already running, check its version matches. - int version = ADB_SERVER_VERSION - 1; + int version = 0; // If we have a file descriptor, then parse version result. if (fd >= 0) { @@ -254,7 +278,7 @@ int adb_connect(const std::string& service, std::string* error) { return 0; } - fd.reset(_adb_connect(service, error)); + fd.reset(_adb_connect(service, transport, error)); if (fd == -1) { D("_adb_connect error: %s", error->c_str()); } else if(fd == -2) { @@ -265,7 +289,6 @@ int adb_connect(const std::string& service, std::string* error) { return fd.release(); } - bool adb_command(const std::string& service) { std::string error; unique_fd fd(adb_connect(service, &error)); diff --git a/adb/client/adb_client.h b/adb/client/adb_client.h index d4675396f..0a7378770 100644 --- a/adb/client/adb_client.h +++ b/adb/client/adb_client.h @@ -24,7 +24,10 @@ // Connect to adb, connect to the named service, and return a valid fd for // interacting with that service upon success or a negative number on failure. -int adb_connect(const std::string& service, std::string* _Nonnull error); +int adb_connect(std::string_view service, std::string* _Nonnull error); + +// Same as above, except returning the TransportId for the service that we've connected to. +int adb_connect(TransportId* _Nullable id, std::string_view service, std::string* _Nonnull error); // Kill the currently running adb server, if it exists. bool adb_kill_server(); diff --git a/adb/sockets.cpp b/adb/sockets.cpp index 04d92db60..dc4402612 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -792,16 +792,22 @@ static int smart_socket_enqueue(asocket* s, apacket::payload_type data) { // Some requests are handled immediately -- in that case the handle_host_request() routine // has sent the OKAY or FAIL message and all we have to do is clean up. - if (handle_host_request(service, type, - serial.empty() ? nullptr : std::string(serial).c_str(), - transport_id, s->peer->fd, s)) { - LOG(VERBOSE) << "SS(" << s->id << "): handled host service '" << service << "'"; - goto fail; - } - if (service.starts_with("transport")) { - D("SS(%d): okay transport", s->id); - s->smart_socket_data.clear(); - return 0; + auto host_request_result = handle_host_request( + service, type, serial.empty() ? nullptr : std::string(serial).c_str(), transport_id, + s->peer->fd, s); + + switch (host_request_result) { + case HostRequestResult::Handled: + LOG(VERBOSE) << "SS(" << s->id << "): handled host service '" << service << "'"; + goto fail; + + case HostRequestResult::SwitchedTransport: + D("SS(%d): okay transport", s->id); + s->smart_socket_data.clear(); + return 0; + + case HostRequestResult::Unhandled: + break; } /* try to find a local service with this name. From 1e9e471c9cd8823617e28cc8745fc9d6f1d57b55 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 22 Feb 2019 14:01:36 -0800 Subject: [PATCH 08/12] adb: implement wait-for-disconnect. Bug: http://b/124244488 Test: manual Change-Id: I316a87994924c51c785e46a4900380c58e726985 --- adb/client/commandline.cpp | 9 +++++---- adb/services.cpp | 17 ++++++++++++++--- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/adb/client/commandline.cpp b/adb/client/commandline.cpp index f70b48009..1909de39f 100644 --- a/adb/client/commandline.cpp +++ b/adb/client/commandline.cpp @@ -190,8 +190,8 @@ static void help() { "scripting:\n" " wait-for[-TRANSPORT]-STATE\n" " wait for device to be in the given state\n" - " State: device, recovery, sideload, or bootloader\n" - " Transport: usb, local, or any [default=any]\n" + " STATE: device, recovery, sideload, bootloader, or disconnect\n" + " TRANSPORT: usb, local, or any [default=any]\n" " get-state print offline | bootloader | device\n" " get-serialno print \n" " get-devpath print \n" @@ -1031,10 +1031,11 @@ static bool wait_for_device(const char* service) { } if (components[3] != "any" && components[3] != "bootloader" && components[3] != "device" && - components[3] != "recovery" && components[3] != "sideload") { + components[3] != "recovery" && components[3] != "sideload" && + components[3] != "disconnect") { fprintf(stderr, "adb: unknown state %s; " - "expected 'any', 'bootloader', 'device', 'recovery', or 'sideload'\n", + "expected 'any', 'bootloader', 'device', 'recovery', 'sideload', or 'disconnect'\n", components[3].c_str()); return false; } diff --git a/adb/services.cpp b/adb/services.cpp index 1ed71823e..80f9f7925 100644 --- a/adb/services.cpp +++ b/adb/services.cpp @@ -109,12 +109,21 @@ static void wait_for_state(int fd, state_info* sinfo) { const char* serial = sinfo->serial.length() ? sinfo->serial.c_str() : nullptr; atransport* t = acquire_one_transport(sinfo->transport_type, serial, sinfo->transport_id, &is_ambiguous, &error); - if (t != nullptr && (sinfo->state == kCsAny || sinfo->state == t->GetConnectionState())) { + if (sinfo->state == kCsOffline) { + // wait-for-disconnect uses kCsOffline, we don't actually want to wait for 'offline'. + if (t == nullptr) { + SendOkay(fd); + break; + } + } else if (t != nullptr && + (sinfo->state == kCsAny || sinfo->state == t->GetConnectionState())) { SendOkay(fd); break; - } else if (!is_ambiguous) { + } + + if (!is_ambiguous) { adb_pollfd pfd = {.fd = fd, .events = POLLIN}; - int rc = adb_poll(&pfd, 1, 1000); + int rc = adb_poll(&pfd, 1, 100); if (rc < 0) { SendFail(fd, error); break; @@ -224,6 +233,8 @@ asocket* host_service_to_socket(std::string_view name, std::string_view serial, sinfo->state = kCsBootloader; } else if (name == "-any") { sinfo->state = kCsAny; + } else if (name == "-disconnect") { + sinfo->state = kCsOffline; } else { return nullptr; } From 2020dd6f2ef448a37322491848721086f2e7ec63 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 22 Feb 2019 15:20:27 -0800 Subject: [PATCH 09/12] adb: wait for device to disconnect upon `adb root`. Previously, there was a race where if adb root took longer than 3 seconds to take effect, we'd return early and allow subsequent commands to be targeted at the still-not-dead transport, and spuriously fail. Bug: http://b/124244488 Test: test_device.py Change-Id: I791a4f82946eb28e4d37729ab0ed3b7fc05b42a2 --- adb/client/commandline.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/adb/client/commandline.cpp b/adb/client/commandline.cpp index 1909de39f..32869590c 100644 --- a/adb/client/commandline.cpp +++ b/adb/client/commandline.cpp @@ -1047,7 +1047,8 @@ static bool wait_for_device(const char* service) { static bool adb_root(const char* command) { std::string error; - unique_fd fd(adb_connect(android::base::StringPrintf("%s:", command), &error)); + TransportId transport_id; + unique_fd fd(adb_connect(&transport_id, android::base::StringPrintf("%s:", command), &error)); if (fd < 0) { fprintf(stderr, "adb: unable to connect for %s: %s\n", command, error.c_str()); return false; @@ -1080,9 +1081,9 @@ static bool adb_root(const char* command) { return true; } - // Give adbd some time to kill itself and come back up. - // We can't use wait-for-device because devices (e.g. adb over network) might not come back. - std::this_thread::sleep_for(3s); + // Wait for the device to go away. + adb_set_transport(kTransportAny, nullptr, transport_id); + wait_for_device("wait-for-disconnect"); return true; } From aa4f31a1249c8df81faf42bce7eb5a5b82b50110 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Fri, 22 Feb 2019 15:26:15 -0800 Subject: [PATCH 10/12] adb: increment server version. Increment the server version for adb_connect with transport id, and wait-for-disconnect. Bug: http://b/124244488 Test: none Change-Id: Ib50d289b68fce4befbf1f5d9507d7e6f9cc1f4f5 --- adb/adb.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adb/adb.h b/adb/adb.h index 5eea8bea7..c60dcbc1c 100644 --- a/adb/adb.h +++ b/adb/adb.h @@ -59,7 +59,7 @@ constexpr size_t LINUX_MAX_SOCKET_SIZE = 4194304; std::string adb_version(); // Increment this when we want to force users to start a new adb server. -#define ADB_SERVER_VERSION 40 +#define ADB_SERVER_VERSION 41 using TransportId = uint64_t; class atransport; From ccc584523ac8f2d39ab73084026416edff96ad2e Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 25 Feb 2019 13:02:43 -0800 Subject: [PATCH 11/12] adbd: switch abb to ConsumePrefix. Test: mma Change-Id: I57d1a30a526c97c5b5a2718740b76220da6eea39 --- adb/daemon/abb.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/adb/daemon/abb.cpp b/adb/daemon/abb.cpp index 4ffa6bb85..eeac41a93 100644 --- a/adb/daemon/abb.cpp +++ b/adb/daemon/abb.cpp @@ -14,15 +14,15 @@ * limitations under the License. */ -#include "adb.h" -#include "adb_io.h" -#include "shell_service.h" - -#include "cmd.h" - #include #include +#include + +#include "adb.h" +#include "adb_io.h" +#include "adb_utils.h" +#include "shell_service.h" namespace { @@ -87,11 +87,9 @@ int main(int argc, char* const argv[]) { std::string_view name = data; auto protocol = SubprocessProtocol::kShell; - if (name.starts_with("abb:")) { - name.remove_prefix(strlen("abb:")); + if (ConsumePrefix(&name, "abb:")) { protocol = SubprocessProtocol::kShell; - } else if (name.starts_with("abb_exec:")) { - name.remove_prefix(strlen("abb_exec:")); + } else if (ConsumePrefix(&name, "abb_exec:")) { protocol = SubprocessProtocol::kNone; } else { LOG(FATAL) << "Unknown command prefix for abb: " << data; From 43f3805950d69f9da87664ada214d5af0e02753f Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 25 Feb 2019 13:29:36 -0800 Subject: [PATCH 12/12] adb: switch sockets.cpp to ConsumePrefix. Test: mma Change-Id: I86c3ec0fd90fe45e59c0187f664d46020bad2c0f --- adb/sockets.cpp | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/adb/sockets.cpp b/adb/sockets.cpp index dc4402612..8a2bf9a2c 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -755,34 +755,27 @@ static int smart_socket_enqueue(asocket* s, apacket::payload_type data) { #if ADB_HOST service = std::string_view(s->smart_socket_data).substr(4); - if (service.starts_with("host-serial:")) { - service.remove_prefix(strlen("host-serial:")); - + if (ConsumePrefix(&service, "host-serial:")) { // serial number should follow "host:" and could be a host:port string. if (!internal::parse_host_service(&serial, &service, service)) { LOG(ERROR) << "SS(" << s->id << "): failed to parse host service: " << service; goto fail; } - } else if (service.starts_with("host-transport-id:")) { - service.remove_prefix(strlen("host-transport-id:")); + } else if (ConsumePrefix(&service, "host-transport-id:")) { if (!ParseUint(&transport_id, service, &service)) { LOG(ERROR) << "SS(" << s->id << "): failed to parse host transport id: " << service; return -1; } - if (!service.starts_with(":")) { + if (!ConsumePrefix(&service, ":")) { LOG(ERROR) << "SS(" << s->id << "): host-transport-id without command"; return -1; } - service.remove_prefix(1); - } else if (service.starts_with("host-usb:")) { + } else if (ConsumePrefix(&service, "host-usb:")) { type = kTransportUsb; - service.remove_prefix(strlen("host-usb:")); - } else if (service.starts_with("host-local:")) { + } else if (ConsumePrefix(&service, "host-local:")) { type = kTransportLocal; - service.remove_prefix(strlen("host-local:")); - } else if (service.starts_with("host:")) { + } else if (ConsumePrefix(&service, "host:")) { type = kTransportAny; - service.remove_prefix(strlen("host:")); } else { service = std::string_view{}; }