Fix incorrect alignment in allocation in libusb_alloc_transfer

After being suspicous of some code I was browsing, I tried changing
PTR_ALIGN to align to 4096 bytes instead of just to pointer size (8
bytes), then enabled ASan then ran the xusb example, and it crashed.
What ensued was a big code review of that function and related code.

- reviewed use of macros like USBI_TRANSFER_TO_LIBUSB_TRANSFER

- introduced new macros TRANSFER_PRIV_TO_USBI_TRANSFER and
USBI_TRANSFER_TO_TRANSFER_PRIV to do pointer offset conversions

- introduced some temporary variables, especially for
USBI_TRANSFER_TO_LIBUSB_TRANSFER results

- move some variable assignment and declaration together

- replaced a few uses of PTR_ALIGN to instead use the alignment macros

libusb/io.c

@@ -1244,8 +1244,8 @@ void usbi_io_exit(struct libusb_context *ctx)
 
 static void calculate_timeout(struct usbi_transfer *itransfer)
 {
-	unsigned int timeout =
-		USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer)->timeout;
+	struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
+	unsigned int timeout = transfer->timeout;
 
 	if (!timeout) {
 		TIMESPEC_CLEAR(&itransfer->timeout);
@@ -1289,30 +1289,25 @@ DEFAULT_VISIBILITY
 struct libusb_transfer * LIBUSB_CALL libusb_alloc_transfer(
 	int iso_packets)
 {
-	size_t priv_size;
-	size_t alloc_size;
-	unsigned char *ptr;
-	struct usbi_transfer *itransfer;
-	struct libusb_transfer *transfer;
-
 	assert(iso_packets >= 0);
 	if (iso_packets < 0)
 		return NULL;
 
-	priv_size = PTR_ALIGN(usbi_backend.transfer_priv_size);
-	alloc_size = priv_size
-		+ sizeof(struct usbi_transfer)
-		+ sizeof(struct libusb_transfer)
-		+ (sizeof(struct libusb_iso_packet_descriptor) * (size_t)iso_packets);
-	ptr = calloc(1, alloc_size);
+	size_t priv_size = PTR_ALIGN(usbi_backend.transfer_priv_size);
+	size_t usbi_transfer_size = PTR_ALIGN(sizeof(struct usbi_transfer));
+	size_t libusb_transfer_size = PTR_ALIGN(sizeof(struct libusb_transfer));
+	size_t iso_packets_size = sizeof(struct libusb_iso_packet_descriptor) * (size_t)iso_packets;
+	size_t alloc_size = priv_size + usbi_transfer_size + libusb_transfer_size + iso_packets_size;
+	unsigned char *ptr = calloc(1, alloc_size);
 	if (!ptr)
 		return NULL;
 
-	itransfer = (struct usbi_transfer *)(ptr + priv_size);
+	struct usbi_transfer *itransfer = (struct usbi_transfer *)(ptr + priv_size);
 	itransfer->num_iso_packets = iso_packets;
 	itransfer->priv = ptr;
 	usbi_mutex_init(&itransfer->lock);
-	transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
+	struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
+
 	return transfer;
 }
 
@@ -1335,24 +1330,19 @@ struct libusb_transfer * LIBUSB_CALL libusb_alloc_transfer(
  */
 void API_EXPORTED libusb_free_transfer(struct libusb_transfer *transfer)
 {
-	struct usbi_transfer *itransfer;
-	size_t priv_size;
-	unsigned char *ptr;
-
 	if (!transfer)
 		return;
 
 	usbi_dbg(TRANSFER_CTX(transfer), "transfer %p", (void *) transfer);
 	if (transfer->flags & LIBUSB_TRANSFER_FREE_BUFFER)
 		free(transfer->buffer);
 
-	itransfer = LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer);
+	struct usbi_transfer *itransfer = LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer);
 	usbi_mutex_destroy(&itransfer->lock);
 	if (itransfer->dev)
 		libusb_unref_device(itransfer->dev);
 
-	priv_size = PTR_ALIGN(usbi_backend.transfer_priv_size);
-	ptr = (unsigned char *)itransfer - priv_size;
+	unsigned char *ptr = USBI_TRANSFER_TO_TRANSFER_PRIV(itransfer);
 	assert(ptr == itransfer->priv);
 	free(ptr);
 }
@@ -1380,7 +1370,8 @@ static int arm_timer_for_next_timeout(struct libusb_context *ctx)
 
 		/* act on first transfer that has not already been handled */
 		if (!(itransfer->timeout_flags & (USBI_TRANSFER_TIMEOUT_HANDLED | USBI_TRANSFER_OS_HANDLES_TIMEOUT))) {
-			usbi_dbg(ctx, "next timeout originally %ums", USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer)->timeout);
+			struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
+			usbi_dbg(ctx, "next timeout originally %ums", transfer->timeout);
 			return usbi_arm_timer(&ctx->timer, cur_ts);
 		}
 	}
