Pause event handling while opening and closing devices

Ludovic Rousseau found that crashes often occur if you close a device
while another thread is doing event handling.

Fix this by adding an internal control pipe, which the close routines
use to interrupt the event handler and obtain the event handling lock,
ensuring that no other thread is handling events while the device is
closed. After the close completes, it signals all the event handlers
to start up again using the usual mechanism.

Also modified libusb_open() to do a similar thing, so that event
handlers are interrupted in order to realise that a new poll fd has
appeared.

3 files changed, 205 insertions, 37 deletions

diff --git a/libusb/core.c b/libusb/core.c
index baf9afb..97aebd2 100644
--- a/libusb/core.c
+++ b/libusb/core.c
@@ -27,6 +27,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <sys/types.h>
+#include <unistd.h>
 
 #include "libusb.h"
 #include "libusbi.h"
@@ -364,7 +365,7 @@ libusb_free_device_list(list, 1);
  * New devices presented by the libusb_get_device_list() function all have a
  * reference count of 1. You can increase and decrease reference count using
  * libusb_ref_device() and libusb_unref_device(). A device is destroyed when
- * it's reference count reaches 0.
+ * its reference count reaches 0.
  *
  * With the above information in mind, the process of opening a device can
  * be viewed as follows:
@@ -739,8 +740,10 @@ API_EXPORTED void libusb_unref_device(libusb_device *dev)
  */
 API_EXPORTED int libusb_open(libusb_device *dev, libusb_device_handle **handle)
 {
+	struct libusb_context *ctx = DEVICE_CTX(dev);
 	struct libusb_device_handle *_handle;
 	size_t priv_size = usbi_backend->device_handle_priv_size;
+	unsigned char dummy = 1;
 	int r;
 	usbi_dbg("open %d.%d", dev->bus_number, dev->device_address);
 
@@ -763,10 +766,50 @@ API_EXPORTED int libusb_open(libusb_device *dev, libusb_device_handle **handle)
 		return r;
 	}
 
-	pthread_mutex_lock(&dev->ctx->open_devs_lock);
-	list_add(&_handle->list, &dev->ctx->open_devs);
-	pthread_mutex_unlock(&dev->ctx->open_devs_lock);
+	pthread_mutex_lock(&ctx->open_devs_lock);
+	list_add(&_handle->list, &ctx->open_devs);
+	pthread_mutex_unlock(&ctx->open_devs_lock);
 	*handle = _handle;
+
+
+	/* At this point, we want to interrupt any existing event handlers so
+	 * that they realise the addition of the new device's poll fd. One
+	 * example when this is desirable is if the user is running a separate
+	 * dedicated libusb events handling thread, which is running with a long
+	 * or infinite timeout. We want to interrupt that iteration of the loop,
+	 * so that it picks up the new fd, and then continues. */
+
+	/* record that we are messing with poll fds */
+	pthread_mutex_lock(&ctx->pollfd_modify_lock);
+	ctx->pollfd_modify++;
+	pthread_mutex_unlock(&ctx->pollfd_modify_lock);
+
+	/* write some data on control pipe to interrupt event handlers */
+	r = write(ctx->ctrl_pipe[1], &dummy, sizeof(dummy));
+	if (r <= 0) {
+		usbi_warn(ctx, "internal signalling write failed");
+		pthread_mutex_lock(&ctx->pollfd_modify_lock);
+		ctx->pollfd_modify--;
+		pthread_mutex_unlock(&ctx->pollfd_modify_lock);
+		return 0;
+	}
+
+	/* take event handling lock */
+	libusb_lock_events(ctx);
+
+	/* read the dummy data */
+	r = read(ctx->ctrl_pipe[0], &dummy, sizeof(dummy));
+	if (r <= 0)
+		usbi_warn(ctx, "internal signalling read failed");
+
+	/* we're done with modifying poll fds */
+	pthread_mutex_lock(&ctx->pollfd_modify_lock);
+	ctx->pollfd_modify--;
+	pthread_mutex_unlock(&ctx->pollfd_modify_lock);
+
+	/* Release event handling lock and wake up event waiters */
+	libusb_unlock_events(ctx);
+
 	return 0;
 }
 
@@ -821,10 +864,16 @@ out:
 	return handle;
 }
 
