fix(connector)!: remove arbitrary feature (#1273)

The connector types are unlikely to benefit from random value generation
for fuzzing purposes; a more targeted approach would be preferable if
the connector is fuzzed in the future. We’ll reevaluate later.

Author: CBenoit

Cargo.lock

@@ -18,7 +18,6 @@ test = false
 
 [features]
 default = []
-arbitrary = ["dep:arbitrary", "ironrdp-pdu/arbitrary"]
 qoi = ["ironrdp-pdu/qoi"]
 qoiz = ["ironrdp-pdu/qoiz"]
 
@@ -27,7 +26,6 @@ ironrdp-svc = { path = "../ironrdp-svc", version = "0.6" } # public
 ironrdp-core = { path = "../ironrdp-core", version = "0.1" } # public
 ironrdp-error = { path = "../ironrdp-error", version = "0.1" } # public
 ironrdp-pdu = { path = "../ironrdp-pdu", version = "0.7", features = ["std"] } # public
-arbitrary = { version = "1", features = ["derive"], optional = true } # public
 sspi = { version = "0.19", features = ["scard"] }
 url = "2.5" # public
 rand = { version = "0.9", features = ["std"] } # TODO: dependency injection?

crates/ironrdp-connector/Cargo.toml

@@ -12,7 +12,6 @@ use crate::{
 
 #[derive(Default, Debug)]
 #[non_exhaustive]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum ChannelConnectionState {
     #[default]
     Consumed,
@@ -56,7 +55,6 @@ impl State for ChannelConnectionState {
 }
 
 #[derive(Debug)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct ChannelConnectionSequence {
     pub state: ChannelConnectionState,
     pub channel_ids: Option<HashSet<u16>>,

crates/ironrdp-connector/src/channel_connection.rs

@@ -11,7 +11,6 @@ use crate::{ConnectorResult, Sequence, State, Written, general_err, legacy, reas
 
 #[derive(Default, Debug, Copy, Clone)]
 #[non_exhaustive]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum ConnectionFinalizationState {
     #[default]
     Consumed,
@@ -49,7 +48,6 @@ impl State for ConnectionFinalizationState {
 }
 
 #[derive(Debug, Copy, Clone)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct ConnectionFinalizationSequence {
     pub state: ConnectionFinalizationState,
     pub io_channel_id: u16,

crates/ironrdp-connector/src/connection_finalization.rs

@@ -34,7 +34,6 @@ pub use crate::license_exchange::LicenseCache;
 
 /// Provides user-friendly error messages for RDP negotiation failures
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct NegotiationFailure(ironrdp_pdu::nego::FailureCode);
 
 impl NegotiationFailure {
@@ -83,22 +82,19 @@ impl fmt::Display for NegotiationFailure {
 }
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct DesktopSize {
     pub width: u16,
     pub height: u16,
 }
 
 #[derive(Debug, Clone)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct BitmapConfig {
     pub lossy_compression: bool,
     pub color_depth: u32,
     pub codecs: BitmapCodecs,
 }
 
 #[derive(Debug, Clone)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct SmartCardIdentity {
     /// DER-encoded X509 certificate
     pub certificate: Vec<u8>,
@@ -113,7 +109,6 @@ pub struct SmartCardIdentity {
 }
 
 #[derive(Debug, Clone)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum Credentials {
     UsernamePassword {
         username: String,
@@ -142,7 +137,6 @@ impl Credentials {
 }
 
 #[derive(Debug, Clone)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub struct Config {
     /// The initial desktop size to request
     pub desktop_size: DesktopSize,
@@ -236,11 +230,6 @@ pub struct Config {
     pub enable_audio_playback: bool,
     pub performance_flags: PerformanceFlags,
 
-    // `Arc<dyn LicenseCache>` cannot be generated by `arbitrary`. Skipped via
-    // `arbitrary(default)` which yields `None`. License-cache-dependent paths
-    // are not exercised under fuzz; a fuzz-mode `LicenseCache` impl is a
-    // possible follow-up.
-    #[cfg_attr(feature = "arbitrary", arbitrary(default))]
     pub license_cache: Option<Arc<dyn LicenseCache>>,
 
     // For Timezone Redirection to sync the server's timezone with the client's.
@@ -304,7 +293,6 @@ impl State for () {
 }
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum Written {
     Nothing,
     Size(core::num::NonZeroUsize),
@@ -351,8 +339,6 @@ pub type ConnectorResult<T> = Result<T, ConnectorError>;
 
 #[non_exhaustive]
 #[derive(Debug)]
-// ConnectorErrorKind carries foreign error types (sspi::Error, ironrdp_error::Error)
-// that do not implement Arbitrary, so this enum is intentionally not annotated.
 pub enum ConnectorErrorKind {
     Encode(ironrdp_core::EncodeError),
     Decode(ironrdp_core::DecodeError),

crates/ironrdp-connector/src/lib.rs

@@ -15,7 +15,6 @@ use crate::{ConnectorResult, ConnectorResultExt as _, Sequence, State, Written,
 
 #[derive(Default, Debug)]
 #[non_exhaustive]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 pub enum LicenseExchangeState {
     #[default]
     Consumed,
@@ -65,29 +64,6 @@ pub struct LicenseExchangeSequence {
     pub license_cache: Arc<dyn LicenseCache>,
 }
 
-// `license_cache` is `Arc<dyn LicenseCache>` which cannot be derived. The hand-rolled impl
-// hardcodes `NoopLicenseCache`, so license-cache-dependent paths are not exercised under fuzz.
-// Reaching the license sequence is still useful coverage (MS-RDPELE is a historically
-// interesting target). A fuzz-mode `LicenseCache` impl is a possible follow-up.
-#[cfg(feature = "arbitrary")]
-impl<'a> arbitrary::Arbitrary<'a> for LicenseExchangeSequence {
-    fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {
-        Ok(Self {
-            state: LicenseExchangeState::arbitrary(u)?,
-            io_channel_id: u16::arbitrary(u)?,
-            username: String::arbitrary(u)?,
-            domain: Option::<String>::arbitrary(u)?,
-            hardware_id: [
-                u32::arbitrary(u)?,
-                u32::arbitrary(u)?,
-                u32::arbitrary(u)?,
-                u32::arbitrary(u)?,
-            ],
-            license_cache: Arc::new(NoopLicenseCache),
-        })
-    }
-}
-
 // Use RefUnwindSafe so that types that embed LicenseCache remain UnwindSafe
 pub trait LicenseCache: Sync + Send + Debug + RefUnwindSafe {
     fn get_license(&self, license_info: LicenseInformation) -> ConnectorResult<Option<Vec<u8>>>;

crates/ironrdp-connector/src/license_exchange.rs

@@ -46,7 +46,6 @@ To verify the feature compiles cleanly for a single crate:
 
 ```shell
 cargo check -p ironrdp-pdu --features arbitrary
-cargo check -p ironrdp-connector --features arbitrary
 ```
 
 The feature is also compatible with the `no_std + alloc` build path:
@@ -57,15 +56,13 @@ cargo check -p ironrdp-pdu --no-default-features --features arbitrary,alloc
 
 A handful of PDU types do not implement `Arbitrary`. They fall into two categories:
 
-- **Types with non-derivable fields** (e.g., `Arc<dyn LicenseCache>`,
-  `StaticChannelSet` keyed by `TypeId`). These are either skipped via
-  `#[arbitrary(default)]` on the containing struct's field, or hand-rolled
-  with a placeholder.
+- **Types with non-derivable fields** (e.g., `StaticChannelSet` keyed by `TypeId`).
+  These are either skipped via `#[arbitrary(default)]` on the containing struct's
+  field, or hand-rolled with a placeholder.
 - **Error types that are not part of the wire-protocol surface** (e.g.,
-  `ServerLicenseError`, `ConnectorErrorKind`). Most error enums fall here:
-  they are constructed locally rather than decoded from the wire, so the
-  fuzzer has no reason to generate them. The exception is wire-protocol
-  error PDUs such as `DisconnectProviderUltimatum` in `mcs.rs`, which do
-  implement `Arbitrary` because they are decoded from the wire.
+  `ServerLicenseError`). Most error enums fall here: they are constructed locally
+  rather than decoded from the wire, so the fuzzer has no reason to generate them.
+  The exception is wire-protocol error PDUs such as `DisconnectProviderUltimatum`
+  in `mcs.rs`, which do implement `Arbitrary` because they are decoded from the wire.
 
 Inline source comments mark each skip with the rationale.