From e8b271d874998236215706dec7d2b302d4aee951 Mon Sep 17 00:00:00 2001 From: Kevin Mehall Date: Wed, 19 Feb 2025 07:18:36 -0700 Subject: [PATCH] Return option instead of panic in descriptor constructors --- src/descriptors.rs | 188 ++++++++++----------- src/device.rs | 5 +- src/platform/linux_usbfs/device.rs | 14 +- src/platform/windows_winusb/device.rs | 30 ++-- src/platform/windows_winusb/enumeration.rs | 7 +- 5 files changed, 115 insertions(+), 129 deletions(-) diff --git a/src/descriptors.rs b/src/descriptors.rs index a505a5b..4239319 100644 --- a/src/descriptors.rs +++ b/src/descriptors.rs @@ -182,37 +182,6 @@ macro_rules! descriptor_fields { } } -/// Check whether the buffer contains a valid device descriptor. -/// On success, it will return length of the descriptor, or returns `None`. -#[allow(unused)] -pub(crate) fn validate_device_descriptor(buf: &[u8]) -> Option { - if buf.len() < DESCRIPTOR_LEN_DEVICE as usize { - if buf.len() != 0 { - warn!( - "device descriptor buffer is {} bytes, need {}", - buf.len(), - DESCRIPTOR_LEN_DEVICE - ); - } - return None; - } - - if buf[0] < DESCRIPTOR_LEN_DEVICE { - warn!("invalid device descriptor bLength"); - return None; - } - - if buf[1] != DESCRIPTOR_TYPE_DEVICE { - warn!( - "device bDescriptorType is {}, not a device descriptor", - buf[1] - ); - return None; - } - - return Some(buf[0] as usize); -} - /// Information about a USB device. #[derive(Clone)] pub struct DeviceDescriptor([u8; DESCRIPTOR_LEN_DEVICE as usize]); @@ -223,14 +192,31 @@ impl DeviceDescriptor { /// You normally obtain a `DeviceDescriptor` from a [`Device`][crate::Device], but this allows creating /// one from your own descriptor bytes for tests. /// - /// ### Panics - /// * when the buffer is too short for a device descriptor - /// * when the first descriptor is not a device descriptor - pub fn new(buf: &[u8]) -> Self { - assert!(buf.len() >= DESCRIPTOR_LEN_DEVICE as usize); - assert!(buf[0] as usize >= DESCRIPTOR_LEN_DEVICE as usize); - assert!(buf[1] == DESCRIPTOR_TYPE_DEVICE); - Self(buf[0..DESCRIPTOR_LEN_DEVICE as usize].try_into().unwrap()) + /// This ignores any trailing data after the `bLength` specified in the descriptor. + pub fn new(buf: &[u8]) -> Option { + let Some(buf) = buf.get(0..DESCRIPTOR_LEN_DEVICE as usize) else { + if buf.len() != 0 { + warn!( + "device descriptor buffer is {} bytes, need {}", + buf.len(), + DESCRIPTOR_LEN_DEVICE + ); + } + return None; + }; + let buf: [u8; DESCRIPTOR_LEN_DEVICE as usize] = buf.try_into().ok()?; + if buf[0] < DESCRIPTOR_LEN_DEVICE { + warn!("invalid device descriptor bLength"); + None + } else if buf[1] != DESCRIPTOR_TYPE_DEVICE { + warn!( + "device bDescriptorType is {}, not a device descriptor", + buf[1] + ); + None + } else { + Some(Self(buf)) + } } /// Get the bytes of the descriptor. @@ -364,44 +350,6 @@ impl Debug for DeviceDescriptor { } } -#[allow(unused)] -pub(crate) fn validate_config_descriptor(buf: &[u8]) -> Option { - if buf.len() < DESCRIPTOR_LEN_CONFIGURATION as usize { - if buf.len() != 0 { - warn!( - "config descriptor buffer is {} bytes, need {}", - buf.len(), - DESCRIPTOR_LEN_CONFIGURATION - ); - } - return None; - } - - if buf[0] < DESCRIPTOR_LEN_CONFIGURATION { - warn!("invalid config descriptor bLength"); - return None; - } - - if buf[1] != DESCRIPTOR_TYPE_CONFIGURATION { - warn!( - "config bDescriptorType is {}, not a configuration descriptor", - buf[0] - ); - return None; - } - - let total_len = u16::from_le_bytes(buf[2..4].try_into().unwrap()) as usize; - if total_len < buf[0] as usize || total_len > buf.len() { - warn!( - "invalid config descriptor wTotalLen of {total_len} (buffer size is {bufsize})", - bufsize = buf.len() - ); - return None; - } - - Some(total_len) -} - /// Information about a USB configuration with access to all associated interfaces, endpoints, and other descriptors. #[derive(Clone)] pub struct ConfigurationDescriptor<'a>(&'a [u8]); @@ -412,16 +360,47 @@ impl<'a> ConfigurationDescriptor<'a> { /// You normally obtain a `Configuration` from a [`Device`][crate::Device], but this allows creating /// one from your own descriptor bytes for tests. /// - /// ### Panics - /// * when the buffer is too short for a configuration descriptor - /// * when the bLength and wTotalLength fields are longer than the buffer - /// * when the first descriptor is not a configuration descriptor - pub fn new(buf: &[u8]) -> ConfigurationDescriptor { - assert!(buf.len() >= DESCRIPTOR_LEN_CONFIGURATION as usize); - assert!(buf[0] as usize >= DESCRIPTOR_LEN_CONFIGURATION as usize); - assert!(buf[1] == DESCRIPTOR_TYPE_CONFIGURATION); - assert!(buf.len() == u16::from_le_bytes(buf[2..4].try_into().unwrap()) as usize); - ConfigurationDescriptor(buf) + /// This ignores any trailing data after the length specified in `wTotalLen`. + pub fn new(buf: &[u8]) -> Option { + if buf.len() < DESCRIPTOR_LEN_CONFIGURATION as usize { + if buf.len() != 0 { + warn!( + "config descriptor buffer is {} bytes, need {}", + buf.len(), + DESCRIPTOR_LEN_CONFIGURATION + ); + } + return None; + } + + if buf[0] < DESCRIPTOR_LEN_CONFIGURATION { + warn!("invalid config descriptor bLength"); + return None; + } + + if buf[1] != DESCRIPTOR_TYPE_CONFIGURATION { + warn!( + "config bDescriptorType is {}, not a configuration descriptor", + buf[0] + ); + return None; + } + + let total_len = u16::from_le_bytes(buf[2..4].try_into().unwrap()) as usize; + if total_len < buf[0] as usize || total_len > buf.len() { + warn!( + "invalid config descriptor wTotalLen of {total_len} (buffer size is {bufsize})", + bufsize = buf.len() + ); + return None; + } + + Some(ConfigurationDescriptor(&buf[..total_len])) + } + + #[allow(unused)] + pub(crate) fn new_unchecked(d: &'a [u8]) -> Self { + Self(d) } /// Get the configuration descriptor followed by all trailing interface and other descriptors. @@ -735,12 +714,13 @@ impl From for Error { /// Split a chain of concatenated configuration descriptors by `wTotalLength` #[allow(unused)] -pub(crate) fn parse_concatenated_config_descriptors(mut buf: &[u8]) -> impl Iterator { +pub(crate) fn parse_concatenated_config_descriptors( + mut buf: &[u8], +) -> impl Iterator { iter::from_fn(move || { - let total_len = validate_config_descriptor(buf)?; - let descriptors = &buf[..total_len]; - buf = &buf[total_len..]; - Some(descriptors) + let desc = ConfigurationDescriptor::new(buf)?; + buf = &buf[desc.0.len()..]; + Some(desc) }) } @@ -775,15 +755,19 @@ mod test_concatenated { #[test] fn test_empty() { assert_eq!( - parse_concatenated_config_descriptors(&[]).collect::>(), - Vec::<&[u8]>::new() + parse_concatenated_config_descriptors(&[]) + .collect::>() + .len(), + 0 ); } #[test] fn test_short() { assert_eq!( - parse_concatenated_config_descriptors(&[0]).collect::>(), + parse_concatenated_config_descriptors(&[0]) + .map(|d| d.descriptors().as_bytes()) + .collect::>(), Vec::<&[u8]>::new() ); } @@ -792,7 +776,8 @@ mod test_concatenated { fn test_invalid_total_len() { assert_eq!( parse_concatenated_config_descriptors(&[9, 2, 0, 0, 0, 0, 0, 0, 0]) - .collect::>(), + .map(|d| d.descriptors().as_bytes()) + .collect::>(), Vec::<&[u8]>::new() ); } @@ -801,13 +786,15 @@ mod test_concatenated { fn test_one_config() { assert_eq!( parse_concatenated_config_descriptors(&[9, 2, 9, 0, 0, 0, 0, 0, 0]) - .collect::>(), + .map(|d| d.descriptors().as_bytes()) + .collect::>(), vec![&[9, 2, 9, 0, 0, 0, 0, 0, 0]] ); assert_eq!( parse_concatenated_config_descriptors(&[9, 2, 13, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0]) - .collect::>(), + .map(|d| d.descriptors().as_bytes()) + .collect::>(), vec![&[9, 2, 13, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0]] ); } @@ -818,7 +805,8 @@ mod test_concatenated { parse_concatenated_config_descriptors(&[ 9, 2, 13, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 9, 2, 9, 0, 0, 0, 0, 0, 0 ]) - .collect::>(), + .map(|d| d.descriptors().as_bytes()) + .collect::>(), vec![ [9, 2, 13, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0].as_slice(), [9, 2, 9, 0, 0, 0, 0, 0, 0].as_slice() @@ -848,7 +836,7 @@ fn test_linux_root_hub() { let dev = DeviceDescriptor::new(&[ 0x12, 0x01, 0x00, 0x02, 0x09, 0x00, 0x01, 0x40, 0x6b, 0x1d, 0x02, 0x00, 0x10, 0x05, 0x03, 0x02, 0x01, 0x01 - ]); + ]).unwrap(); assert_eq!(dev.usb_version(), 0x0200); assert_eq!(dev.class(), 0x09); assert_eq!(dev.subclass(), 0x00); diff --git a/src/device.rs b/src/device.rs index 4f31cd7..5bcfa97 100644 --- a/src/device.rs +++ b/src/device.rs @@ -134,9 +134,7 @@ impl Device { /// /// This returns cached data and does not perform IO. pub fn configurations(&self) -> impl Iterator { - self.backend - .configuration_descriptors() - .map(ConfigurationDescriptor::new) + self.backend.configuration_descriptors() } /// Set the device configuration. @@ -604,7 +602,6 @@ impl Interface { .backend .device .configuration_descriptors() - .map(ConfigurationDescriptor::new) .find(|c| c.configuration_value() == active); configuration diff --git a/src/platform/linux_usbfs/device.rs b/src/platform/linux_usbfs/device.rs index 3aa1b8a..91eb101 100644 --- a/src/platform/linux_usbfs/device.rs +++ b/src/platform/linux_usbfs/device.rs @@ -25,7 +25,7 @@ use super::{ usbfs::{self, Urb}, SysfsPath, }; -use crate::descriptors::{validate_device_descriptor, ConfigurationDescriptor, DeviceDescriptor}; +use crate::descriptors::{ConfigurationDescriptor, DeviceDescriptor}; use crate::maybe_future::{blocking::Blocking, MaybeFuture}; use crate::platform::linux_usbfs::events::Watch; use crate::transfer::{ControlType, Recipient}; @@ -91,7 +91,7 @@ impl LinuxDevice { buf }; - let Some(_) = validate_device_descriptor(&descriptors) else { + let Some(_) = DeviceDescriptor::new(&descriptors) else { return Err(Error::new( ErrorKind::InvalidData, "invalid device descriptor", @@ -167,10 +167,12 @@ impl LinuxDevice { } pub(crate) fn device_descriptor(&self) -> DeviceDescriptor { - DeviceDescriptor::new(&self.descriptors) + DeviceDescriptor::new(&self.descriptors).unwrap() } - pub(crate) fn configuration_descriptors(&self) -> impl Iterator { + pub(crate) fn configuration_descriptors( + &self, + ) -> impl Iterator> { parse_concatenated_config_descriptors(&self.descriptors[DESCRIPTOR_LEN_DEVICE as usize..]) } @@ -421,9 +423,7 @@ impl LinuxDevice { // Assume the current configuration is the first one // See: https://github.com/libusb/libusb/blob/467b6a8896daea3d104958bf0887312c5d14d150/libusb/os/linux_usbfs.c#L865 let mut descriptors = - parse_concatenated_config_descriptors(&descriptors[DESCRIPTOR_LEN_DEVICE as usize..]) - .map(ConfigurationDescriptor::new); - + parse_concatenated_config_descriptors(&descriptors[DESCRIPTOR_LEN_DEVICE as usize..]); if let Some(config) = descriptors.next() { return Ok(config.configuration_value()); } diff --git a/src/platform/windows_winusb/device.rs b/src/platform/windows_winusb/device.rs index 976e82e..2eb6510 100644 --- a/src/platform/windows_winusb/device.rs +++ b/src/platform/windows_winusb/device.rs @@ -24,7 +24,7 @@ use windows_sys::Win32::{ use crate::{ descriptors::{ - validate_config_descriptor, DeviceDescriptor, DESCRIPTOR_LEN_DEVICE, + ConfigurationDescriptor, DeviceDescriptor, DESCRIPTOR_LEN_DEVICE, DESCRIPTOR_TYPE_CONFIGURATION, }, maybe_future::{blocking::Blocking, MaybeFuture, Ready}, @@ -68,22 +68,20 @@ impl WindowsDevice { // Safety: Windows API struct is repr(C), packed, and we're assuming Windows is little-endian let device_descriptor = unsafe { - DeviceDescriptor::new(&transmute::<_, [u8; DESCRIPTOR_LEN_DEVICE as usize]>( - connection_info.device_desc, - )) + &transmute::<_, [u8; DESCRIPTOR_LEN_DEVICE as usize]>(connection_info.device_desc) }; + let device_descriptor = DeviceDescriptor::new(device_descriptor) + .ok_or_else(|| Error::new(ErrorKind::InvalidData, "invalid device descriptor"))?; let num_configurations = connection_info.device_desc.bNumConfigurations; let config_descriptors = (0..num_configurations) .flat_map(|i| { - let res = hub_port.get_descriptor(DESCRIPTOR_TYPE_CONFIGURATION, i, 0); - match res { - Ok(v) => validate_config_descriptor(&v[..]).map(|_| v), - Err(e) => { - error!("Failed to read config descriptor {}: {}", i, e); - None - } - } + let d = hub_port + .get_descriptor(DESCRIPTOR_TYPE_CONFIGURATION, i, 0) + .inspect_err(|e| error!("Failed to read config descriptor {}: {}", i, e)) + .ok()?; + + ConfigurationDescriptor::new(&d).is_some().then_some(d) }) .collect(); @@ -110,8 +108,12 @@ impl WindowsDevice { self.active_config } - pub(crate) fn configuration_descriptors(&self) -> impl Iterator { - self.config_descriptors.iter().map(|d| &d[..]) + pub(crate) fn configuration_descriptors( + &self, + ) -> impl Iterator { + self.config_descriptors + .iter() + .map(|d| ConfigurationDescriptor::new_unchecked(&d[..])) } pub(crate) fn set_configuration( diff --git a/src/platform/windows_winusb/enumeration.rs b/src/platform/windows_winusb/enumeration.rs index a714b76..fd7b092 100644 --- a/src/platform/windows_winusb/enumeration.rs +++ b/src/platform/windows_winusb/enumeration.rs @@ -15,8 +15,8 @@ use windows_sys::Win32::Devices::{ use crate::{ descriptors::{ - decode_string_descriptor, language_id::US_ENGLISH, validate_config_descriptor, - ConfigurationDescriptor, DESCRIPTOR_TYPE_CONFIGURATION, DESCRIPTOR_TYPE_STRING, + decode_string_descriptor, language_id::US_ENGLISH, ConfigurationDescriptor, + DESCRIPTOR_TYPE_CONFIGURATION, DESCRIPTOR_TYPE_STRING, }, maybe_future::{blocking::Blocking, MaybeFuture}, BusInfo, DeviceInfo, Error, InterfaceInfo, UsbControllerType, @@ -206,8 +206,7 @@ fn list_interfaces_from_desc(hub_port: &HubPort, active_config: u8) -> Option