-static void do_close(struct libusb_device_handle *dev_handle)
+static void do_close(struct libusb_context *ctx,
+	struct libusb_device_handle *dev_handle)
 {
+	pthread_mutex_lock(&ctx->open_devs_lock);
+	list_del(&dev_handle->list);
+	pthread_mutex_unlock(&ctx->open_devs_lock);
+
 	usbi_backend->close(dev_handle);
 	libusb_unref_device(dev_handle->dev);
+	free(dev_handle);
 }
 
 /** \ingroup dev
@@ -840,16 +889,56 @@ static void do_close(struct libusb_device_handle *dev_handle)
  */
 API_EXPORTED void libusb_close(libusb_device_handle *dev_handle)
 {
+	struct libusb_context *ctx;
+	unsigned char dummy = 1;
+	ssize_t r;
+
 	if (!dev_handle)
 		return;
 	usbi_dbg("");
 
-	pthread_mutex_lock(&HANDLE_CTX(dev_handle)->open_devs_lock);
-	list_del(&dev_handle->list);
-	pthread_mutex_unlock(&HANDLE_CTX(dev_handle)->open_devs_lock);
+	ctx = HANDLE_CTX(dev_handle);
+
+	/* Similarly to libusb_open(), we want to interrupt all event handlers
+	 * at this point. More importantly, we want to perform the actual close of
+	 * the device while holding the event handling lock (preventing any other
+	 * thread from doing event handling) because we will be removing a file
+	 * descriptor from the polling loop. */
+
+	/* record that we are messing with poll fds */
+	pthread_mutex_lock(&ctx->pollfd_modify_lock);
+	ctx->pollfd_modify++;
+	pthread_mutex_unlock(&ctx->pollfd_modify_lock);
+
+	/* write some data on control pipe to interrupt event handlers */
+	r = write(ctx->ctrl_pipe[1], &dummy, sizeof(dummy));
+	if (r <= 0) {
+		usbi_warn(ctx, "internal signalling write failed, closing anyway");
+		do_close(ctx, dev_handle);
+		pthread_mutex_lock(&ctx->pollfd_modify_lock);
+		ctx->pollfd_modify--;
+		pthread_mutex_unlock(&ctx->pollfd_modify_lock);
+		return;
+	}
 
-	do_close(dev_handle);
-	free(dev_handle);
+	/* take event handling lock */
+	libusb_lock_events(ctx);
+
+	/* read the dummy data */
+	r = read(ctx->ctrl_pipe[0], &dummy, sizeof(dummy));
+	if (r <= 0)
+		usbi_warn(ctx, "internal signalling read failed, closing anyway");
+
+	/* Close the device */
+	do_close(ctx, dev_handle);
+
+	/* we're done with modifying poll fds */
+	pthread_mutex_lock(&ctx->pollfd_modify_lock);
+	ctx->pollfd_modify--;
+	pthread_mutex_unlock(&ctx->pollfd_modify_lock);
+
+	/* Release event handling lock and wake up event waiters */
+	libusb_unlock_events(ctx);
 }
 
 /** \ingroup dev
@@ -1216,7 +1305,7 @@ API_EXPORTED int libusb_attach_kernel_driver(libusb_device_handle *dev,
  * choose to increase the message verbosity level, ensure that your
  * application does not close the stdout/stderr file descriptors.
  *
- * You are advised to set level 3. libusb is conservative with it's message
+ * You are advised to set level 3. libusb is conservative with its message
  * logging and most of the time, will only log messages that explain error
  * conditions and other oddities. This will help you debug your software.
  *
@@ -1251,6 +1340,7 @@ API_EXPORTED int libusb_init(libusb_context **context)
 {
 	char *dbg = getenv("LIBUSB_DEBUG");
 	struct libusb_context *ctx = malloc(sizeof(*ctx));
+	int r;
 
 	if (!ctx)
 		return LIBUSB_ERROR_NO_MEM;
@@ -1263,19 +1353,24 @@ API_EXPORTED int libusb_init(libusb_context **context)
 	}
 
 	usbi_dbg("");
+
 	if (usbi_backend->init) {
-		int r = usbi_backend->init(ctx);
-		if (r) {
-			free(ctx);
-			return r;
-		}
+		r = usbi_backend->init(ctx);
+		if (r)
+			goto err;
 	}
 
 	pthread_mutex_init(&ctx->usb_devs_lock, NULL);
 	pthread_mutex_init(&ctx->open_devs_lock, NULL);
 	list_init(&ctx->usb_devs);
 	list_init(&ctx->open_devs);
-	usbi_io_init(ctx);
+
+	r = usbi_io_init(ctx);
+	if (r < 0) {
+		if (usbi_backend->exit)
+			usbi_backend->exit();
+		goto err;
+	}
 
 	pthread_mutex_lock(&default_context_lock);
 	if (!usbi_default_context) {
@@ -1287,6 +1382,10 @@ API_EXPORTED int libusb_init(libusb_context **context)
 	if (context)
 		*context = ctx;
 	return 0;
+
+err:
+	free(ctx);
+	return r;
 }
 
 /** \ingroup lib
@@ -1299,23 +1398,15 @@ API_EXPORTED void libusb_exit(struct libusb_context *ctx)
 	USBI_GET_CONTEXT(ctx);
 	usbi_dbg("");
 
-	pthread_mutex_lock(&ctx->open_devs_lock);
-	if (!list_empty(&ctx->open_devs)) {
-		struct libusb_device_handle *devh;
-		struct libusb_device_handle *tmp;
-
-		usbi_dbg("naughty app left some devices open!");
-		list_for_each_entry_safe(devh, tmp, &ctx->open_devs, list) {
-			list_del(&devh->list);
-			do_close(devh);
-			free(devh);
-		}
-	}
-	pthread_mutex_unlock(&ctx->open_devs_lock);
+	/* a little sanity check. doesn't bother with open_devs locking because
+	 * unless there is an application bug, nobody will be accessing this. */
+	if (!list_empty(&ctx->open_devs))
+		usbi_warn(ctx, "application left some devices open");
 
