commit c78ecca70b44be14b06f526f17db199773ad160c
author Josh Gao <jmgao@google.com> Wed Sep 13 13:40:57 2017 -0700
committer Josh Gao <jmgao@google.com> Wed Sep 27 14:36:08 2017 -0700
tree 49193de964c49fba4f05f740d46be37b3cb404ac
parent 1bd8498fc89d14b98e64142e07ad0c35c3bbffed

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

diff --git a/adb/client/usb_libusb.cpp b/adb/client/usb_libusb.cpp
index e7f44c6..7025f28 100644
--- a/adb/client/usb_libusb.cpp
+++ b/adb/client/usb_libusb.cpp
@@ -412,8 +412,13 @@
     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.
         }

adb/transport.cpp

diff --git a/adb/transport.cpp b/adb/transport.cpp
index 91d708f..3cfb3f3 100644
--- a/adb/transport.cpp
+++ b/adb/transport.cpp
@@ -613,15 +613,15 @@
 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);
     }
 }
 

adb/transport.h

diff --git a/adb/transport.h b/adb/transport.h
index 238e959..dee27e1 100644
--- a/adb/transport.h
+++ b/adb/transport.h
@@ -64,7 +64,7 @@
     // 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 @@
     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;