commit 09ebf055449c79a2526ad64efa08572956e0d4b2
author Josh Gao <jmgao@google.com> Thu Aug 17 13:50:51 2017 -0700
committer Josh Gao <jmgao@google.com> Wed Sep 27 14:35:52 2017 -0700
tree faad3046e04f11ea0a466d1b9cbb6dc3fa52a21d
parent 1f8ec6f776baf50a432ca06a892e72fe950aaafc

adb: fix deadlock in kick_all_transports.

Previously, kick_all_transports would deadlock if there were any
inaccessible transports, because the transport kick function would call
unregister_usb_transport, which attempts to take the already-held
transport lock.

Fix this by switching the transport lock over to a recursive mutex.

Bug: 65419665
Test: manual
Change-Id: If61296ff4745e1699f3e216811c1383582627604
(cherry picked from commit 1db71afe97c0c73167bc5d1fffea574b65bfa32c)

adb/transport.cpp

diff --git a/adb/transport.cpp b/adb/transport.cpp
index 2bbbefd..c26252e 100644
--- a/adb/transport.cpp
+++ b/adb/transport.cpp
@@ -49,7 +49,7 @@
 static auto& transport_list = *new std::list<atransport*>();
 static auto& pending_list = *new std::list<atransport*>();
 
-static std::mutex& transport_lock = *new std::mutex();
+static auto& transport_lock = *new std::recursive_mutex();
 
 const char* const kFeatureShell2 = "shell_v2";
 const char* const kFeatureCmd = "cmd";
@@ -298,7 +298,7 @@
 }
 
 void kick_transport(atransport* t) {
-    std::lock_guard<std::mutex> lock(transport_lock);
+    std::lock_guard<std::recursive_mutex> lock(transport_lock);
     // As kick_transport() can be called from threads without guarantee that t is valid,
     // check if the transport is in transport_list first.
     if (std::find(transport_list.begin(), transport_list.end(), t) != transport_list.end()) {
@@ -330,7 +330,7 @@
     device_tracker** pnode = &device_tracker_list;
     device_tracker* node = *pnode;
 
-    std::lock_guard<std::mutex> lock(transport_lock);
+    std::lock_guard<std::recursive_mutex> lock(transport_lock);
     while (node) {
         if (node == tracker) {
             *pnode = node->next;
@@ -403,7 +403,7 @@
 
 // Check if all of the USB transports are connected.
 bool iterate_transports(std::function<bool(const atransport*)> fn) {
-    std::lock_guard<std::mutex> lock(transport_lock);
+    std::lock_guard<std::recursive_mutex> lock(transport_lock);
     for (const auto& t : transport_list) {
         if (!fn(t)) {
             return false;
@@ -507,7 +507,7 @@
         adb_close(t->fd);
 
         {
-            std::lock_guard<std::mutex> lock(transport_lock);
+            std::lock_guard<std::recursive_mutex> lock(transport_lock);
             transport_list.remove(t);
         }
 
@@ -546,7 +546,7 @@
     }
 
     {
-        std::lock_guard<std::mutex> lock(transport_lock);
+        std::lock_guard<std::recursive_mutex> lock(transport_lock);
         pending_list.remove(t);
         transport_list.push_front(t);
     }
@@ -573,7 +573,7 @@
 
 void kick_all_transports() {
     // To avoid only writing part of a packet to a transport after exit, kick all transports.
-    std::lock_guard<std::mutex> lock(transport_lock);
+    std::lock_guard<std::recursive_mutex> lock(transport_lock);
     for (auto t : transport_list) {
         t->Kick();
     }
@@ -652,7 +652,7 @@
         *error_out = "no devices found";
     }
 
-    std::unique_lock<std::mutex> lock(transport_lock);
+    std::unique_lock<std::recursive_mutex> lock(transport_lock);
     for (const auto& t : transport_list) {
         if (t->GetConnectionState() == kCsNoPerm) {
 #if ADB_HOST
@@ -927,7 +927,7 @@
 std::string list_transports(bool long_listing) {
     std::string result;
 
-    std::lock_guard<std::mutex> lock(transport_lock);
+    std::lock_guard<std::recursive_mutex> lock(transport_lock);
     for (const auto& t : transport_list) {
         append_transport(t, &result, long_listing);
     }
@@ -935,7 +935,7 @@
 }
 
 void close_usb_devices(std::function<bool(const atransport*)> predicate) {
-    std::lock_guard<std::mutex> lock(transport_lock);
+    std::lock_guard<std::recursive_mutex> lock(transport_lock);
     for (auto& t : transport_list) {
         if (predicate(t)) {
             t->Kick();
@@ -964,7 +964,7 @@
         return -1;
     }
 
-    std::unique_lock<std::mutex> lock(transport_lock);
+    std::unique_lock<std::recursive_mutex> lock(transport_lock);
     for (const auto& transport : pending_list) {
         if (transport->serial && strcmp(serial, transport->serial) == 0) {
             VLOG(TRANSPORT) << "socket transport " << transport->serial
@@ -996,7 +996,7 @@
 atransport* find_transport(const char* serial) {
     atransport* result = nullptr;
 
-    std::lock_guard<std::mutex> lock(transport_lock);
+    std::lock_guard<std::recursive_mutex> lock(transport_lock);
     for (auto& t : transport_list) {
         if (t->serial && strcmp(serial, t->serial) == 0) {
             result = t;
@@ -1008,7 +1008,7 @@
 }
 
 void kick_all_tcp_devices() {
-    std::lock_guard<std::mutex> lock(transport_lock);
+    std::lock_guard<std::recursive_mutex> lock(transport_lock);
     for (auto& t : transport_list) {
         if (t->IsTcpDevice()) {
             // Kicking breaks the read_transport thread of this transport out of any read, then
@@ -1037,7 +1037,7 @@
     }
 
     {
-        std::lock_guard<std::mutex> lock(transport_lock);
+        std::lock_guard<std::recursive_mutex> lock(transport_lock);
         pending_list.push_front(t);
     }
 
@@ -1046,7 +1046,7 @@
 
 // This should only be used for transports with connection_state == kCsNoPerm.
 void unregister_usb_transport(usb_handle* usb) {
-    std::lock_guard<std::mutex> lock(transport_lock);
+    std::lock_guard<std::recursive_mutex> lock(transport_lock);
     transport_list.remove_if(
         [usb](atransport* t) { return t->usb == usb && t->GetConnectionState() == kCsNoPerm; });
 }