Merge pull request #135 from kevinmehall/errors

Error handling fixes

src/device.rs

@@ -10,7 +10,7 @@ use crate::{
     },
     DeviceInfo, Error, MaybeFuture, Speed,
 };
-use log::error;
+use log::{error, warn};
 use std::{
     future::{poll_fn, Future},
     io::ErrorKind,
@@ -635,10 +635,26 @@ impl<EpType: BulkOrInterrupt, Dir: EndpointDirection> Endpoint<EpType, Dir> {
     /// For an OUT transfer, the buffer's `len` is the number of bytes
     /// initialized, which will be sent to the device.
     ///
-    /// For an IN transfer, the buffer's `requested_len` is the number of
-    /// bytes requested. It must be a multiple of the endpoint's [maximum packet
-    /// size][`Self::max_packet_size`].
+    /// For an IN transfer, the buffer's `requested_len` is the number of bytes
+    /// requested. It must be a nonzero multiple of the endpoint's [maximum
+    /// packet size][`Self::max_packet_size`] or the transfer will fail with
+    /// `TransferError::InvalidArgument`. Up to `requested_len /
+    /// max_packet_size` packets will be received, ending early when any packet
+    /// is shorter than `max_packet_size`.
     pub fn submit(&mut self, buf: Buffer) {
+        if Dir::DIR == Direction::In {
+            let req_len = buf.requested_len();
+            if req_len == 0 || req_len % self.max_packet_size() != 0 {
+                warn!(
+                    "Submitting transfer with length {req_len} which is not a multiple of max packet size {} on IN endpoint {:02x}",
+                    self.max_packet_size(),
+                    self.endpoint_address(),
+                );
+
+                return self.backend.submit_err(buf, TransferError::InvalidArgument);
+            }
+        }
+
         self.backend.submit(buf)
     }
 
@@ -647,6 +663,12 @@ impl<EpType: BulkOrInterrupt, Dir: EndpointDirection> Endpoint<EpType, Dir> {
     /// This future is cancel-safe: it can be cancelled and re-created without
     /// side effects, enabling its use in `select!{}` or similar.
     ///
+    /// An OUT transfer completes when the specified data has been sent or an
+    /// error occurs. An IN transfer completes when a packet smaller than
+    /// `max_packet_size` is received, the full `requested_len` is received
+    /// (without waiting for or consuming any subsequent zero-length packet), or
+    /// an error occurs.
+    ///
     /// ## Panics
     /// * if there are no transfers pending (that is, if [`Self::pending()`]
     ///   would return 0).

src/platform/linux_usbfs/device.rs

@@ -691,18 +691,30 @@ impl LinuxEndpoint {
         }
     }
 
-    pub(crate) fn submit(&mut self, data: Buffer) {
-        let mut transfer = self.idle_transfer.take().unwrap_or_else(|| {
+    fn get_transfer(&mut self) -> Idle<TransferData> {
+        self.idle_transfer.take().unwrap_or_else(|| {
             Idle::new(
                 self.inner.clone(),
                 super::TransferData::new(self.inner.address, self.inner.ep_type),
             )
-        });
+        })
+    }
+
+    pub(crate) fn submit(&mut self, data: Buffer) {
+        let mut transfer = self.get_transfer();
         transfer.set_buffer(data);
         self.pending
             .push_back(self.inner.interface.device.submit(transfer));
     }
 
+    pub(crate) fn submit_err(&mut self, data: Buffer, error: TransferError) {
+        assert_eq!(error, TransferError::InvalidArgument);
+        let mut transfer = self.get_transfer();
+        transfer.set_buffer(data);
+        transfer.urb_mut().status = Errno::INVAL.raw_os_error();
+        self.pending.push_back(transfer.simulate_complete());
+    }
+
     pub(crate) fn poll_next_complete(&mut self, cx: &mut Context) -> Poll<Completion> {
         self.inner.notify.subscribe(cx);
         if let Some(mut transfer) = take_completed_from_queue(&mut self.pending) {

src/platform/linux_usbfs/mod.rs

@@ -31,6 +31,7 @@ fn errno_to_transfer_error(e: Errno) -> TransferError {
         Errno::PROTO | Errno::ILSEQ | Errno::OVERFLOW | Errno::COMM | Errno::TIME => {
             TransferError::Fault
         }
-        _ => TransferError::Unknown(e.raw_os_error()),
+        Errno::INVAL => TransferError::InvalidArgument,
+        _ => TransferError::Unknown(e.raw_os_error() as u32),
     }
 }

src/platform/macos_iokit/device.rs

@@ -466,25 +466,30 @@ impl MacEndpoint {
         );
     }
 
-    pub(crate) fn submit(&mut self, buffer: Buffer) {
+    fn make_transfer(&mut self, buffer: Buffer) -> Idle<TransferData> {
         let mut transfer = self
             .idle_transfer
             .take()
             .unwrap_or_else(|| Idle::new(self.inner.clone(), super::TransferData::new()));
 
         let buffer = ManuallyDrop::new(buffer);
-        let endpoint = self.inner.address;
-        let dir = Direction::from_address(endpoint);
-        let pipe_ref = self.inner.pipe_ref;
-
         transfer.buf = buffer.ptr;
         transfer.capacity = buffer.capacity;
         transfer.actual_len = 0;
-        let req_len = match dir {
+        let req_len = match Direction::from_address(self.inner.address) {
             Direction::Out => buffer.len,
             Direction::In => buffer.requested_len,
         };
         transfer.requested_len = req_len;
+        transfer
+    }
+
+    pub(crate) fn submit(&mut self, buffer: Buffer) {
+        let transfer = self.make_transfer(buffer);
+        let endpoint = self.inner.address;
+        let dir = Direction::from_address(endpoint);
+        let req_len = transfer.requested_len;
+        let buf_ptr = transfer.buf;
 
         let transfer = transfer.pre_submit();
         let ptr = transfer.as_ptr();
@@ -494,9 +499,9 @@ impl MacEndpoint {
                 Direction::Out => call_iokit_function!(
                     self.inner.interface.interface.raw,
                     WritePipeAsync(
-                        pipe_ref,
-                        buffer.ptr as *mut c_void,
-                        buffer.len,
+                        self.inner.pipe_ref,
+                        buf_ptr as *mut c_void,
+                        req_len,
                         transfer_callback,
                         ptr as *mut c_void
                     )
@@ -504,9 +509,9 @@ impl MacEndpoint {
                 Direction::In => call_iokit_function!(
                     self.inner.interface.interface.raw,
                     ReadPipeAsync(
-                        pipe_ref,
-                        buffer.ptr as *mut c_void,
-                        buffer.requested_len,
+                        self.inner.pipe_ref,
+                        buf_ptr as *mut c_void,
+                        req_len,
                         transfer_callback,
                         ptr as *mut c_void
                     )
@@ -519,7 +524,7 @@ impl MacEndpoint {
                 "Submitted {dir:?} transfer {ptr:?} of len {req_len} on endpoint {endpoint:02X}"
             );
         } else {
-            error!("Failed to submit transfer {ptr:?} of len {req_len} on endpoint {endpoint:02X}: {res:x}");
+            error!("Failed to submit {dir:?} transfer {ptr:?} of len {req_len} on endpoint {endpoint:02X}: {res:x}");
             unsafe {
                 // Complete the transfer in the place of the callback
                 (*ptr).status = res;
@@ -530,6 +535,13 @@ impl MacEndpoint {
         self.pending.push_back(transfer);
     }
 
+    pub(crate) fn submit_err(&mut self, buffer: Buffer, err: TransferError) {
+        assert_eq!(err, TransferError::InvalidArgument);
+        let mut transfer = self.make_transfer(buffer);
+        transfer.status = io_kit_sys::ret::kIOReturnBadArgument;
+        self.pending.push_back(transfer.simulate_complete());
+    }
+
     pub(crate) fn poll_next_complete(&mut self, cx: &mut Context) -> Poll<Completion> {
         self.inner.notify.subscribe(cx);
         if let Some(mut transfer) = take_completed_from_queue(&mut self.pending) {

src/platform/macos_iokit/iokit_c.rs

@@ -61,8 +61,6 @@ pub(crate) const kIOUSBTransactionTimeout: c_int = SYS_IOKIT | SUB_IOKIT_USB | 0
 
 pub(crate) const kIOUSBFindInterfaceDontCare: UInt16 = 0xFFFF;
 
-//
-
 //
 // Type aliases.
 //

src/platform/macos_iokit/mod.rs

@@ -29,8 +29,11 @@ fn status_to_transfer_result(status: IOReturn) -> Result<(), TransferError> {
     match status {
         io_kit_sys::ret::kIOReturnSuccess | io_kit_sys::ret::kIOReturnUnderrun => Ok(()),
         io_kit_sys::ret::kIOReturnNoDevice => Err(TransferError::Disconnected),
-        io_kit_sys::ret::kIOReturnAborted => Err(TransferError::Cancelled),
+        io_kit_sys::ret::kIOReturnAborted | iokit_c::kIOUSBTransactionTimeout => {
+            Err(TransferError::Cancelled)
+        }
         iokit_c::kIOUSBPipeStalled => Err(TransferError::Stall),
-        _ => Err(TransferError::Unknown(status as i32)),
+        io_kit_sys::ret::kIOReturnBadArgument => Err(TransferError::InvalidArgument), // used for `submit_err`
+        _ => Err(TransferError::Unknown(status as u32)),
     }
 }

src/platform/windows_winusb/device.rs

@@ -21,7 +21,10 @@ use windows_sys::Win32::{
         WinUsb_SetPipePolicy, WinUsb_WritePipe, USB_DEVICE_DESCRIPTOR, WINUSB_INTERFACE_HANDLE,
         WINUSB_SETUP_PACKET,
     },
-    Foundation::{GetLastError, ERROR_IO_PENDING, ERROR_NOT_FOUND, FALSE, HANDLE, TRUE},
+    Foundation::{
+        GetLastError, ERROR_BAD_COMMAND, ERROR_DEVICE_NOT_CONNECTED, ERROR_FILE_NOT_FOUND,
+        ERROR_IO_PENDING, ERROR_NOT_FOUND, ERROR_NO_SUCH_DEVICE, FALSE, HANDLE, TRUE,
+    },
     System::IO::{CancelIoEx, OVERLAPPED},
 };
 
@@ -409,10 +412,6 @@ impl WindowsInterface {
         data: ControlIn,
         timeout: Duration,
     ) -> impl MaybeFuture<Output = Result<Vec<u8>, TransferError>> {
-        if data.recipient == Recipient::Interface && data.index as u8 != self.interface_number {
-            warn!("WinUSB sends interface number instead of passed `index` when performing a control transfer with `Recipient::Interface`");
-        }
-
         let mut t = TransferData::new(0x80);
         t.set_buffer(Buffer::new(data.length as usize));
 
@@ -438,10 +437,6 @@ impl WindowsInterface {
         data: ControlOut,
         timeout: Duration,
     ) -> impl MaybeFuture<Output = Result<(), TransferError>> {
-        if data.recipient == Recipient::Interface && data.index as u8 != self.interface_number {
-            warn!("WinUSB sends interface number instead of passed `index` when performing a control transfer with `Recipient::Interface`");
-        }
-
         let mut t = TransferData::new(0x00);
         t.set_buffer(Buffer::from(data.data.to_vec()));
 
@@ -540,6 +535,7 @@ impl WindowsInterface {
         let len = t.request_len;
         let buf = t.buf;
         t.overlapped.InternalHigh = 0;
+        t.error_from_submit = Ok(());
 
         let t = t.pre_submit();
         let ptr = t.as_ptr();
@@ -580,6 +576,15 @@ impl WindowsInterface {
         let len = t.request_len;
         let buf = t.buf;
         t.overlapped.InternalHigh = 0;
+        t.error_from_submit = Ok(());
+
+        if pkt.RequestType & 0x1f == Recipient::Interface as u8
+            && pkt.Index as u8 != self.interface_number
+        {
+            warn!("WinUSB requires control transfer with `Recipient::Interface` to pass the interface number in `index`");
+            t.error_from_submit = Err(TransferError::InvalidArgument);
+            return t.simulate_complete();
+        }
 
         let t = t.pre_submit();
         let ptr = t.as_ptr();
@@ -613,7 +618,13 @@ impl WindowsInterface {
             // Safety: Transfer was not submitted, so we still own it
             // and must complete it in place of the event thread.
             unsafe {
-                (*t.as_ptr()).overlapped.Internal = err as _;
+                (*t.as_ptr()).error_from_submit = match err {
+                    ERROR_BAD_COMMAND
+                    | ERROR_FILE_NOT_FOUND
+                    | ERROR_DEVICE_NOT_CONNECTED
+                    | ERROR_NO_SUCH_DEVICE => Err(TransferError::Disconnected),
+                    other => Err(TransferError::Unknown(other)),
+                };
                 notify_completion::<TransferData>(t.as_ptr());
             }
         }
@@ -672,15 +683,26 @@ impl WindowsEndpoint {
         }
     }
 
-    pub(crate) fn submit(&mut self, buffer: Buffer) {
+    fn make_transfer(&mut self, buffer: Buffer) -> Idle<TransferData> {
         let mut t = self.idle_transfer.take().unwrap_or_else(|| {
             Idle::new(self.inner.clone(), TransferData::new(self.inner.address))
         });
         t.set_buffer(buffer);
+        t
+    }
+
+    pub(crate) fn submit(&mut self, buffer: Buffer) {
+        let t = self.make_transfer(buffer);
         let t = self.inner.interface.submit(t);
         self.pending.push_back(t);
     }
 
+    pub(crate) fn submit_err(&mut self, buffer: Buffer, err: TransferError) {
+        let mut t = self.make_transfer(buffer);
+        t.error_from_submit = Err(err);
+        self.pending.push_back(t.simulate_complete());
+    }
+
     pub(crate) fn poll_next_complete(&mut self, cx: &mut Context) -> Poll<Completion> {
         self.inner.notify.subscribe(cx);
         if let Some(mut transfer) = take_completed_from_queue(&mut self.pending) {

src/platform/windows_winusb/transfer.rs

@@ -24,6 +24,7 @@ pub struct TransferData {
     pub(crate) capacity: u32,
     pub(crate) request_len: u32,
     pub(crate) endpoint: u8,
+    pub(crate) error_from_submit: Result<(), TransferError>,
 }
 
 unsafe impl Send for TransferData {}
@@ -39,6 +40,7 @@ impl TransferData {
             capacity: 0,
             request_len: 0,
             endpoint,
+            error_from_submit: Ok(()),
         }
     }
 
@@ -62,19 +64,22 @@ impl TransferData {
     pub fn take_completion(&mut self, intf: &Interface) -> Completion {
         let mut actual_len: u32 = 0;
 
-        unsafe { GetOverlappedResult(intf.handle, &self.overlapped, &mut actual_len, 0) };
+        let status = self.error_from_submit.and_then(|()| {
+            unsafe { GetOverlappedResult(intf.handle, &self.overlapped, &mut actual_len, 0) };
 
-        let status = match unsafe { GetLastError() } {
-            ERROR_SUCCESS => Ok(()),
-            ERROR_GEN_FAILURE => Err(TransferError::Stall),
-            ERROR_REQUEST_ABORTED | ERROR_TIMEOUT | ERROR_SEM_TIMEOUT | ERROR_OPERATION_ABORTED => {
-                Err(TransferError::Cancelled)
+            match unsafe { GetLastError() } {
+                ERROR_SUCCESS => Ok(()),
+                ERROR_GEN_FAILURE => Err(TransferError::Stall),
+                ERROR_REQUEST_ABORTED
+                | ERROR_TIMEOUT
+                | ERROR_SEM_TIMEOUT
+                | ERROR_OPERATION_ABORTED => Err(TransferError::Cancelled),
+                ERROR_FILE_NOT_FOUND | ERROR_DEVICE_NOT_CONNECTED | ERROR_NO_SUCH_DEVICE => {
+                    Err(TransferError::Disconnected)
+                }
+                e => Err(TransferError::Unknown(e)),
             }
-            ERROR_FILE_NOT_FOUND | ERROR_DEVICE_NOT_CONNECTED | ERROR_NO_SUCH_DEVICE => {
-                Err(TransferError::Disconnected)
-            }
-            e => Err(TransferError::Unknown(e as i32)),
-        };
+        });
 
         let mut empty = ManuallyDrop::new(Vec::new());
         let ptr = mem::replace(&mut self.buf, empty.as_mut_ptr());

src/transfer/internal.rs

@@ -128,6 +128,12 @@ impl<P> Idle<P> {
             ptr: unsafe { NonNull::new_unchecked(Box::into_raw(self.0)) },
         }
     }
+
+    pub(crate) fn simulate_complete(self) -> Pending<P> {
+        Pending {
+            ptr: unsafe { NonNull::new_unchecked(Box::into_raw(self.0)) },
+        }
+    }
 }
 
 impl<P> Deref for Idle<P> {

src/transfer/mod.rs

@@ -39,13 +39,16 @@ pub enum TransferError {
     /// Hardware issue or protocol violation.
     Fault,
 
+    /// The request has an invalid argument or is not supported by this OS.
+    InvalidArgument,
+
     /// Unknown or OS-specific error.
     ///
     /// It won't be considered a breaking change to map unhandled errors from
     /// `Unknown` to one of the above variants. If you are matching on the
     /// OS-specific code because an error is not correctly mapped, please open
     /// an issue or pull request.
-    Unknown(i32),
+    Unknown(u32),
 }
 
 impl Display for TransferError {
@@ -55,8 +58,13 @@ impl Display for TransferError {
             TransferError::Stall => write!(f, "endpoint stalled"),
             TransferError::Disconnected => write!(f, "device disconnected"),
             TransferError::Fault => write!(f, "hardware fault or protocol violation"),
+            TransferError::InvalidArgument => write!(f, "invalid or unsupported argument"),
             TransferError::Unknown(e) => {
-                write!(f, "unknown error ({})", io::Error::from_raw_os_error(*e))
+                if cfg!(target_os = "macos") {
+                    write!(f, "unknown error (0x{e:08x})")
+                } else {
+                    write!(f, "unknown error ({e})")
+                }
             }
         }
     }
@@ -71,6 +79,7 @@ impl From<TransferError> for io::Error {
             TransferError::Stall => io::Error::new(io::ErrorKind::ConnectionReset, value),
             TransferError::Disconnected => io::Error::new(io::ErrorKind::ConnectionAborted, value),
             TransferError::Fault => io::Error::other(value),
+            TransferError::InvalidArgument => io::Error::new(io::ErrorKind::InvalidInput, value),
             TransferError::Unknown(_) => io::Error::other(value),
         }
     }