From 55c8b34f144ab37f0e8a9df4b2399b74f155408f Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Tue, 12 Sep 2017 13:23:33 -0700 Subject: [PATCH 1/3] adb: reformat comments. Test: none Change-Id: Ib484f701f66cdb57f303dbd6d5eb2e5a15abdc0e --- adb/socket.h | 105 +++++++++++++++++++++++++-------------------------- 1 file changed, 52 insertions(+), 53 deletions(-) diff --git a/adb/socket.h b/adb/socket.h index 4acdf4a6d..64d05a92a 100644 --- a/adb/socket.h +++ b/adb/socket.h @@ -19,84 +19,83 @@ #include +#include + #include "fdevent.h" struct apacket; class atransport; /* An asocket represents one half of a connection between a local and -** remote entity. A local asocket is bound to a file descriptor. A -** remote asocket is bound to the protocol engine. -*/ + * remote entity. A local asocket is bound to a file descriptor. A + * remote asocket is bound to the protocol engine. + */ struct asocket { - /* chain pointers for the local/remote list of - ** asockets that this asocket lives in - */ - asocket *next; - asocket *prev; + /* chain pointers for the local/remote list of + * asockets that this asocket lives in + */ + asocket* next; + asocket* prev; - /* the unique identifier for this asocket - */ + /* the unique identifier for this asocket + */ unsigned id; - /* flag: set when the socket's peer has closed - ** but packets are still queued for delivery - */ - int closing; + /* flag: set when the socket's peer has closed + * but packets are still queued for delivery + */ + int closing; // flag: set when the socket failed to write, so the socket will not wait to // write packets and close directly. bool has_write_error; - /* flag: quit adbd when both ends close the - ** local service socket - */ - int exit_on_close; + /* flag: quit adbd when both ends close the + * local service socket + */ + int exit_on_close; - /* the asocket we are connected to - */ + // the asocket we are connected to + asocket* peer; - asocket *peer; - - /* For local asockets, the fde is used to bind - ** us to our fd event system. For remote asockets - ** these fields are not used. - */ + /* For local asockets, the fde is used to bind + * us to our fd event system. For remote asockets + * these fields are not used. + */ fdevent fde; int fd; - /* queue of apackets waiting to be written - */ - apacket *pkt_first; - apacket *pkt_last; + // queue of apackets waiting to be written + apacket* pkt_first; + apacket* pkt_last; - /* enqueue is called by our peer when it has data - ** for us. It should return 0 if we can accept more - ** data or 1 if not. If we return 1, we must call - ** peer->ready() when we once again are ready to - ** receive data. - */ - int (*enqueue)(asocket *s, apacket *pkt); + /* enqueue is called by our peer when it has data + * for us. It should return 0 if we can accept more + * data or 1 if not. If we return 1, we must call + * peer->ready() when we once again are ready to + * receive data. + */ + int (*enqueue)(asocket* s, apacket* pkt); - /* ready is called by the peer when it is ready for - ** us to send data via enqueue again - */ - void (*ready)(asocket *s); + /* ready is called by the peer when it is ready for + * us to send data via enqueue again + */ + void (*ready)(asocket* s); - /* shutdown is called by the peer before it goes away. - ** the socket should not do any further calls on its peer. - ** Always followed by a call to close. Optional, i.e. can be NULL. - */ - void (*shutdown)(asocket *s); + /* shutdown is called by the peer before it goes away. + * the socket should not do any further calls on its peer. + * Always followed by a call to close. Optional, i.e. can be NULL. + */ + void (*shutdown)(asocket* s); - /* close is called by the peer when it has gone away. - ** we are not allowed to make any further calls on the - ** peer once our close method is called. - */ - void (*close)(asocket *s); + /* close is called by the peer when it has gone away. + * we are not allowed to make any further calls on the + * peer once our close method is called. + */ + void (*close)(asocket* s); - /* A socket is bound to atransport */ - atransport *transport; + /* A socket is bound to atransport */ + atransport* transport; size_t get_max_payload() const; }; From 62c92f0c0529d8d1817afb4ff0c83151e4bb0ea0 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 13 Sep 2017 11:17:33 -0700 Subject: [PATCH 2/3] adb: add lock to remove_socket. The comment that was previously here says that local_socket_list_lock must be taken, but this function is exposed to external callers that can't possibly take the lock. Bug: http://b/65419665 Test: python test_device.py Change-Id: I12d464933936b2a210a827ccf19ea201020d8d78 --- adb/sockets.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/adb/sockets.cpp b/adb/sockets.cpp index f28a3df50..c53fbb4ff 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -96,7 +96,7 @@ void install_local_socket(asocket* s) { } void remove_socket(asocket* s) { - // socket_list_lock should already be held + std::lock_guard lock(local_socket_list_lock); if (s->prev && s->next) { s->prev->next = s->next; s->next->prev = s->prev; From e48ecce671a2ab6647cc26a3cb11163aec0b712c Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 13 Sep 2017 13:40:57 -0700 Subject: [PATCH 3/3] 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 Test: python test_device.py Change-Id: I0ed0044129b1671b2c5dd1b9fa2e70a9b4475dc5 --- 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 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 b2e03a057..1b597fdd8 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -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 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 00fad5646..374bfc30d 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 ref_count; + size_t ref_count = 0; uint32_t sync_token = 0; bool online = false; TransportType type = kTransportAny;