Core: Fix unconditional disarming of timerfd

* Existing code appears disarms the timerfd always, which cancels
  pending timeouts as soon as one packet completes.
* This fix moves the disarming of the timerfd to
  arm_timerfd_for_next_timeout(), where it is now done conditionally.
  It also avoids calling disarm outside of the above call.
* This patch also ensures that all handling of the timerfd is done
  under the flying transfers lock.
* Issue reported by Hans de Goede. For more info, see:
  https://sourceforge.net/mailarchive/message.php?msg_id=29442693

2 files changed, 83 insertions, 92 deletions

diff --git a/libusb/io.c b/libusb/io.c
index b8d445c..e6d4132 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -1166,14 +1166,13 @@ static int add_to_flying_list(struct usbi_transfer *transfer)
 	/* if we have no other flying transfers, start the list with this one */
 	if (list_empty(&ctx->flying_transfers)) {
 		list_add(&transfer->list, &ctx->flying_transfers);
-		if (timerisset(timeout))
-			r = 1;
 		goto out;
 	}
 
 	/* if we have infinite timeout, append to end of list */
 	if (!timerisset(timeout)) {
 		list_add_tail(&transfer->list, &ctx->flying_transfers);
+		/* first is irrelevant in this case */
 		goto out;
 	}
 
@@ -1186,15 +1185,33 @@ static int add_to_flying_list(struct usbi_transfer *transfer)
 				(cur_tv->tv_sec == timeout->tv_sec &&
 					cur_tv->tv_usec > timeout->tv_usec)) {
 			list_add_tail(&transfer->list, &cur->list);
-			r = first;
 			goto out;
 		}
 		first = 0;
 	}
+	/* first is 0 at this stage (list not empty) */
 
 	/* otherwise we need to be inserted at the end */
 	list_add_tail(&transfer->list, &ctx->flying_transfers);
 out:
+#ifdef USBI_TIMERFD_AVAILABLE
+	if (first && usbi_using_timerfd(ctx) && timerisset(timeout)) {
+		/* if this transfer has the lowest timeout of all active transfers,
+		 * rearm the timerfd with this transfer's timeout */
+		const struct itimerspec it = { {0, 0},
+			{ transfer->timeout.tv_sec, transfer->timeout.tv_usec * 1000 } };
+		usbi_dbg("arm timerfd for timeout in %dms (first in line)",
+			USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer)->timeout);
+		r = timerfd_settime(ctx->timerfd, TFD_TIMER_ABSTIME, &it, NULL);
+		if (r < 0) {
+			usbi_warn(ctx, "failed to arm first timerfd (error %d)", r);
+			r = LIBUSB_ERROR_OTHER;
+		}
+	}
+#else
+	UNUSED(first);
+#endif
+
 	usbi_mutex_unlock(&ctx->flying_transfers_lock);
 	return r;
 }
@@ -1272,6 +1289,62 @@ void API_EXPORTED libusb_free_transfer(struct libusb_transfer *transfer)
 	free(itransfer);
 }
 
+#ifdef USBI_TIMERFD_AVAILABLE
+static int disarm_timerfd(struct libusb_context *ctx)
+{
+	const struct itimerspec disarm_timer = { { 0, 0 }, { 0, 0 } };
+	int r;
+
+	usbi_dbg("");
+	r = timerfd_settime(ctx->timerfd, 0, &disarm_timer, NULL);
+	if (r < 0)
+		return LIBUSB_ERROR_OTHER;
+	else
+		return 0;
+}
+
+/* iterates through the flying transfers, and rearms the timerfd based on the
+ * next upcoming timeout.
+ * must be called with flying_list locked.
+ * returns 0 if there was no timeout to arm, 1 if the next timeout was armed,
+ * or a LIBUSB_ERROR code on failure.
+ */
+static int arm_timerfd_for_next_timeout(struct libusb_context *ctx)
+{
+	struct usbi_transfer *transfer;
+
+	list_for_each_entry(transfer, &ctx->flying_transfers, list, struct usbi_transfer) {
+		struct timeval *cur_tv = &transfer->timeout;
+
+		/* if we've reached transfers of infinite timeout, then we have no
+		 * arming to do */
+		if (!timerisset(cur_tv))
+			goto disarm;
+
+		/* act on first transfer that is not already cancelled */
+		if (!(transfer->flags & USBI_TRANSFER_TIMED_OUT)) {
+			int r;
+			const struct itimerspec it = { {0, 0},
+				{ cur_tv->tv_sec, cur_tv->tv_usec * 1000 } };
+			usbi_dbg("next timeout originally %dms", USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer)->timeout);
+			r = timerfd_settime(ctx->timerfd, TFD_TIMER_ABSTIME, &it, NULL);
+			if (r < 0)
+				return LIBUSB_ERROR_OTHER;
+			return 1;
+		}
+	}
+
+disarm:
+	return disarm_timerfd(ctx);
+}
+#else
+static int arm_timerfd_for_next_timeout(struct libusb_context *ctx)
+{
+	(void)ctx;
+	return 0;
+}
+#endif
+
 /** \ingroup asyncio
  * Submit a transfer. This function will fire off the USB transfer and then
  * return immediately.
@@ -1290,7 +1363,6 @@ int API_EXPORTED libusb_submit_transfer(struct libusb_transfer *transfer)
 	struct usbi_transfer *itransfer =
 		LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer);
 	int r;
-	int first;
 	int updated_fds;
 
 	usbi_mutex_lock(&itransfer->lock);
@@ -1302,27 +1374,16 @@ int API_EXPORTED libusb_submit_transfer(struct libusb_transfer *transfer)
 		goto out;
 	}
 
-	first = add_to_flying_list(itransfer);
+	r = add_to_flying_list(itransfer);
+	if (r)
+		goto out;
 	r = usbi_backend->submit_transfer(itransfer);
 	if (r) {
 		usbi_mutex_lock(&ctx->flying_transfers_lock);
 		list_del(&itransfer->list);
+		arm_timerfd_for_next_timeout(ctx);
 		usbi_mutex_unlock(&ctx->flying_transfers_lock);
 	}
-#ifdef USBI_TIMERFD_AVAILABLE
-	else if (first && usbi_using_timerfd(ctx)) {
-		/* if this transfer has the lowest timeout of all active transfers,
-		 * rearm the timerfd with this transfer's timeout */
-		const struct itimerspec it = { {0, 0},
-			{ itransfer->timeout.tv_sec, itransfer->timeout.tv_usec * 1000 } };
-		usbi_dbg("arm timerfd for timeout in %dms (first in line)", transfer->timeout);
-		r = timerfd_settime(ctx->timerfd, TFD_TIMER_ABSTIME, &it, NULL);
-		if (r < 0)
-			r = LIBUSB_ERROR_OTHER;
-	}
-#else
-	(void)first;
-#endif
 
 out:
 	updated_fds = (itransfer->flags & USBI_TRANSFER_UPDATED_FDS);
