From 7ede4b76bdfec18850d1c3bbe87af6a0f69f76dc Mon Sep 17 00:00:00 2001 From: Chris Dickens Date: Mon, 17 Nov 2014 23:53:06 -0800 Subject: 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 --- libusb/core.c | 31 +------------------------------ libusb/io.c | 19 +++++++++++++++++++ libusb/version_nano.h | 2 +- 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 -- cgit v1.2.3