Merge "adb: disconnect: fix write-after-free memory corruption and crash."

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
+            kick_all_tcp_devices();
-            unregister_all_tcp_transports();
             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()));
     }
 

adb/transport.cpp

@@ -28,6 +28,7 @@
 
 #include <list>
 
+#include <base/logging.h>
 #include <base/stringprintf.h>
 #include <base/strings.h>
 
@@ -41,23 +42,6 @@ static std::list<atransport*> 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) {
+        kick_transport_locked(t);
-            t->kicked = 1;
-            t->kick(t);
-        }
         t->close(t);
         remove_transport(t);
     } else {
-        D("transport: %s unref (count=%d)\n", t->serial, t->ref_count);
+        D("transport: %s unref (count=%zu)\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);
     }
+    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);
+    for (auto& t : transport_list) {
-    adb_mutex_unlock(&transport_lock);
+        // TCP/IP devices have adb_port == 0.
-
-    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;
         if (t->type == kTransportLocal && t->adb_port == 0) {
-            // We cannot call kick_transport when holding transport_lock.
+            // Kicking breaks the output thread of this transport out of any read, then
-            if (!t->kicked) {
+            // the output thread will notify the main thread to make this transport
-                t->kicked = 1;
+            // offline. Then the main thread will notify the input thread to exit.
-                t->kick(t);
+            // Finally, this transport will be closed and freed in the main thread.
-            }
+            kick_transport_locked(t);
-            transport_unref_locked(t);
-
-            it = transport_list.erase(it);
-        } else {
-            ++it;
         }
     }
-
     adb_mutex_unlock(&transport_lock);
 }
 

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);