From 2ba8a6d24309488e11fd88fb08e81f06927e68ba Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Mon, 7 Sep 2026 20:00:37 -0500 Subject: [PATCH 1/4] feat(displaycontrol): let a server handler override its capabilities DisplayControlServer::start() hardcoded DisplayControlCapabilities::new(1, 3840, 2400), giving every server a fixed single-monitor cap with no way to advertise more. Added a default trait method mirroring the existing monitor_layout() pattern, so a handler serving multiple monitors can override it; anything that doesn't override it keeps today's exact behavior. --- crates/ironrdp-displaycontrol/src/server.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/ironrdp-displaycontrol/src/server.rs b/crates/ironrdp-displaycontrol/src/server.rs index 8b845aed8e..3db73433d1 100644 --- a/crates/ironrdp-displaycontrol/src/server.rs +++ b/crates/ironrdp-displaycontrol/src/server.rs @@ -10,6 +10,17 @@ pub trait DisplayControlHandler: Send { fn monitor_layout(&self, layout: DisplayControlMonitorLayout) { debug!(?layout); } + + /// Capabilities advertised to the client when the channel starts. + /// + /// Defaults to today's single-monitor values (`max_num_monitors = 1`, + /// factors `3840`/`2400`, i.e. one 4K-area monitor), so any existing + /// handler that doesn't override this keeps its current behavior. + /// A handler serving more than one monitor should override this to + /// return `DisplayControlCapabilities::new(monitor_count, 3840, 2400)`. + fn capabilities(&self) -> DisplayControlCapabilities { + DisplayControlCapabilities::new(1, 3840, 2400).expect("(1, 3840, 2400) are always within the valid range") + } } /// A server for the Display Control Virtual Channel. @@ -32,9 +43,7 @@ impl DvcProcessor for DisplayControlServer { } fn start(&mut self, _channel_id: u32) -> PduResult> { - let pdu: DisplayControlPdu = DisplayControlCapabilities::new(1, 3840, 2400) - .map_err(|e| decode_err!(e))? - .into(); + let pdu: DisplayControlPdu = self.handler.capabilities().into(); Ok(vec![Box::new(pdu)]) } From 9c6cd88f981c7c57500ea0c373cb95b9fe67816f Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Wed, 9 Sep 2026 14:17:23 -0500 Subject: [PATCH 2/4] docs(displaycontrol): rework capabilities() doc comment per review feedback Removes the today and current framing CBenoit flagged as unnecessary historical wording, and fixes the override example to show the DecodeResult being unwrapped, matching what the default implementation already does. --- crates/ironrdp-displaycontrol/src/server.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/ironrdp-displaycontrol/src/server.rs b/crates/ironrdp-displaycontrol/src/server.rs index 3db73433d1..620e4ec09d 100644 --- a/crates/ironrdp-displaycontrol/src/server.rs +++ b/crates/ironrdp-displaycontrol/src/server.rs @@ -13,11 +13,11 @@ pub trait DisplayControlHandler: Send { /// Capabilities advertised to the client when the channel starts. /// - /// Defaults to today's single-monitor values (`max_num_monitors = 1`, - /// factors `3840`/`2400`, i.e. one 4K-area monitor), so any existing - /// handler that doesn't override this keeps its current behavior. - /// A handler serving more than one monitor should override this to - /// return `DisplayControlCapabilities::new(monitor_count, 3840, 2400)`. + /// Defaults to a single-monitor value (`max_num_monitors = 1`, factors + /// `3840`/`2400`, i.e. one 4K-area monitor), so any handler that doesn't + /// override this keeps that behavior. + /// A handler serving more than one monitor should override this, e.g. + /// `DisplayControlCapabilities::new(monitor_count, 3840, 2400).expect("valid range")`. fn capabilities(&self) -> DisplayControlCapabilities { DisplayControlCapabilities::new(1, 3840, 2400).expect("(1, 3840, 2400) are always within the valid range") } From fa86832b310e7d1663910edd2bbe9958c13de1c8 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Thu, 10 Sep 2026 23:09:56 -0500 Subject: [PATCH 3/4] fix(displaycontrol): propagate an invalid capabilities override as an error capabilities() was declared infallible, so the only way an overrider could report invalid values (a monitor count or area factor combination DisplayControlCapabilities::new rejects) was to panic. That panic runs inside start(), a DvcProcessor callback, and unwinds through it rather than surfacing as a recoverable PduResult::Err. The trait method now returns PduResult, and start() propagates via the same decode_err! conversion process() already uses. Adds a regression test exercising start() with an out-of-range override. --- crates/ironrdp-displaycontrol/src/server.rs | 13 ++++++++---- .../tests/displaycontrol/mod.rs | 21 +++++++++++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/crates/ironrdp-displaycontrol/src/server.rs b/crates/ironrdp-displaycontrol/src/server.rs index 620e4ec09d..8b9f82fb73 100644 --- a/crates/ironrdp-displaycontrol/src/server.rs +++ b/crates/ironrdp-displaycontrol/src/server.rs @@ -17,9 +17,14 @@ pub trait DisplayControlHandler: Send { /// `3840`/`2400`, i.e. one 4K-area monitor), so any handler that doesn't /// override this keeps that behavior. /// A handler serving more than one monitor should override this, e.g. - /// `DisplayControlCapabilities::new(monitor_count, 3840, 2400).expect("valid range")`. - fn capabilities(&self) -> DisplayControlCapabilities { - DisplayControlCapabilities::new(1, 3840, 2400).expect("(1, 3840, 2400) are always within the valid range") + /// `DisplayControlCapabilities::new(monitor_count, 3840, 2400)`. Fallible + /// because [`DisplayControlCapabilities::new`] validates its arguments + /// (MS-RDPEDISP does not bound them, but the wire encoding does); an + /// overrider deriving values from runtime display state should propagate + /// that error rather than `expect` it, since this is called from + /// [`DvcProcessor::start`] and a panic there aborts the connection. + fn capabilities(&self) -> PduResult { + DisplayControlCapabilities::new(1, 3840, 2400).map_err(|e| decode_err!(e)) } } @@ -43,7 +48,7 @@ impl DvcProcessor for DisplayControlServer { } fn start(&mut self, _channel_id: u32) -> PduResult> { - let pdu: DisplayControlPdu = self.handler.capabilities().into(); + let pdu: DisplayControlPdu = self.handler.capabilities()?.into(); Ok(vec![Box::new(pdu)]) } diff --git a/crates/ironrdp-testsuite-core/tests/displaycontrol/mod.rs b/crates/ironrdp-testsuite-core/tests/displaycontrol/mod.rs index 3130e460a4..f6108992ff 100644 --- a/crates/ironrdp-testsuite-core/tests/displaycontrol/mod.rs +++ b/crates/ironrdp-testsuite-core/tests/displaycontrol/mod.rs @@ -3,7 +3,9 @@ use std::sync::{Arc, Mutex}; use ironrdp_core::decode; use ironrdp_displaycontrol::client::DisplayControlClient; use ironrdp_displaycontrol::pdu; +use ironrdp_displaycontrol::server::{DisplayControlHandler, DisplayControlServer}; use ironrdp_dvc::DvcProcessor as _; +use ironrdp_pdu::{PduResult, decode_err}; use ironrdp_testsuite_core::encode_decode_test; encode_decode_test! { @@ -217,3 +219,22 @@ fn client_process_rejects_trailing_bytes_after_caps() { ); assert!(!client.ready()); } + +struct OutOfRangeCapsHandler; + +impl DisplayControlHandler for OutOfRangeCapsHandler { + fn capabilities(&self) -> PduResult { + // More than 1024 monitors is rejected by DisplayControlCapabilities::new, per + // invalid_caps above. + pdu::DisplayControlCapabilities::new(2000, 100, 100).map_err(|e| decode_err!(e)) + } +} + +#[test] +fn server_start_propagates_an_invalid_capabilities_override_as_an_error() { + // A handler deriving capabilities from real runtime state can hit the wire-encoding bound + // DisplayControlCapabilities::new enforces; start() must surface that as a PduResult::Err + // rather than let a handler's own expect()/panic tear down the connection. + let mut server = DisplayControlServer::new(Box::new(OutOfRangeCapsHandler)); + assert!(server.start(0).is_err()); +} From 679d07b303ac8bf7175384ac703af4142b8116ea Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Fri, 11 Sep 2026 14:51:57 -0500 Subject: [PATCH 4/4] test(displaycontrol): cover the multi-monitor override and default paths The PR's core claim is that a handler can override capabilities() to advertise more than one monitor, but nothing verified a valid override's values were actually encoded into the DISPLAYCONTROL_CAPS_PDU returned by DisplayControlServer::start(). The one existing test only asserted that an invalid override errors, proving handler invocation and error propagation but not correct encoding. Added a positive-path test overriding capabilities() to (2, 3840, 2400) and decoding the resulting DVC message back to confirm the values reach the wire, plus a test pinning the default (1, 3840, 2400) capabilities() impl, which predates this PR as a hardcoded start() literal and was never covered by a test either. --- .../tests/displaycontrol/mod.rs | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/crates/ironrdp-testsuite-core/tests/displaycontrol/mod.rs b/crates/ironrdp-testsuite-core/tests/displaycontrol/mod.rs index f6108992ff..fe57452122 100644 --- a/crates/ironrdp-testsuite-core/tests/displaycontrol/mod.rs +++ b/crates/ironrdp-testsuite-core/tests/displaycontrol/mod.rs @@ -238,3 +238,55 @@ fn server_start_propagates_an_invalid_capabilities_override_as_an_error() { let mut server = DisplayControlServer::new(Box::new(OutOfRangeCapsHandler)); assert!(server.start(0).is_err()); } + +/// Decode a `DvcMessage` produced by `DisplayControlServer::start()` back into the +/// `DisplayControlPdu` it encodes, the same round trip a real client would perform. +fn decode_dvc(msg: &ironrdp_dvc::DvcMessage) -> pdu::DisplayControlPdu { + let bytes = ironrdp_core::encode_vec(msg.as_ref()).expect("encode dvc message"); + decode(&bytes).expect("decode dvc message") +} + +struct MultiMonitorCapsHandler; + +impl DisplayControlHandler for MultiMonitorCapsHandler { + fn capabilities(&self) -> PduResult { + pdu::DisplayControlCapabilities::new(2, 3840, 2400).map_err(|e| decode_err!(e)) + } +} + +#[test] +fn server_start_encodes_a_multi_monitor_override() { + // The motivating case for a fallible, overridable capabilities(): a handler + // advertising more than the hardcoded single-monitor default. Verify the + // override's values actually reach the encoded DISPLAYCONTROL_CAPS_PDU, not + // just that start() succeeds. + let mut server = DisplayControlServer::new(Box::new(MultiMonitorCapsHandler)); + let messages = server.start(0).expect("valid override should not error"); + assert_eq!(messages.len(), 1); + match decode_dvc(&messages[0]) { + pdu::DisplayControlPdu::Caps(caps) => { + assert_eq!(caps, pdu::DisplayControlCapabilities::new(2, 3840, 2400).unwrap()); + } + other => panic!("expected DisplayControlPdu::Caps, got {other:?}"), + } +} + +struct DefaultCapsHandler; + +impl DisplayControlHandler for DefaultCapsHandler {} + +#[test] +fn server_start_encodes_the_default_capabilities_when_not_overridden() { + // The default capabilities() impl (single monitor, 3840x2400) was the + // hardcoded start() literal before this PR and remains untested; pin it so a + // regression in the default constants or the .into() encoding step is caught. + let mut server = DisplayControlServer::new(Box::new(DefaultCapsHandler)); + let messages = server.start(0).expect("default capabilities should not error"); + assert_eq!(messages.len(), 1); + match decode_dvc(&messages[0]) { + pdu::DisplayControlPdu::Caps(caps) => { + assert_eq!(caps, pdu::DisplayControlCapabilities::new(1, 3840, 2400).unwrap()); + } + other => panic!("expected DisplayControlPdu::Caps, got {other:?}"), + } +}