diff --git a/crates/ironrdp-server/src/display.rs b/crates/ironrdp-server/src/display.rs index 3d232cf37c..660c7f1c20 100644 --- a/crates/ironrdp-server/src/display.rs +++ b/crates/ironrdp-server/src/display.rs @@ -350,6 +350,24 @@ pub trait RdpServerDisplay: Send { async fn monitor_count(&mut self) -> u32 { 1 } + + /// Whether to offer the Display Control Virtual Channel (MS-RDPEDISP) on + /// this connection. + /// + /// Called once per connection, before the dynamic channels are set up. + /// Return `false` when the display cannot honor a client layout request, + /// for example when it mirrors a fixed-size physical screen: MS-RDPEDISP + /// section 1.3 defines no reject message, so a client that sent a layout + /// the server cannot apply just waits for a reconfiguration that never + /// comes. Defaults to `true`, matching every existing implementation. + /// + /// A connection binder installs its own display during the capability + /// exchange, after this is asked. The display given to + /// `with_display_handler` therefore decides for every connection, and a + /// display installed by a binder can't withdraw the offer. + async fn offers_display_control(&mut self) -> bool { + true + } } #[cfg(test)] diff --git a/crates/ironrdp-server/src/server.rs b/crates/ironrdp-server/src/server.rs index 44c88e50c9..75ce7a3cb5 100644 --- a/crates/ironrdp-server/src/server.rs +++ b/crates/ironrdp-server/src/server.rs @@ -437,6 +437,14 @@ impl RdpServerDisplay for BoundDisplaySlot { } } + async fn offers_display_control(&mut self) -> bool { + if let Some(mut lease) = BoundDisplayLease::take(&self.bound) { + lease.display_mut().offers_display_control().await + } else { + self.default.offers_display_control().await + } + } + async fn updates(&mut self) -> ServerResult> { if let Some(mut lease) = BoundDisplayLease::take(&self.bound) { lease.display_mut().updates().await @@ -2406,7 +2414,7 @@ impl RdpServer { self.gfx_handle.as_ref() } - fn attach_channels(&mut self, acceptor: &mut Acceptor, monitor_count: u32) { + async fn attach_channels(&mut self, acceptor: &mut Acceptor) { if let Some(cliprdr_factory) = self.cliprdr_factory.as_deref() { let backend = cliprdr_factory.build_cliprdr_backend(); @@ -2427,13 +2435,28 @@ impl RdpServer { acceptor.attach_static_channel(RdpdrServer::new(backend)); } - 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), - active: Arc::clone(&self.advanced_input_active), - }) - .with_dynamic_channel(DisplayControlServer::new(Box::new(dcs_backend))); + let dvc = dvc::DrdynvcServer::new().with_dynamic_channel(AInputHandler { + handler: Arc::clone(&self.handler), + active: Arc::clone(&self.advanced_input_active), + }); + + // `monitor_count` is only asked when the channel is offered, since its + // contract is tied to the channel opening. Both queries share one guard, + // so no other task can interleave between them. + let display_control_monitors = { + let mut display = self.display.lock().await; + if display.offers_display_control().await { + Some(display.monitor_count().await) + } else { + None + } + }; + let dvc = if let Some(monitor_count) = display_control_monitors { + let dcs_backend = DisplayControlBackend::new(Arc::clone(&self.display), monitor_count); + dvc.with_dynamic_channel(DisplayControlServer::new(Box::new(dcs_backend))) + } else { + dvc + }; let dvc = { let echo_handle = self.echo_handle.clone(); @@ -2585,8 +2608,7 @@ 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. - let monitor_count = self.display.lock().await.monitor_count().await; - self.attach_channels(&mut candidate.acceptor, monitor_count); + self.attach_channels(&mut candidate.acceptor).await; self.finalize_negotiated(*candidate).await } @@ -2733,7 +2755,6 @@ impl RdpServer { self.discard_stale_session_events().await; 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(), @@ -2749,7 +2770,7 @@ impl RdpServer { }, ); - self.attach_channels(pending.acceptor_mut(), monitor_count); + self.attach_channels(pending.acceptor_mut()).await; let Some(negotiated) = pending.negotiate_and_authenticate(stream, tls).await? else { return Ok(()); diff --git a/crates/ironrdp-testsuite-extra/Cargo.toml b/crates/ironrdp-testsuite-extra/Cargo.toml index bb5d8cb085..8e444c0db8 100644 --- a/crates/ironrdp-testsuite-extra/Cargo.toml +++ b/crates/ironrdp-testsuite-extra/Cargo.toml @@ -24,7 +24,7 @@ harness = true [dev-dependencies] anyhow = "1.0" async-trait = "0.1" -ironrdp = { path = "../ironrdp", features = ["server", "pdu", "connector", "session", "svc", "dvc", "echo"] } +ironrdp = { path = "../ironrdp", features = ["server", "pdu", "connector", "session", "svc", "dvc", "echo", "displaycontrol"] } ironrdp-cfg.path = "../ironrdp-cfg" ironrdp-async.path = "../ironrdp-async" ironrdp-agent.path = "../ironrdp-agent" diff --git a/crates/ironrdp-testsuite-extra/tests/e2e.rs b/crates/ironrdp-testsuite-extra/tests/e2e.rs index fb060a8f12..96518f904a 100644 --- a/crates/ironrdp-testsuite-extra/tests/e2e.rs +++ b/crates/ironrdp-testsuite-extra/tests/e2e.rs @@ -362,6 +362,101 @@ async fn a_new_connection_starts_without_the_previous_sessions_events() { .await } +#[tokio::test] +async fn display_control_channel_is_created_when_the_display_offers_it() { + let (echo_started, display_control_started) = run_display_control_probe(true).await; + + assert!( + echo_started, + "the echo channel must open for the probe to mean anything" + ); + assert!( + display_control_started, + "a display that offers Display Control must get the channel created on the client" + ); +} + +#[tokio::test] +async fn display_control_channel_is_not_created_when_the_display_declines_it() { + let (echo_started, display_control_started) = run_display_control_probe(false).await; + + assert!( + echo_started, + "the echo channel must open for the probe to mean anything" + ); + assert!( + !display_control_started, + "a display that declines Display Control must not get the channel created on the client" + ); +} + +/// Connects a real client that registers a Display Control channel and an +/// echo channel, and reports whether each was created by the server. +/// +/// The server sends the Create request for every dynamic channel together, in +/// its reply to the client's capabilities. Once the echo channel has opened and +/// the connection then goes quiet, every one of them has been processed, so the +/// answer for Display Control is final whatever order the channels were +/// registered in. +async fn run_display_control_probe(offers_display_control: bool) -> (bool, bool) { + let echo_started = Arc::new(AtomicBool::new(false)); + let display_control_started = Arc::new(AtomicBool::new(false)); + + let connector_echo = Arc::clone(&echo_started); + let connector_display_control = Arc::clone(&display_control_started); + let observed_echo = Arc::clone(&echo_started); + + client_server_with_display( + move |rx| TestDisplay { + rx, + offers_display_control, + }, + None, + default_client_config(), + Vec::new(), + None, + move |connector| { + connector.with_static_channel( + DrdynvcClient::new() + .with_dynamic_channel(StartRecorder::new( + ironrdp::displaycontrol::CHANNEL_NAME, + connector_display_control, + )) + .with_dynamic_channel(StartRecorder::new(ironrdp::echo::CHANNEL_NAME, connector_echo)), + ) + }, + move |mut stage, _activation_factory, mut framed, display_tx, _echo_handle| async move { + let _display_tx = display_tx; + let mut image = DecodedImage::new(PixelFormat::RgbA32, DESKTOP_WIDTH, DESKTOP_HEIGHT); + + let deadline = Instant::now() + Duration::from_secs(5); + let mut settled = false; + while !settled && Instant::now() < deadline { + let read_result = tokio::time::timeout(Duration::from_millis(150), framed.read_pdu()).await; + let Ok(Ok((action, frame))) = read_result else { + settled = observed_echo.load(Ordering::Relaxed); + continue; + }; + + let outputs = stage.process(&mut image, action, &frame).expect("stage process"); + for output in outputs { + if let ActiveStageOutput::ResponseFrame(frame) = output { + framed.write_all(&frame).await.expect("write response frame"); + } + } + } + + (stage, framed) + }, + ) + .await; + + ( + echo_started.load(Ordering::Relaxed), + display_control_started.load(Ordering::Relaxed), + ) +} + #[tokio::test] async fn rdpdr_static_channel_announces_a_drive_and_completes_an_unsupported_create() { let fixture = RdpdrFixtureFactory::new(RdpdrFixtureOperation::UnsupportedCreate); @@ -528,6 +623,7 @@ impl RdpServerDisplayUpdates for TestDisplayUpdates { struct TestDisplay { rx: DisplayUpdatesRx, + offers_display_control: bool, } #[async_trait::async_trait] @@ -544,8 +640,43 @@ impl RdpServerDisplay for TestDisplay { rx: Arc::clone(&self.rx), })) } + + async fn offers_display_control(&mut self) -> bool { + self.offers_display_control + } +} + +/// Client-side dynamic channel that only records that the server created it. +struct StartRecorder { + name: &'static str, + started: Arc, +} + +impl StartRecorder { + fn new(name: &'static str, started: Arc) -> Self { + Self { name, started } + } +} + +impl_as_any!(StartRecorder); + +impl DvcProcessor for StartRecorder { + fn channel_name(&self) -> &str { + self.name + } + + fn start(&mut self, _channel_id: u32) -> pdu::PduResult> { + self.started.store(true, Ordering::Relaxed); + Ok(Vec::new()) + } + + fn process(&mut self, _channel_id: u32, _payload: &[u8]) -> pdu::PduResult> { + Ok(Vec::new()) + } } +impl DvcClientProcessor for StartRecorder {} + struct TestInputHandler; impl RdpServerInputHandler for TestInputHandler { fn keyboard(&mut self, _: KeyboardEvent) {} @@ -1012,6 +1143,43 @@ async fn client_server_impl( + 'static, Fut: Future>>)>, C: FnOnce(connector::ClientConnector) -> connector::ClientConnector + 'static, +{ + client_server_with_display( + |rx| TestDisplay { + rx, + offers_display_control: true, + }, + stale_events, + client_config, + static_channel_factories, + server_udp_addr, + connector_factory, + clientfn, + ) + .await; +} + +async fn client_server_with_display( + display_factory: DF, + stale_events: Option>, + client_config: connector::Config, + static_channel_factories: Vec>, + server_udp_addr: Option, + connector_factory: C, + clientfn: F, +) where + D: RdpServerDisplay + 'static, + DF: FnOnce(DisplayUpdatesRx) -> D, + F: FnOnce( + ActiveStage, + connector::connection_activation::ConnectionActivationFactory, + Framed>>, + UnboundedSender, + server::EchoServerHandle, + ) -> Fut + + 'static, + Fut: Future>>)>, + C: FnOnce(connector::ClientConnector) -> connector::ClientConnector + 'static, { // FIXME(@CBenoit): If this is really necessary, we may consider a non-global way of registering the subscriber; otherwise it’s unnecessary to register that. let _ = tracing_subscriber::fmt() @@ -1028,9 +1196,7 @@ async fn client_server_impl( .with_addr(([127, 0, 0, 1], 0)) .with_tls(acceptor) .with_input_handler(TestInputHandler) - .with_display_handler(TestDisplay { - rx: Arc::new(Mutex::new(display_rx)), - }); + .with_display_handler(display_factory(Arc::new(Mutex::new(display_rx)))); for factory in static_channel_factories { server_builder = server_builder.with_static_channel_factory(factory); } @@ -1169,6 +1335,7 @@ async fn egfx_moves_onto_the_udp_tunnel_with_soft_sync() { .with_input_handler(TestInputHandler) .with_display_handler(TestDisplay { rx: Arc::new(Mutex::new(display_rx)), + offers_display_control: true, }) .with_gfx_factory(Some(Box::new(TestGfxFactory { caps_tx }))) .with_udp_transport(udp_addr) @@ -1635,6 +1802,7 @@ fn auto_reconnect_server(on_request: bool) -> (RdpServer, UnboundedSender