diff --git a/crates/ironrdp-displaycontrol/src/pdu/mod.rs b/crates/ironrdp-displaycontrol/src/pdu/mod.rs index a564657eed..0a5ebc0f72 100644 --- a/crates/ironrdp-displaycontrol/src/pdu/mod.rs +++ b/crates/ironrdp-displaycontrol/src/pdu/mod.rs @@ -174,6 +174,17 @@ impl DisplayControlCapabilities { pub fn max_monitor_area(&self) -> u64 { self.max_monitor_area } + + /// One 4K-area (3840x2400) monitor: the single-monitor capabilities every + /// existing server advertised before per-display monitor counts existed, + /// and the safe fallback for a monitor count that turns out to be invalid. + /// + /// # Panics + /// + /// Never: `(1, 3840, 2400)` is always within [`new`](Self::new)'s valid range. + pub fn single_monitor() -> Self { + Self::new(1, 3840, 2400).expect("(1, 3840, 2400) are always within the valid range") + } } impl Encode for DisplayControlCapabilities { diff --git a/crates/ironrdp-displaycontrol/src/server.rs b/crates/ironrdp-displaycontrol/src/server.rs index 8b845aed8e..fde336afbc 100644 --- a/crates/ironrdp-displaycontrol/src/server.rs +++ b/crates/ironrdp-displaycontrol/src/server.rs @@ -10,6 +10,22 @@ pub trait DisplayControlHandler: Send { fn monitor_layout(&self, layout: DisplayControlMonitorLayout) { debug!(?layout); } + + /// Capabilities advertised to the client when the channel starts. + /// + /// Defaults to [`DisplayControlCapabilities::single_monitor()`] (one monitor, + /// 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)`. 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 { + Ok(DisplayControlCapabilities::single_monitor()) + } } /// A server for the Display Control Virtual Channel. @@ -32,9 +48,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)]) } diff --git a/crates/ironrdp-server/src/display.rs b/crates/ironrdp-server/src/display.rs index 0807013ae6..3d232cf37c 100644 --- a/crates/ironrdp-server/src/display.rs +++ b/crates/ironrdp-server/src/display.rs @@ -335,6 +335,21 @@ pub trait RdpServerDisplay: Send { fn request_layout(&mut self, layout: DisplayControlMonitorLayout) { debug!(?layout, "Requesting layout") } + + /// The maximum number of monitors this display will honor in a client's + /// `request_layout()` call for the rest of the session. + /// + /// This is a capacity ceiling (MS-RDPEDISP `MaxNumMonitors`), not a report + /// of the current topology: the client may request any layout up to this + /// many monitors, and it is validated against this number, not the other + /// way around. Called once, before the Display Control Virtual Channel + /// opens, to build the capabilities the server advertises; the display's + /// actual monitor count may later grow or shrink within that ceiling + /// without a way to advertise a new one mid-session. Defaults to `1`, + /// matching every existing implementation's current behavior. + async fn monitor_count(&mut self) -> u32 { + 1 + } } #[cfg(test)] diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index 5c9151b443..53b0f163fe 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -14,7 +14,7 @@ use ironrdp_async::Framed; use ironrdp_cliprdr::CliprdrServer; use ironrdp_cliprdr::backend::ClipboardMessage; use ironrdp_core::{decode, encode_vec, impl_as_any}; -use ironrdp_displaycontrol::pdu::DisplayControlMonitorLayout; +use ironrdp_displaycontrol::pdu::{DisplayControlCapabilities, DisplayControlMonitorLayout}; use ironrdp_displaycontrol::server::{DisplayControlHandler, DisplayControlServer}; use ironrdp_dvc as dvc; #[cfg(feature = "usb")] @@ -552,11 +552,12 @@ impl dvc::DvcServerProcessor for AInputHandler {} struct DisplayControlBackend { display: Arc>>, + monitor_count: u32, } impl DisplayControlBackend { - fn new(display: Arc>>) -> Self { - Self { display } + fn new(display: Arc>>, monitor_count: u32) -> Self { + Self { display, monitor_count } } } @@ -565,6 +566,23 @@ impl DisplayControlHandler for DisplayControlBackend { let display = Arc::clone(&self.display); task::spawn_blocking(move || display.blocking_lock().request_layout(layout)); } + + fn capabilities(&self) -> PduResult { + // `DisplayControlCapabilities::new` only rejects `monitor_count > 1024`; 0 passes its + // validation (0 * 3840 * 2400 does not overflow) but would advertise a server that + // supports no monitors, so it is folded into the same out-of-range fallback below. + let monitor_count = if self.monitor_count == 0 { + warn!("RdpServerDisplay::monitor_count() returned 0, falling back to 1"); + 1 + } else { + self.monitor_count + }; + let capabilities = DisplayControlCapabilities::new(monitor_count, 3840, 2400).unwrap_or_else(|e| { + warn!(monitor_count, error = %e, "RdpServerDisplay::monitor_count() out of range, falling back to 1"); + DisplayControlCapabilities::single_monitor() + }); + Ok(capabilities) + } } #[cfg(feature = "usb")] @@ -1814,7 +1832,7 @@ impl RdpServer { self.gfx_handle.as_ref() } - fn attach_channels(&mut self, acceptor: &mut Acceptor) { + fn attach_channels(&mut self, acceptor: &mut Acceptor, monitor_count: u32) { if let Some(cliprdr_factory) = self.cliprdr_factory.as_deref() { let backend = cliprdr_factory.build_cliprdr_backend(); @@ -1835,7 +1853,7 @@ impl RdpServer { acceptor.attach_static_channel(RdpdrServer::new(backend)); } - let dcs_backend = DisplayControlBackend::new(Arc::clone(&self.display)); + let dcs_backend = DisplayControlBackend::new(Arc::clone(&self.display), monitor_count); let dvc = dvc::DrdynvcServer::new() .with_dynamic_channel(AInputHandler { handler: Arc::clone(&self.handler), @@ -1970,7 +1988,8 @@ impl RdpServer { // `accept_finalize`, which is where the acceptor first consumes the // static channel set (the MCS Connect Initial); `accept_begin`, already // done, stops at the security-upgrade gate before that. - self.attach_channels(&mut candidate.acceptor); + let monitor_count = self.display.lock().await.monitor_count().await; + self.attach_channels(&mut candidate.acceptor, monitor_count); self.finalize_negotiated(*candidate).await } @@ -2098,6 +2117,7 @@ impl RdpServer { self.display_suppressed.store(false, Ordering::Relaxed); let size = self.display.lock().await.size().await; + let monitor_count = self.display.lock().await.monitor_count().await; let capabilities = capabilities::capabilities(&self.opts, size); let mut pending = PendingConnection::new( self.opts.security.clone(), @@ -2107,7 +2127,7 @@ impl RdpServer { self.opts.honor_client_desktop_size, ); - self.attach_channels(pending.acceptor_mut()); + self.attach_channels(pending.acceptor_mut(), monitor_count); let Some(negotiated) = pending.negotiate_and_authenticate(stream, tls).await? else { return Ok(()); diff --git a/crates/ironrdp-testsuite-core/tests/displaycontrol/mod.rs b/crates/ironrdp-testsuite-core/tests/displaycontrol/mod.rs index 3130e460a4..fe57452122 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,74 @@ 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()); +} + +/// 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:?}"), + } +}