From f4b9928563e97620fc1d9bd5c2efdaa0ded96488 Mon Sep 17 00:00:00 2001 From: Yabin Cui Date: Thu, 27 Aug 2015 12:03:11 -0700 Subject: [PATCH] adb: disconnect: fix write-after-free memory corruption and crash. Transport atransport objects are semi-reference counted: the input and output threads each hold a reference. The adb disconnect command was calling transport_unref to release a reference that it never had in the first place. This meant that the refcount dropped to zero and the object was deleted before either the input or output thread released its reference. When that last thread released its reference, it wrote to freed memory and also sometimes crashed. This fix is to not release any unheld reference, instead it just kicks the transport to break remote_read in output_thread. So all transport close flow goes the following way: output_thread (exit) -> main thread (offline the transport) -> input thread (exit) -> main thread (destroy the transport) Change-Id: Iad1fe718acc8716f3a79c8c22b426a1b2450452c --- adb/adb.cpp | 5 ++- adb/transport.cpp | 89 ++++++++++++++++------------------------------- adb/transport.h | 10 ++---- 3 files changed, 34 insertions(+), 70 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index a0501a677..d6e6d91f2 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -972,8 +972,7 @@ int handle_host_request(const char* service, TransportType type, if (!strncmp(service, "disconnect:", 11)) { const std::string address(service + 11); if (address.empty()) { - // disconnect from all TCP devices - unregister_all_tcp_transports(); + kick_all_tcp_devices(); return SendOkay(reply_fd, "disconnected everything"); } @@ -990,7 +989,7 @@ int handle_host_request(const char* service, TransportType type, return SendFail(reply_fd, android::base::StringPrintf("no such device '%s'", serial.c_str())); } - unregister_transport(t); + kick_transport(t); return SendOkay(reply_fd, android::base::StringPrintf("disconnected %s", address.c_str())); } diff --git a/adb/transport.cpp b/adb/transport.cpp index 20d5d7139..46c709f7e 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -28,6 +28,7 @@ #include +#include #include #include @@ -41,23 +42,6 @@ static std::list pending_list; ADB_MUTEX_DEFINE( transport_lock ); -void kick_transport(atransport* t) -{ - if (t != nullptr) - { - int kicked; - - adb_mutex_lock(&transport_lock); - kicked = t->kicked; - if (!kicked) - t->kicked = 1; - adb_mutex_unlock(&transport_lock); - - if (!kicked) - t->kick(t); - } -} - // Each atransport contains a list of adisconnects (t->disconnects). // An adisconnect contains a link to the next/prev adisconnect, a function // pointer to a disconnect callback which takes a void* piece of user data and @@ -336,6 +320,19 @@ static void *input_thread(void *_t) return 0; } +static void kick_transport_locked(atransport* t) { + CHECK(t != nullptr); + if (!t->kicked) { + t->kicked = true; + t->kick(t); + } +} + +void kick_transport(atransport* t) { + adb_mutex_lock(&transport_lock); + kick_transport_locked(t); + adb_mutex_unlock(&transport_lock); +} static int transport_registration_send = -1; static int transport_registration_recv = -1; @@ -642,29 +639,20 @@ static void remove_transport(atransport *transport) } -static void transport_unref_locked(atransport *t) -{ +static void transport_unref(atransport* t) { + CHECK(t != nullptr); + adb_mutex_lock(&transport_lock); + CHECK_GT(t->ref_count, 0u); t->ref_count--; if (t->ref_count == 0) { D("transport: %s unref (kicking and closing)\n", t->serial); - if (!t->kicked) { - t->kicked = 1; - t->kick(t); - } + kick_transport_locked(t); t->close(t); remove_transport(t); } else { - D("transport: %s unref (count=%d)\n", t->serial, t->ref_count); - } -} - -static void transport_unref(atransport *t) -{ - if (t) { - adb_mutex_lock(&transport_lock); - transport_unref_locked(t); - adb_mutex_unlock(&transport_lock); + D("transport: %s unref (count=%zu)\n", t->serial, t->ref_count); } + adb_mutex_unlock(&transport_lock); } void add_transport_disconnect(atransport* t, adisconnect* dis) @@ -967,7 +955,7 @@ atransport *find_transport(const char *serial) { atransport* result = nullptr; adb_mutex_lock(&transport_lock); - for (auto t : transport_list) { + for (auto& t : transport_list) { if (t->serial && strcmp(serial, t->serial) == 0) { result = t; break; @@ -978,35 +966,18 @@ atransport *find_transport(const char *serial) { return result; } -void unregister_transport(atransport *t) -{ +void kick_all_tcp_devices() { adb_mutex_lock(&transport_lock); - transport_list.remove(t); - adb_mutex_unlock(&transport_lock); - - kick_transport(t); - transport_unref(t); -} - -// Unregisters all non-emulator TCP transports. -void unregister_all_tcp_transports() { - adb_mutex_lock(&transport_lock); - for (auto it = transport_list.begin(); it != transport_list.end(); ) { - atransport* t = *it; + for (auto& t : transport_list) { + // TCP/IP devices have adb_port == 0. if (t->type == kTransportLocal && t->adb_port == 0) { - // We cannot call kick_transport when holding transport_lock. - if (!t->kicked) { - t->kicked = 1; - t->kick(t); - } - transport_unref_locked(t); - - it = transport_list.erase(it); - } else { - ++it; + // Kicking breaks the output thread of this transport out of any read, then + // the output thread will notify the main thread to make this transport + // offline. Then the main thread will notify the input thread to exit. + // Finally, this transport will be closed and freed in the main thread. + kick_transport_locked(t); } } - adb_mutex_unlock(&transport_lock); } diff --git a/adb/transport.h b/adb/transport.h index e809407c5..abb26a7d9 100644 --- a/adb/transport.h +++ b/adb/transport.h @@ -52,7 +52,7 @@ public: int fd = -1; int transport_socket = -1; fdevent transport_fde; - int ref_count = 0; + size_t ref_count = 0; uint32_t sync_token = 0; ConnectionState connection_state = kCsOffline; bool online = false; @@ -120,12 +120,10 @@ void kick_transport(atransport* t); void run_transport_disconnects(atransport* t); void update_transports(void); -/* transports are ref-counted -** get_device_transport does an acquire on your behalf before returning -*/ void init_transport_registration(void); std::string list_transports(bool long_listing); atransport* find_transport(const char* serial); +void kick_all_tcp_devices(); void register_usb_transport(usb_handle* h, const char* serial, const char* devpath, unsigned writeable); @@ -136,10 +134,6 @@ int register_socket_transport(int s, const char* serial, int port, int local); // This should only be used for transports with connection_state == kCsNoPerm. void unregister_usb_transport(usb_handle* usb); -/* these should only be used for the "adb disconnect" command */ -void unregister_transport(atransport* t); -void unregister_all_tcp_transports(); - int check_header(apacket* p, atransport* t); int check_data(apacket* p);