Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e48ecce6 authored by Josh Gao's avatar Josh Gao
Browse files

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

This reverts commit 7e197ef8.

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
Test: python test_device.py
Change-Id: I0ed0044129b1671b2c5dd1b9fa2e70a9b4475dc5
parent 62c92f0c

adb/client/usb_libusb.cpp

+6 −1
Original line number Diff line number Diff line
@@ -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.

        }

adb/transport.cpp

+5 −5
Original line number Diff line number Diff line
@@ -615,15 +615,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);

    }

}

adb/transport.h

+2 −2
Original line number Diff line number Diff line
@@ -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;