ACPR/android_system_core
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Revert "adb: fix deadlock between transport_unref and usb_close."

Browse source 
This reverts commit 7e197ef833.

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 e48ecce671)
This commit is contained in:
Josh Gao 2017-09-13 13:40:57 -07:00
parent 1bd8498fc8
commit c78ecca70b
3 changed files with 13 additions and 8 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
adb/client/usb_libusb.cpp
View file

@ -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.
}

10
adb/transport.cpp
View file

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

4
adb/transport.h
View file

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