@@ -1442,8 +1433,9 @@ static int add_to_flying_list(struct usbi_transfer *itransfer)
 	if (first && usbi_using_timer(ctx) && TIMESPEC_IS_SET(timeout)) {
 		/* if this transfer has the lowest timeout of all active transfers,
 		 * rearm the timer with this transfer's timeout */
+		struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
 		usbi_dbg(ctx, "arm timer for timeout in %ums (first in line)",
-			USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer)->timeout);
+			transfer->timeout);
 		r = usbi_arm_timer(&ctx->timer, timeout);
 	}
 #else
@@ -2837,7 +2829,8 @@ void usbi_handle_disconnect(struct libusb_device_handle *dev_handle)
 		to_cancel = NULL;
 		usbi_mutex_lock(&ctx->flying_transfers_lock);
 		for_each_transfer(ctx, cur) {
-			if (USBI_TRANSFER_TO_LIBUSB_TRANSFER(cur)->dev_handle == dev_handle) {
+			struct libusb_transfer *cur_transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(cur);
+			if (cur_transfer->dev_handle == dev_handle) {
 				usbi_mutex_lock(&cur->lock);
 				if (cur->state_flags & USBI_TRANSFER_IN_FLIGHT)
 					to_cancel = cur;
@@ -2852,8 +2845,9 @@ void usbi_handle_disconnect(struct libusb_device_handle *dev_handle)
 		if (!to_cancel)
 			break;
 
+		struct libusb_transfer *transfer_to_cancel = USBI_TRANSFER_TO_LIBUSB_TRANSFER(to_cancel);
 		usbi_dbg(ctx, "cancelling transfer %p from disconnect",
-			 (void *) USBI_TRANSFER_TO_LIBUSB_TRANSFER(to_cancel));
+			 (void *) transfer_to_cancel);
 
 		usbi_mutex_lock(&to_cancel->lock);
 		usbi_backend.clear_transfer_priv(to_cancel);

libusb/libusbi.h

@@ -563,8 +563,11 @@ void usbi_get_real_time(struct timespec *tp);
  * 2. struct usbi_transfer
  * 3. struct libusb_transfer (which includes iso packets) [variable size]
  *
- * from a libusb_transfer, you can get the usbi_transfer by rewinding the
- * appropriate number of bytes.
+ * You can convert between them with the macros:
+ *  TRANSFER_PRIV_TO_USBI_TRANSFER
+ *  USBI_TRANSFER_TO_TRANSFER_PRIV
+ *  USBI_TRANSFER_TO_LIBUSB_TRANSFER
+ *  LIBUSB_TRANSFER_TO_USBI_TRANSFER
  */
 
 struct usbi_transfer {
@@ -617,10 +620,21 @@ enum usbi_transfer_timeout_flags {
 	USBI_TRANSFER_TIMED_OUT = 1U << 2,
 };
 
+#define TRANSFER_PRIV_TO_USBI_TRANSFER(transfer_priv) \
+	((struct usbi_transfer *)			\
+	 ((unsigned char *)(transfer_priv)	\
+	  + PTR_ALIGN(sizeof(*transfer_priv))))
+
+#define USBI_TRANSFER_TO_TRANSFER_PRIV(itransfer) \
+	((unsigned char *)			\
+	 ((unsigned char *)(itransfer)	\
+	  - PTR_ALIGN(usbi_backend.transfer_priv_size)))
+
 #define USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer)	\
 	((struct libusb_transfer *)			\
 	 ((unsigned char *)(itransfer)			\
 	  + PTR_ALIGN(sizeof(struct usbi_transfer))))
+
 #define LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer)	\
 	((struct usbi_transfer *)			\
 	 ((unsigned char *)(transfer)			\

libusb/os/windows_common.c

@@ -491,9 +491,10 @@ static unsigned __stdcall windows_iocp_thread(void *arg)
 			continue;
 		}
 
-		itransfer = (struct usbi_transfer *)((unsigned char *)transfer_priv + PTR_ALIGN(sizeof(*transfer_priv)));
+		itransfer = TRANSFER_PRIV_TO_USBI_TRANSFER(transfer_priv);
+		struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
 		usbi_dbg(ctx, "transfer %p completed, length %lu",
-			 USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer), ULONG_CAST(num_bytes));
+			 transfer, ULONG_CAST(num_bytes));
 		usbi_signal_transfer_completion(itransfer);
 	}
 
@@ -805,8 +806,9 @@ static int windows_handle_transfer_completion(struct usbi_transfer *itransfer)
 	else
 		result = GetLastError();
 
+	struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
 	usbi_dbg(ctx, "handling transfer %p completion with errcode %lu, length %lu",
-		 USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer), ULONG_CAST(result), ULONG_CAST(bytes_transferred));
+		 transfer, ULONG_CAST(result), ULONG_CAST(bytes_transferred));
 
 	switch (result) {
 	case NO_ERROR: