From c78ecca70b44be14b06f526f17db199773ad160c Mon Sep 17 00:00:00 2001
From: Josh Gao <jmgao@google.com>
Date: Wed, 13 Sep 2017 13:40:57 -0700
Subject: [PATCH] Revert "adb: fix deadlock between transport_unref and
 usb_close."

This reverts commit 7e197ef83307ffcf2f55d1b8a603d0d3bd2e1d4d.

The mutex lock in transport_unref hides a race that seems otherwise
hard to fix. Specifically, there's no synchronization between acquiring
a transport and attaching it to an asocket*, leading to badness if the
transport is closed in between the two operations.

Fix the original problem the reverted patch addressed by manually
unlocking before calling unregister_usb_transport.

Bug: http://b/65419665
Bug: 64709603 (presubmit balking at the line above)
Test: python test_device.py
Change-Id: I0ed0044129b1671b2c5dd1b9fa2e70a9b4475dc5
(cherry picked from commit e48ecce671a2ab6647cc26a3cb11163aec0b712c)
---
 adb/client/usb_libusb.cpp |  7 ++++++-
 adb/transport.cpp         | 10 +++++-----
 adb/transport.h           |  4 ++--
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/adb/client/usb_libusb.cpp b/adb/client/usb_libusb.cpp
index e7f44c685..7025f283c 100644
--- a/adb/client/usb_libusb.cpp
+++ b/adb/client/usb_libusb.cpp
@@ -412,8 +412,13 @@ static void device_disconnected(libusb_device* device) {
     if (it != usb_handles.end()) {
         if (!it->second->device_handle) {
             // If the handle is null, we were never able to open the device.
-            unregister_usb_transport(it->second.get());
+
+            // Temporarily release the usb handles mutex to avoid deadlock.
+            std::unique_ptr<usb_handle> handle = std::move(it->second);
             usb_handles.erase(it);
+            lock.unlock();
+            unregister_usb_transport(handle.get());
+            lock.lock();
         } else {
             // Closure of the transport will erase the usb_handle.
         }
diff --git a/adb/transport.cpp b/adb/transport.cpp
index 91d708f6a..3cfb3f330 100644
--- a/adb/transport.cpp
+++ b/adb/transport.cpp
@@ -613,15 +613,15 @@ static void remove_transport(atransport* transport) {
 static void transport_unref(atransport* t) {
     CHECK(t != nullptr);
 
-    size_t old_refcount = t->ref_count--;
-    CHECK_GT(old_refcount, 0u);
-
-    if (old_refcount == 1u) {
+    std::lock_guard<std::recursive_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)", t->serial);
         t->close(t);
         remove_transport(t);
     } else {
-        D("transport: %s unref (count=%zu)", t->serial, old_refcount - 1);
+        D("transport: %s unref (count=%zu)", t->serial, t->ref_count);
     }
 }
 
diff --git a/adb/transport.h b/adb/transport.h
index 238e959f2..dee27e16f 100644
--- a/adb/transport.h
+++ b/adb/transport.h
@@ -64,7 +64,7 @@ class atransport {
     // it's better to do this piece by piece.
 
     atransport(ConnectionState state = kCsOffline)
-        : id(NextTransportId()), ref_count(0), connection_state_(state) {
+        : id(NextTransportId()), connection_state_(state) {
         transport_fde = {};
         protocol_version = A_VERSION;
         max_payload = MAX_PAYLOAD;
@@ -88,7 +88,7 @@ class atransport {
     int fd = -1;
     int transport_socket = -1;
     fdevent transport_fde;
-    std::atomic<size_t> ref_count;
+    size_t ref_count = 0;
     uint32_t sync_token = 0;
     bool online = false;
     TransportType type = kTransportAny;