@@ -1372,66 +1433,6 @@ int API_EXPORTED libusb_cancel_transfer(struct libusb_transfer *transfer)
 	return r;
 }
 
-#ifdef USBI_TIMERFD_AVAILABLE
-static int disarm_timerfd(struct libusb_context *ctx)
-{
-	const struct itimerspec disarm_timer = { { 0, 0 }, { 0, 0 } };
-	int r;
-
-	usbi_dbg("");
-	r = timerfd_settime(ctx->timerfd, 0, &disarm_timer, NULL);
-	if (r < 0)
-		return LIBUSB_ERROR_OTHER;
-	else
-		return 0;
-}
-
-/* iterates through the flying transfers, and rearms the timerfd based on the
- * next upcoming timeout.
- * must be called with flying_list locked.
- * returns 0 if there was no timeout to arm, 1 if the next timeout was armed,
- * or a LIBUSB_ERROR code on failure.
- */
-static int arm_timerfd_for_next_timeout(struct libusb_context *ctx)
-{
-	struct usbi_transfer *transfer;
-
-	list_for_each_entry(transfer, &ctx->flying_transfers, list, struct usbi_transfer) {
-		struct timeval *cur_tv = &transfer->timeout;
-
-		/* if we've reached transfers of infinite timeout, then we have no
-		 * arming to do */
-		if (!timerisset(cur_tv))
-			return 0;
-
-		/* act on first transfer that is not already cancelled */
-		if (!(transfer->flags & USBI_TRANSFER_TIMED_OUT)) {
-			int r;
-			const struct itimerspec it = { {0, 0},
-				{ cur_tv->tv_sec, cur_tv->tv_usec * 1000 } };
-			usbi_dbg("next timeout originally %dms", USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer)->timeout);
-			r = timerfd_settime(ctx->timerfd, TFD_TIMER_ABSTIME, &it, NULL);
-			if (r < 0)
-				return LIBUSB_ERROR_OTHER;
-			return 1;
-		}
-	}
-
-	return 0;
-}
-#else
-static int disarm_timerfd(struct libusb_context *ctx)
-{
-	(void)ctx;
-	return 0;
-}
-static int arm_timerfd_for_next_timeout(struct libusb_context *ctx)
-{
-	(void)ctx;
-	return 0;
-}
-#endif
-
 /* Handle completion of a transfer (completion might be an error condition).
  * This will invoke the user-supplied callback function, which may end up
  * freeing the transfer. Therefore you cannot use the transfer structure
@@ -1459,14 +1460,8 @@ int usbi_handle_transfer_completion(struct usbi_transfer *itransfer,
 	if (usbi_using_timerfd(ctx))
 		r = arm_timerfd_for_next_timeout(ctx);
 	usbi_mutex_unlock(&ctx->flying_transfers_lock);
-
-	if (usbi_using_timerfd(ctx)) {
-		if (r < 0)
-			return r;
-		r = disarm_timerfd(ctx);
-		if (r < 0)
-			return r;
-	}
+	if (usbi_using_timerfd(ctx) && (r < 0))
+		return r;
 
 	if (status == LIBUSB_TRANSFER_COMPLETED
 			&& transfer->flags & LIBUSB_TRANSFER_SHORT_NOT_OK) {
@@ -1829,10 +1824,6 @@ static int handle_timerfd_trigger(struct libusb_context *ctx)
 {
 	int r;
 
-	r = disarm_timerfd(ctx);
-	if (r < 0)
-		return r;
-
 	usbi_mutex_lock(&ctx->flying_transfers_lock);
 
 	/* process the timeout that just happened */
diff --git a/libusb/version_nano.h b/libusb/version_nano.h
index fbf5397..a43c750 100644
--- a/libusb/version_nano.h
+++ b/libusb/version_nano.h
@@ -1 +1 @@
-#define LIBUSB_NANO 10543
+#define LIBUSB_NANO 10544