core: Remove taking of events lock inside usbi_fd_notification()

It is unnecessary to take the events lock when a thread needs to
interrupt an event handler to receive a change list of poll fds.
It is sufficient to simply write to the control pipe and let the
event handler be notified of this event when it polls.

Taking the events lock inside this function leads to opportunity
for deadlock in certain situations, such as a client program
running on an OS that uses fd notification on each individual
transfer. If the client program were to protect a list of transfers
with a single lock, it could deadlock if that lock were taken in
two separate threads, one which is attempting to submit a new
transfer and one which is executing inside the transfer completion
callback.

Thanks to Dmitry Fleytman and Pavel Gurvich for reporting this.

Signed-off-by: Chris Dickens <christopher.a.dickens@gmail.com>

3 files changed, 21 insertions, 31 deletions

diff --git a/libusb/core.c b/libusb/core.c
index c992747..997368a 100644
--- a/libusb/core.c
+++ b/libusb/core.c
@@ -1180,39 +1180,10 @@ void usbi_fd_notification(struct libusb_context *ctx)
 	unsigned char dummy = 1;
 	ssize_t r;
 
-	if (ctx == NULL)
-		return;
-
-	/* record that we are messing with poll fds */
-	usbi_mutex_lock(&ctx->pollfd_modify_lock);
-	ctx->pollfd_modify++;
-	usbi_mutex_unlock(&ctx->pollfd_modify_lock);
-
 	/* write some data on control pipe to interrupt event handlers */
 	r = usbi_write(ctx->ctrl_pipe[1], &dummy, sizeof(dummy));
-	if (r <= 0) {
+	if (r != sizeof(dummy))
 		usbi_warn(ctx, "internal signalling write failed");
-		usbi_mutex_lock(&ctx->pollfd_modify_lock);
-		ctx->pollfd_modify--;
-		usbi_mutex_unlock(&ctx->pollfd_modify_lock);
-		return;
-	}
-
-	/* take event handling lock */
-	libusb_lock_events(ctx);
-
-	/* read the dummy data */
-	r = usbi_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 */
-	usbi_mutex_lock(&ctx->pollfd_modify_lock);
-	ctx->pollfd_modify--;
-	usbi_mutex_unlock(&ctx->pollfd_modify_lock);
-
-	/* Release event handling lock and wake up event waiters */
-	libusb_unlock_events(ctx);
 }
 
 /** \ingroup dev
diff --git a/libusb/io.c b/libusb/io.c
index 9cf38f8..07010c4 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -2051,8 +2051,27 @@ redo_poll:
 		/* another thread wanted to interrupt event handling, and it succeeded!
 		 * handle any other events that cropped up at the same time, and
 		 * simply return */
+		ssize_t ret;
+		unsigned char dummy;
+		unsigned int ru;
+
 		usbi_dbg("caught a fish on the control pipe");
 
+		/* read the dummy data from the control pipe unless someone is closing
+		 * a device */
+		usbi_mutex_lock(&ctx->pollfd_modify_lock);
+		ru = ctx->pollfd_modify;
+		usbi_mutex_unlock(&ctx->pollfd_modify_lock);
+		if (!ru) {
+			ret = usbi_read(ctx->ctrl_pipe[0], &dummy, sizeof(dummy));
+			if (ret != sizeof(dummy)) {
+				usbi_err(ctx, "control pipe read error %d err=%d",
+					 ret, errno);
+				r = LIBUSB_ERROR_OTHER;
+				goto handled;
+			}
+		}
+
 		if (0 == --r)
 			goto handled;
 	}
diff --git a/libusb/version_nano.h b/libusb/version_nano.h
index ec8df57..b6431e8 100644
--- a/libusb/version_nano.h
+++ b/libusb/version_nano.h
@@ -1 +1 @@
-#define LIBUSB_NANO 10930
+#define LIBUSB_NANO 10931