From d1f7fef69b8a65195f104eff8051fc808a5b5706 Mon Sep 17 00:00:00 2001
From: Josh Gao <jmgao@google.com>
Date: Wed, 7 Jun 2017 18:18:36 -0700
Subject: [PATCH] adb: fix deadlock between transport_unref and usb_close.

Fix a deadlock that happened when a reader/writer thread released a
transport while the hotplug thread attempted  to handle a device
disconnection. Decrementing a transport refcount to zero would hold the
global transport mutex and attempt to take the usb handles mutex, while
the hotplug thread would hold the usb handles mutex and try to call
unregister_usb_transport, which would attempt to take the global
transport mutex.

Resolve this by making transport_unref not take the global transport
mutex.

Bug: http://b/62423753
Test: python test_device.py
Merged-In: Ib48b80a2091a254527f3a7d945b6a11fae61f937
Change-Id: Ib48b80a2091a254527f3a7d945b6a11fae61f937
(cherry picked from commit 7e197ef83307ffcf2f55d1b8a603d0d3bd2e1d4d)
---
 adb/transport.cpp | 10 +++++-----
 adb/transport.h   |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/adb/transport.cpp b/adb/transport.cpp
index 308ee8db7..2bbbefdd4 100644
--- a/adb/transport.cpp
+++ b/adb/transport.cpp
@@ -603,15 +603,15 @@ static void remove_transport(atransport* transport) {
 static void transport_unref(atransport* t) {
     CHECK(t != nullptr);
 
-    std::lock_guard<std::mutex> lock(transport_lock);
-    CHECK_GT(t->ref_count, 0u);
-    t->ref_count--;
-    if (t->ref_count == 0) {
+    size_t old_refcount = t->ref_count--;
+    CHECK_GT(old_refcount, 0u);
+
+    if (old_refcount == 1u) {
         D("transport: %s unref (kicking and closing)", t->serial);
         t->close(t);
         remove_transport(t);
     } else {
-        D("transport: %s unref (count=%zu)", t->serial, t->ref_count);
+        D("transport: %s unref (count=%zu)", t->serial, old_refcount - 1);
     }
 }
 
diff --git a/adb/transport.h b/adb/transport.h
index 57fc9889a..4a89ed993 100644
--- a/adb/transport.h
+++ b/adb/transport.h
@@ -61,7 +61,7 @@ public:
     // class in one go is a very large change. Given how bad our testing is,
     // it's better to do this piece by piece.
 
-    atransport(ConnectionState state = kCsOffline) : connection_state_(state) {
+    atransport(ConnectionState state = kCsOffline) : ref_count(0), connection_state_(state) {
         transport_fde = {};
         protocol_version = A_VERSION;
         max_payload = MAX_PAYLOAD;
@@ -88,7 +88,7 @@ public:
     int fd = -1;
     int transport_socket = -1;
     fdevent transport_fde;
-    size_t ref_count = 0;
+    std::atomic<size_t> ref_count;
     uint32_t sync_token = 0;
     bool online = false;
     TransportType type = kTransportAny;