Clean up the locking in usb_linux.cpp.
tsan complained that usb_bulk_write accesses usb_handle members outside
a lock. Fix that, but by moving everything over to C++11 locking.
Note that the old code was checking whether pthread_cond_timedwait returned
a negative value, which it will never do --- it will signal timeout (or
any other error) by returning a positive errno value. The rewrite does
what they appeared to intend to do (break out on timeout), rather than
what they actually did (keep trying forever).
Bug: http://b/22598587
Change-Id: Iab6869ffed4874143a7da97193d6b09e34cf2933
diff --git a/adb/Android.mk b/adb/Android.mk
index 7977009..4f75b43 100644
--- a/adb/Android.mk
+++ b/adb/Android.mk
@@ -47,6 +47,7 @@
LIBADB_CFLAGS := \
$(ADB_COMMON_CFLAGS) \
-fvisibility=hidden \
+ -std=c++14 \
LIBADB_darwin_SRC_FILES := \
fdevent.cpp \
diff --git a/adb/usb_linux.cpp b/adb/usb_linux.cpp
index 439826a..e570ef5 100644
--- a/adb/usb_linux.cpp
+++ b/adb/usb_linux.cpp
@@ -22,6 +22,7 @@
#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
+#include <linux/usb/ch9.h>
#include <linux/usbdevice_fs.h>
#include <linux/version.h>
#include <stdio.h>
@@ -31,7 +32,12 @@
#include <sys/time.h>
#include <sys/types.h>
#include <unistd.h>
-#include <linux/usb/ch9.h>
+
+#include <chrono>
+#include <condition_variable>
+#include <list>
+#include <mutex>
+#include <string>
#include <base/file.h>
#include <base/stringprintf.h>
@@ -40,110 +46,92 @@
#include "adb.h"
#include "transport.h"
+using namespace std::literals;
+
/* usb scan debugging is waaaay too verbose */
#define DBGX(x...)
-ADB_MUTEX_DEFINE( usb_lock );
+struct usb_handle {
+ ~usb_handle() {
+ if (fd != -1) unix_close(fd);
+ }
-struct usb_handle
-{
- usb_handle *prev;
- usb_handle *next;
-
- char fname[64];
- int desc;
+ std::string path;
+ int fd = -1;
unsigned char ep_in;
unsigned char ep_out;
unsigned zero_mask;
- unsigned writeable;
+ unsigned writeable = 1;
- struct usbdevfs_urb urb_in;
- struct usbdevfs_urb urb_out;
+ usbdevfs_urb urb_in;
+ usbdevfs_urb urb_out;
- int urb_in_busy;
- int urb_out_busy;
- int dead;
+ bool urb_in_busy = false;
+ bool urb_out_busy = false;
+ bool dead = false;
- adb_cond_t notify;
- adb_mutex_t lock;
+ std::condition_variable cv;
+ std::mutex mutex;
// for garbage collecting disconnected devices
- int mark;
+ bool mark;
// ID of thread currently in REAPURB
- pthread_t reaper_thread;
+ pthread_t reaper_thread = 0;
};
-static usb_handle handle_list = {
- .prev = &handle_list,
- .next = &handle_list,
-};
+static std::mutex g_usb_handles_mutex;
+static std::list<usb_handle*> g_usb_handles;
-static int known_device(const char *dev_name)
-{
- usb_handle *usb;
-
- adb_mutex_lock(&usb_lock);
- for(usb = handle_list.next; usb != &handle_list; usb = usb->next){
- if(!strcmp(usb->fname, dev_name)) {
+static int is_known_device(const char* dev_name) {
+ std::lock_guard<std::mutex> lock(g_usb_handles_mutex);
+ for (usb_handle* usb : g_usb_handles) {
+ if (usb->path == dev_name) {
// set mark flag to indicate this device is still alive
- usb->mark = 1;
- adb_mutex_unlock(&usb_lock);
+ usb->mark = true;
return 1;
}
}
- adb_mutex_unlock(&usb_lock);
return 0;
}
-static void kick_disconnected_devices()
-{
- usb_handle *usb;
-
- adb_mutex_lock(&usb_lock);
+static void kick_disconnected_devices() {
+ std::lock_guard<std::mutex> lock(g_usb_handles_mutex);
// kick any devices in the device list that were not found in the device scan
- for(usb = handle_list.next; usb != &handle_list; usb = usb->next){
- if (usb->mark == 0) {
+ for (usb_handle* usb : g_usb_handles) {
+ if (!usb->mark) {
usb_kick(usb);
} else {
- usb->mark = 0;
+ usb->mark = false;
}
}
- adb_mutex_unlock(&usb_lock);
-
}
-static inline int badname(const char *name)
-{
- while(*name) {
- if(!isdigit(*name++)) return 1;
+static inline bool contains_non_digit(const char* name) {
+ while (*name) {
+ if (!isdigit(*name++)) return true;
}
- return 0;
+ return false;
}
-static void find_usb_device(const char *base,
+static void find_usb_device(const std::string& base,
void (*register_device_callback)
- (const char *, const char *, unsigned char, unsigned char, int, int, unsigned))
+ (const char*, const char*, unsigned char, unsigned char, int, int, unsigned))
{
- char busname[32], devname[32];
- unsigned char local_ep_in, local_ep_out;
- DIR *busdir , *devdir ;
- struct dirent *de;
- int fd ;
+ std::unique_ptr<DIR, int(*)(DIR*)> bus_dir(opendir(base.c_str()), closedir);
+ if (!bus_dir) return;
- busdir = opendir(base);
- if(busdir == 0) return;
+ dirent* de;
+ while ((de = readdir(bus_dir.get())) != 0) {
+ if (contains_non_digit(de->d_name)) continue;
- while((de = readdir(busdir)) != 0) {
- if(badname(de->d_name)) continue;
+ std::string bus_name = base + "/" + de->d_name;
- snprintf(busname, sizeof busname, "%s/%s", base, de->d_name);
- devdir = opendir(busname);
- if(devdir == 0) continue;
+ std::unique_ptr<DIR, int(*)(DIR*)> dev_dir(opendir(bus_name.c_str()), closedir);
+ if (!dev_dir) continue;
-// DBGX("[ scanning %s ]\n", busname);
- while((de = readdir(devdir))) {
+ while ((de = readdir(dev_dir.get()))) {
unsigned char devdesc[4096];
unsigned char* bufptr = devdesc;
unsigned char* bufend;
@@ -153,22 +141,20 @@
struct usb_endpoint_descriptor *ep1, *ep2;
unsigned zero_mask = 0;
unsigned vid, pid;
- size_t desclength;
- if(badname(de->d_name)) continue;
- snprintf(devname, sizeof devname, "%s/%s", busname, de->d_name);
+ if (contains_non_digit(de->d_name)) continue;
- if(known_device(devname)) {
- DBGX("skipping %s\n", devname);
+ std::string dev_name = bus_name + "/" + de->d_name;
+ if (is_known_device(dev_name.c_str())) {
continue;
}
-// DBGX("[ scanning %s ]\n", devname);
- if((fd = unix_open(devname, O_RDONLY | O_CLOEXEC)) < 0) {
+ int fd = unix_open(dev_name.c_str(), O_RDONLY | O_CLOEXEC);
+ if (fd == -1) {
continue;
}
- desclength = unix_read(fd, devdesc, sizeof(devdesc));
+ size_t desclength = unix_read(fd, devdesc, sizeof(devdesc));
bufend = bufptr + desclength;
// should have device and configuration descriptors, and atleast two endpoints
@@ -188,7 +174,7 @@
vid = device->idVendor;
pid = device->idProduct;
- DBGX("[ %s is V:%04x P:%04x ]\n", devname, vid, pid);
+ DBGX("[ %s is V:%04x P:%04x ]\n", dev_name.c_str(), vid, pid);
// should have config descriptor next
config = (struct usb_config_descriptor *)bufptr;
@@ -225,7 +211,7 @@
struct stat st;
char pathbuf[128];
char link[256];
- char *devpath = NULL;
+ char *devpath = nullptr;
DBGX("looking for bulk endpoints\n");
// looks like ADB...
@@ -267,6 +253,7 @@
}
// we have a match. now we just need to figure out which is in and which is out.
+ unsigned char local_ep_in, local_ep_out;
if (ep1->bEndpointAddress & USB_ENDPOINT_DIR_MASK) {
local_ep_in = ep1->bEndpointAddress;
local_ep_out = ep2->bEndpointAddress;
@@ -293,7 +280,7 @@
}
}
- register_device_callback(devname, devpath,
+ register_device_callback(dev_name.c_str(), devpath,
local_ep_in, local_ep_out,
interface->bInterfaceNumber, device->iSerialNumber, zero_mask);
break;
@@ -304,72 +291,54 @@
} // end of while
unix_close(fd);
- } // end of devdir while
- closedir(devdir);
- } //end of busdir while
- closedir(busdir);
+ }
+ }
}
-static int usb_bulk_write(usb_handle *h, const void *data, int len)
-{
- struct usbdevfs_urb *urb = &h->urb_out;
- int res;
- struct timeval tv;
- struct timespec ts;
+static int usb_bulk_write(usb_handle* h, const void* data, int len) {
+ std::unique_lock<std::mutex> lock(h->mutex);
+ D("++ usb_bulk_write ++\n");
+ usbdevfs_urb* urb = &h->urb_out;
memset(urb, 0, sizeof(*urb));
urb->type = USBDEVFS_URB_TYPE_BULK;
urb->endpoint = h->ep_out;
urb->status = -1;
- urb->buffer = (void*) data;
+ urb->buffer = const_cast<void*>(data);
urb->buffer_length = len;
- D("++ write ++\n");
-
- adb_mutex_lock(&h->lock);
- if(h->dead) {
- res = -1;
- goto fail;
- }
- do {
- res = ioctl(h->desc, USBDEVFS_SUBMITURB, urb);
- } while((res < 0) && (errno == EINTR));
-
- if(res < 0) {
- goto fail;
+ if (h->dead) {
+ errno = EINVAL;
+ return -1;
}
- res = -1;
- h->urb_out_busy = 1;
- for(;;) {
- /* time out after five seconds */
- gettimeofday(&tv, NULL);
- ts.tv_sec = tv.tv_sec + 5;
- ts.tv_nsec = tv.tv_usec * 1000L;
- res = pthread_cond_timedwait(&h->notify, &h->lock, &ts);
- if(res < 0 || h->dead) {
- break;
+ if (TEMP_FAILURE_RETRY(ioctl(h->fd, USBDEVFS_SUBMITURB, urb)) == -1) {
+ return -1;
+ }
+
+ h->urb_out_busy = true;
+ while (true) {
+ auto now = std::chrono::system_clock::now();
+ if (h->cv.wait_until(lock, now + 5s) == std::cv_status::timeout || h->dead) {
+ // TODO: call USBDEVFS_DISCARDURB?
+ errno = ETIMEDOUT;
+ return -1;
}
- if(h->urb_out_busy == 0) {
- if(urb->status == 0) {
- res = urb->actual_length;
+ if (!h->urb_out_busy) {
+ if (urb->status != 0) {
+ errno = -urb->status;
+ return -1;
}
- break;
+ return urb->actual_length;
}
}
-fail:
- adb_mutex_unlock(&h->lock);
- D("-- write --\n");
- return res;
}
-static int usb_bulk_read(usb_handle *h, void *data, int len)
-{
- struct usbdevfs_urb *urb = &h->urb_in;
- struct usbdevfs_urb *out = NULL;
- int res;
-
+static int usb_bulk_read(usb_handle* h, void* data, int len) {
+ std::unique_lock<std::mutex> lock(h->mutex);
D("++ usb_bulk_read ++\n");
+
+ usbdevfs_urb* urb = &h->urb_in;
memset(urb, 0, sizeof(*urb));
urb->type = USBDEVFS_URB_TYPE_BULK;
urb->endpoint = h->ep_in;
@@ -377,63 +346,58 @@
urb->buffer = data;
urb->buffer_length = len;
-
- adb_mutex_lock(&h->lock);
- if(h->dead) {
- res = -1;
- goto fail;
- }
- do {
- res = ioctl(h->desc, USBDEVFS_SUBMITURB, urb);
- } while((res < 0) && (errno == EINTR));
-
- if(res < 0) {
- goto fail;
+ if (h->dead) {
+ errno = EINVAL;
+ return -1;
}
- h->urb_in_busy = 1;
- for(;;) {
+ if (TEMP_FAILURE_RETRY(ioctl(h->fd, USBDEVFS_SUBMITURB, urb)) == -1) {
+ return -1;
+ }
+
+ h->urb_in_busy = true;
+ while (true) {
D("[ reap urb - wait ]\n");
h->reaper_thread = pthread_self();
- adb_mutex_unlock(&h->lock);
- res = ioctl(h->desc, USBDEVFS_REAPURB, &out);
+ int fd = h->fd;
+ lock.unlock();
+
+ // This ioctl must not have TEMP_FAILURE_RETRY because we send SIGALRM to break out.
+ usbdevfs_urb* out = nullptr;
+ int res = ioctl(fd, USBDEVFS_REAPURB, &out);
int saved_errno = errno;
- adb_mutex_lock(&h->lock);
+
+ lock.lock();
h->reaper_thread = 0;
- if(h->dead) {
- res = -1;
- break;
+ if (h->dead) {
+ errno = EINVAL;
+ return -1;
}
- if(res < 0) {
- if(saved_errno == EINTR) {
+ if (res < 0) {
+ if (saved_errno == EINTR) {
continue;
}
D("[ reap urb - error ]\n");
- break;
+ errno = saved_errno;
+ return -1;
}
- D("[ urb @%p status = %d, actual = %d ]\n",
- out, out->status, out->actual_length);
+ D("[ urb @%p status = %d, actual = %d ]\n", out, out->status, out->actual_length);
- if(out == &h->urb_in) {
+ if (out == &h->urb_in) {
D("[ reap urb - IN complete ]\n");
- h->urb_in_busy = 0;
- if(urb->status == 0) {
- res = urb->actual_length;
- } else {
- res = -1;
+ h->urb_in_busy = false;
+ if (urb->status != 0) {
+ errno = -urb->status;
+ return -1;
}
- break;
+ return urb->actual_length;
}
- if(out == &h->urb_out) {
+ if (out == &h->urb_out) {
D("[ reap urb - OUT compelete ]\n");
- h->urb_out_busy = 0;
- adb_cond_broadcast(&h->notify);
+ h->urb_out_busy = false;
+ h->cv.notify_all();
}
}
-fail:
- adb_mutex_unlock(&h->lock);
- D("-- usb_bulk_read --\n");
- return res;
}
@@ -443,19 +407,15 @@
unsigned char *data = (unsigned char*) _data;
int n = usb_bulk_write(h, data, len);
- if(n != len) {
- D("ERROR: n = %d, errno = %d (%s)\n",
- n, errno, strerror(errno));
+ if (n != len) {
+ D("ERROR: n = %d, errno = %d (%s)\n", n, errno, strerror(errno));
return -1;
}
- if(h->zero_mask && !(len & h->zero_mask)) {
- /* if we need 0-markers and our transfer
- ** is an even multiple of the packet size,
- ** then send the zero markers.
- */
- n = usb_bulk_write(h, _data, 0);
- return n;
+ if (h->zero_mask && !(len & h->zero_mask)) {
+ // If we need 0-markers and our transfer is an even multiple of the packet size,
+ // then send a zero marker.
+ return usb_bulk_write(h, _data, 0);
}
D("-- usb_write --\n");
@@ -471,11 +431,11 @@
while(len > 0) {
int xfer = len;
- D("[ usb read %d fd = %d], fname=%s\n", xfer, h->desc, h->fname);
+ D("[ usb read %d fd = %d], path=%s\n", xfer, h->fd, h->path.c_str());
n = usb_bulk_read(h, data, xfer);
- D("[ usb read %d ] = %d, fname=%s\n", xfer, n, h->fname);
+ D("[ usb read %d ] = %d, path=%s\n", xfer, n, h->path.c_str());
if(n != xfer) {
- if((errno == ETIMEDOUT) && (h->desc != -1)) {
+ if((errno == ETIMEDOUT) && (h->fd != -1)) {
D("[ timeout ]\n");
if(n > 0){
data += n;
@@ -496,12 +456,11 @@
return 0;
}
-void usb_kick(usb_handle *h)
-{
- D("[ kicking %p (fd = %d) ]\n", h, h->desc);
- adb_mutex_lock(&h->lock);
- if(h->dead == 0) {
- h->dead = 1;
+void usb_kick(usb_handle* h) {
+ std::lock_guard<std::mutex> lock(h->mutex);
+ D("[ kicking %p (fd = %d) ]\n", h, h->fd);
+ if (!h->dead) {
+ h->dead = true;
if (h->writeable) {
/* HACK ALERT!
@@ -517,34 +476,27 @@
** but this ensures that a reader blocked on REAPURB
** will get unblocked
*/
- ioctl(h->desc, USBDEVFS_DISCARDURB, &h->urb_in);
- ioctl(h->desc, USBDEVFS_DISCARDURB, &h->urb_out);
+ ioctl(h->fd, USBDEVFS_DISCARDURB, &h->urb_in);
+ ioctl(h->fd, USBDEVFS_DISCARDURB, &h->urb_out);
h->urb_in.status = -ENODEV;
h->urb_out.status = -ENODEV;
- h->urb_in_busy = 0;
- h->urb_out_busy = 0;
- adb_cond_broadcast(&h->notify);
+ h->urb_in_busy = false;
+ h->urb_out_busy = false;
+ h->cv.notify_all();
} else {
unregister_usb_transport(h);
}
}
- adb_mutex_unlock(&h->lock);
}
-int usb_close(usb_handle *h)
-{
- D("++ usb close ++\n");
- adb_mutex_lock(&usb_lock);
- h->next->prev = h->prev;
- h->prev->next = h->next;
- h->prev = 0;
- h->next = 0;
+int usb_close(usb_handle* h) {
+ std::lock_guard<std::mutex> lock(g_usb_handles_mutex);
+ g_usb_handles.remove(h);
- unix_close(h->desc);
- D("-- usb closed %p (fd = %d) --\n", h, h->desc);
- adb_mutex_unlock(&usb_lock);
+ D("-- usb close %p (fd = %d) --\n", h, h->fd);
- free(h);
+ delete h;
+
return 0;
}
@@ -557,54 +509,44 @@
// from the list when we're finally closed and everything will work out
// fine.
//
- // If we have a usb_handle on the list 'o handles with a matching name, we
+ // If we have a usb_handle on the list of handles with a matching name, we
// have no further work to do.
- adb_mutex_lock(&usb_lock);
- for (usb_handle* usb = handle_list.next; usb != &handle_list; usb = usb->next) {
- if (!strcmp(usb->fname, dev_name)) {
- adb_mutex_unlock(&usb_lock);
- return;
+ {
+ std::lock_guard<std::mutex> lock(g_usb_handles_mutex);
+ for (usb_handle* usb: g_usb_handles) {
+ if (usb->path == dev_name) {
+ return;
+ }
}
}
- adb_mutex_unlock(&usb_lock);
D("[ usb located new device %s (%d/%d/%d) ]\n", dev_name, ep_in, ep_out, interface);
- usb_handle* usb = reinterpret_cast<usb_handle*>(calloc(1, sizeof(usb_handle)));
- if (usb == nullptr) fatal("couldn't allocate usb_handle");
- strcpy(usb->fname, dev_name);
+ std::unique_ptr<usb_handle> usb(new usb_handle);
+ usb->path = dev_name;
usb->ep_in = ep_in;
usb->ep_out = ep_out;
usb->zero_mask = zero_mask;
- usb->writeable = 1;
- adb_cond_init(&usb->notify, 0);
- adb_mutex_init(&usb->lock, 0);
- // Initialize mark to 1 so we don't get garbage collected after the device
- // scan.
- usb->mark = 1;
- usb->reaper_thread = 0;
+ // Initialize mark so we don't get garbage collected after the device scan.
+ usb->mark = true;
- usb->desc = unix_open(usb->fname, O_RDWR | O_CLOEXEC);
- if (usb->desc == -1) {
+ usb->fd = unix_open(usb->path.c_str(), O_RDWR | O_CLOEXEC);
+ if (usb->fd == -1) {
// Opening RW failed, so see if we have RO access.
- usb->desc = unix_open(usb->fname, O_RDONLY | O_CLOEXEC);
- if (usb->desc == -1) {
- D("[ usb open %s failed: %s]\n", usb->fname, strerror(errno));
- free(usb);
+ usb->fd = unix_open(usb->path.c_str(), O_RDONLY | O_CLOEXEC);
+ if (usb->fd == -1) {
+ D("[ usb open %s failed: %s]\n", usb->path.c_str(), strerror(errno));
return;
}
usb->writeable = 0;
}
- D("[ usb opened %s%s, fd=%d]\n", usb->fname,
- (usb->writeable ? "" : " (read-only)"), usb->desc);
+ D("[ usb opened %s%s, fd=%d]\n",
+ usb->path.c_str(), (usb->writeable ? "" : " (read-only)"), usb->fd);
if (usb->writeable) {
- if (ioctl(usb->desc, USBDEVFS_CLAIMINTERFACE, &interface) != 0) {
- D("[ usb ioctl(%d, USBDEVFS_CLAIMINTERFACE) failed: %s]\n",
- usb->desc, strerror(errno));
- unix_close(usb->desc);
- free(usb);
+ if (ioctl(usb->fd, USBDEVFS_CLAIMINTERFACE, &interface) != 0) {
+ D("[ usb ioctl(%d, USBDEVFS_CLAIMINTERFACE) failed: %s]\n", usb->fd, strerror(errno));
return;
}
}
@@ -623,14 +565,12 @@
serial = android::base::Trim(serial);
// Add to the end of the active handles.
- adb_mutex_lock(&usb_lock);
- usb->next = &handle_list;
- usb->prev = handle_list.prev;
- usb->prev->next = usb;
- usb->next->prev = usb;
- adb_mutex_unlock(&usb_lock);
-
- register_usb_transport(usb, serial.c_str(), dev_path, usb->writeable);
+ usb_handle* done_usb = usb.release();
+ {
+ std::lock_guard<std::mutex> lock(g_usb_handles_mutex);
+ g_usb_handles.push_back(done_usb);
+ }
+ register_usb_transport(done_usb, serial.c_str(), dev_path, done_usb->writeable);
}
static void* device_poll_thread(void* unused) {
@@ -644,19 +584,13 @@
return nullptr;
}
-static void sigalrm_handler(int signo) {
- // don't need to do anything here
-}
-
-void usb_init()
-{
- struct sigaction actions;
-
+void usb_init() {
+ struct sigaction actions;
memset(&actions, 0, sizeof(actions));
sigemptyset(&actions.sa_mask);
actions.sa_flags = 0;
- actions.sa_handler = sigalrm_handler;
- sigaction(SIGALRM,& actions, NULL);
+ actions.sa_handler = [](int) {};
+ sigaction(SIGALRM, &actions, nullptr);
if (!adb_thread_create(device_poll_thread, nullptr)) {
fatal_errno("cannot create input thread");