+	usbi_io_exit(ctx);
 	if (usbi_backend->exit)
 		usbi_backend->exit();
-	
+
 	pthread_mutex_lock(&default_context_lock);
 	if (ctx == usbi_default_context) {
 		usbi_dbg("freeing default context");
diff --git a/libusb/io.c b/libusb/io.c
index 50033ca..d06025e 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -760,7 +760,6 @@ void myfunc() {
 retry:
 if (libusb_try_lock_events(ctx) == 0) {
 	// we obtained the event lock: do our own event handling
-	libusb_lock_events(ctx);
 	while (!completed) {
 		poll(libusb file descriptors, 120*1000);
 		if (poll indicates activity)
@@ -841,15 +840,36 @@ printf("completed!\n");
  * fall back to the "event waiters" mechanism detailed above.
  */
 
-void usbi_io_init(struct libusb_context *ctx)
+int usbi_io_init(struct libusb_context *ctx)
 {
+	int r;
+
 	pthread_mutex_init(&ctx->flying_transfers_lock, NULL);
 	pthread_mutex_init(&ctx->pollfds_lock, NULL);
+	pthread_mutex_init(&ctx->pollfd_modify_lock, NULL);
 	pthread_mutex_init(&ctx->events_lock, NULL);
 	pthread_mutex_init(&ctx->event_waiters_lock, NULL);
 	pthread_cond_init(&ctx->event_waiters_cond, NULL);
 	list_init(&ctx->flying_transfers);
 	list_init(&ctx->pollfds);
+
+	/* FIXME should use an eventfd on kernels that support it */
+	r = pipe(ctx->ctrl_pipe);
+	if (r < 0)
+		return LIBUSB_ERROR_OTHER;
+
+	r = usbi_add_pollfd(ctx, ctx->ctrl_pipe[0], POLLIN);
+	if (r < 0)
+		return r;
+
+	return 0;
+}
+
+void usbi_io_exit(struct libusb_context *ctx)
+{
+	usbi_remove_pollfd(ctx, ctx->ctrl_pipe[0]);
+	close(ctx->ctrl_pipe[0]);
+	close(ctx->ctrl_pipe[1]);
 }
 
 static int calculate_timeout(struct usbi_transfer *transfer)
@@ -1132,7 +1152,17 @@ API_EXPORTED int libusb_try_lock_events(libusb_context *ctx)
 {
 	int r;
 	USBI_GET_CONTEXT(ctx);
-	
+
+	/* is someone else waiting to modify poll fds? if so, don't let this thread
+	 * start event handling */
+	pthread_mutex_lock(&ctx->pollfd_modify_lock);
+	r = ctx->pollfd_modify;
+	pthread_mutex_unlock(&ctx->pollfd_modify_lock);
+	if (r) {
+		usbi_dbg("someone else is modifying poll fds");
+		return 1;
+	}
+
 	r = pthread_mutex_trylock(&ctx->events_lock);
 	if (r)
 		return 1;
@@ -1196,7 +1226,19 @@ API_EXPORTED void libusb_unlock_events(libusb_context *ctx)
  */
 API_EXPORTED int libusb_event_handler_active(libusb_context *ctx)
 {
+	int r;
 	USBI_GET_CONTEXT(ctx);
+
+	/* is someone else waiting to modify poll fds? if so, don't let this thread
+	 * start event handling -- indicate that event handling is happening */
+	pthread_mutex_lock(&ctx->pollfd_modify_lock);
+	r = ctx->pollfd_modify;
+	pthread_mutex_unlock(&ctx->pollfd_modify_lock);
+	if (r) {
+		usbi_dbg("someone else is modifying poll fds");
+		return 1;
+	}
+
 	return ctx->event_handler_active;
 }
 
@@ -1401,10 +1443,27 @@ static int handle_events(struct libusb_context *ctx, struct timeval *tv)
 		return LIBUSB_ERROR_IO;
 	}
 
+	/* fd[0] is always the ctrl pipe */
+	if (fds[0].revents) {
+		/* another thread wanted to interrupt event handling, and it succeeded!
+		 * handle any other events that cropped up at the same time, and
+		 * simply return */
+		usbi_dbg("caught a fish on the control pipe");
+
+		if (r == 1) {
+			r = 0;
+			goto handled;
+		} else {
+			/* prevent OS backend from trying to handle events on ctrl pipe */
+			fds[0].revents = 0;
+		}
+	}
+
 	r = usbi_backend->handle_events(ctx, fds, nfds, r);
 	if (r)
 		usbi_err(ctx, "backend handle_events failed with error %d", r);
 
+handled:
 	free(fds);
 	return r;
 }
@@ -1641,6 +1700,14 @@ API_EXPORTED int libusb_get_next_timeout(libusb_context *ctx,
  *
  * To remove notifiers, pass NULL values for the function pointers.
  *
+ * Note that file descriptors may have been added even before you register
+ * these notifiers (e.g. at libusb_init() time).
+ *
+ * Additionally, note that the removal notifier may be called during
+ * libusb_exit() (e.g. when it is closing file descriptors that were opened
+ * and added to the poll set at libusb_init() time). If you don't want this,
+ * remove the notifiers immediately before calling libusb_exit().
+ *
  * \param ctx the context to operate on, or NULL for the default context
  * \param added_cb pointer to function for addition notifications
  * \param removed_cb pointer to function for removal notifications
@@ -1670,7 +1737,7 @@ int usbi_add_pollfd(struct libusb_context *ctx, int fd, short events)
 	ipollfd->pollfd.fd = fd;
 	ipollfd->pollfd.events = events;
 	pthread_mutex_lock(&ctx->pollfds_lock);
-	list_add(&ipollfd->list, &ctx->pollfds);
+	list_add_tail(&ipollfd->list, &ctx->pollfds);
 	pthread_mutex_unlock(&ctx->pollfds_lock);
 
 	if (ctx->fd_added_cb)
diff --git a/libusb/libusbi.h b/libusb/libusbi.h
index 43dd109..b51ba0b 100644
--- a/libusb/libusbi.h
+++ b/libusb/libusbi.h
@@ -144,6 +144,10 @@ struct libusb_context {
 	int debug;
 	int debug_fixed;
 
+	/* internal control pipe, used for interrupting event handling when
+	 * something needs to modify poll fds. */
+	int ctrl_pipe[2];
+
 	struct list_head usb_devs;
 	pthread_mutex_t usb_devs_lock;
 
@@ -159,10 +163,15 @@ struct libusb_context {
 	struct list_head flying_transfers;
 	pthread_mutex_t flying_transfers_lock;
 
-	/* list of poll fd's */
+	/* list of poll fds */
 	struct list_head pollfds;
 	pthread_mutex_t pollfds_lock;
 
+	/* a counter that is set when we want to interrupt event handling, in order
+	 * to modify the poll fd set. and a lock to protect it. */
+	unsigned int pollfd_modify;
+	pthread_mutex_t pollfd_modify_lock;
+
 	/* user callbacks for pollfd changes */
 	libusb_pollfd_added_cb fd_added_cb;
 	libusb_pollfd_removed_cb fd_removed_cb;
@@ -255,7 +264,8 @@ struct usb_descriptor_header {
 
 /* shared data and functions */
 
-void usbi_io_init(struct libusb_context *ctx);
+int usbi_io_init(struct libusb_context *ctx);
+void usbi_io_exit(struct libusb_context *ctx);
 
 struct libusb_device *usbi_alloc_device(struct libusb_context *ctx,
 	unsigned long session_id);