From e5a5f9a0107c6a655e44db68480598fc77c47c57 Mon Sep 17 00:00:00 2001 From: AKolenda Date: Fri, 25 Sep 2026 23:31:41 -0600 Subject: [PATCH 1/4] fix(dvc,session)!: serve channels and graphics that Windows moves onto a tunnel Once Soft-Sync has moved dynamic channels onto the reliable UDP tunnel, Windows keeps using the tunnel for more than channel data, and two gaps kept the graphics pipeline from working there: - dvc: after Soft-Sync, Windows opens new channels (AUDIO_PLAYBACK_DVC, RDS::Input) with Create Request PDUs sent on the tunnel, and closes them there too. `process_tunnel` accepted only data PDUs, so the first Create Request failed the session. Create and Close are now handled on a tunnel once Soft-Sync has completed. A channel created on a tunnel is bound to it, and the client sends every response back on the tunnel the request came from, including the NO_LISTENER response for a declined channel, which the Soft-Sync routing table cannot place. - session: `process_dvc_tunnel` never drained the EGFX compositor, so frames decoded from tunnel data never reached the framebuffer and the window stayed black. It now takes the image, follows a pending ResetGraphics, composites completed frames and records their damage the same way `process` does for TCP-carried DVC data, and returns the graphics updates next to the message batch. BREAKING CHANGE: `ActiveStage::process_dvc_tunnel` takes the decoded image and returns the graphics updates along with the DVC message batch. --- crates/ironrdp-client/src/rdp.rs | 40 ++++- crates/ironrdp-dvc/src/client.rs | 150 ++++++++++++------ crates/ironrdp-session/src/active_stage.rs | 84 ++++++---- .../tests/dvc/client.rs | 111 ++++++++++++- 4 files changed, 297 insertions(+), 88 deletions(-) diff --git a/crates/ironrdp-client/src/rdp.rs b/crates/ironrdp-client/src/rdp.rs index a67d49c9af..d184f13b11 100644 --- a/crates/ironrdp-client/src/rdp.rs +++ b/crates/ironrdp-client/src/rdp.rs @@ -2868,6 +2868,10 @@ enum RdpControlFlow { struct ActiveSessionIteration { outputs: Vec, dvc_batch: Option, + /// The tunnel the DVC batch answers a request from. MS-RDPEDYC responses go back on the + /// transport their request arrived on, which the Soft-Sync routing table cannot tell for a + /// Create Request received on the tunnel and declined: that channel is never bound. + reply_tunnel: Option, } impl ActiveSessionIteration { @@ -2875,6 +2879,7 @@ impl ActiveSessionIteration { Self { outputs, dvc_batch: None, + reply_tunnel: None, } } @@ -2882,6 +2887,7 @@ impl ActiveSessionIteration { Self { outputs: Vec::new(), dvc_batch: Some(dvc_batch), + reply_tunnel: None, } } @@ -2889,6 +2895,18 @@ impl ActiveSessionIteration { Self { outputs, dvc_batch: Some(dvc_batch), + reply_tunnel: None, + } + } + + fn tunnel( + tunnel_type: SoftSyncTunnelType, + (dvc_batch, outputs): (DvcMessageBatch, Vec), + ) -> Self { + Self { + outputs, + dvc_batch: Some(dvc_batch), + reply_tunnel: Some(tunnel_type), } } } @@ -3122,8 +3140,9 @@ async fn active_session( }; let buffered_udp_iteration = if initial_outputs.is_none() && active_stage.reliable_udp_dvc_tunnel_in_use() { match pending_udp_payload.take() { - Some(payload) => Some(ActiveSessionIteration::dvc( - active_stage.process_dvc_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &payload)?, + Some(payload) => Some(ActiveSessionIteration::tunnel( + SoftSyncTunnelType::RELIABLE_UDP, + active_stage.process_dvc_tunnel(&mut image, SoftSyncTunnelType::RELIABLE_UDP, &payload)?, )), None => None, } @@ -3263,9 +3282,14 @@ async fn active_session( } Some(payload) => { if active_stage.reliable_udp_dvc_tunnel_in_use() { - let batch = - active_stage.process_dvc_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &payload)?; - ActiveSessionIteration::dvc(batch) + ActiveSessionIteration::tunnel( + SoftSyncTunnelType::RELIABLE_UDP, + active_stage.process_dvc_tunnel( + &mut image, + SoftSyncTunnelType::RELIABLE_UDP, + &payload, + )?, + ) } else { // The server can send on UDP immediately after its Soft-Sync request, // before the independently ordered request arrives over TCP. Stop @@ -3685,11 +3709,11 @@ async fn active_session( let channel_id = batch.channel_id(); let messages = batch.into_messages(); #[cfg(feature = "udp")] - let route_over_udp = - active_stage.dvc_tunnel_for_channel(channel_id) == Some(SoftSyncTunnelType::RELIABLE_UDP); + let route_over_udp = iteration.reply_tunnel == Some(SoftSyncTunnelType::RELIABLE_UDP) + || active_stage.dvc_tunnel_for_channel(channel_id) == Some(SoftSyncTunnelType::RELIABLE_UDP); #[cfg(not(feature = "udp"))] let route_over_udp = { - let _ = channel_id; + let _ = (channel_id, iteration.reply_tunnel); false }; if route_over_udp { diff --git a/crates/ironrdp-dvc/src/client.rs b/crates/ironrdp-dvc/src/client.rs index 9ae1cd35ce..05c41d6ff5 100644 --- a/crates/ironrdp-dvc/src/client.rs +++ b/crates/ironrdp-dvc/src/client.rs @@ -350,24 +350,107 @@ impl DrdynvcClient { /// data, so data arriving on the wrong tunnel is rejected (MS-RDPEDYC 3.1.5.4.3, 3.2.5.3.2). /// The returned batch carries the channel ID so response messages can be routed back onto /// the same tunnel. + /// + /// Once Soft-Sync has completed, the server may also open and close dynamic channels + /// directly on a tunnel: Windows creates `AUDIO_PLAYBACK_DVC` and `RDS::Input` on the + /// reliable UDP tunnel. A channel created on a tunnel is bound to that tunnel for the rest + /// of its life, and every response in the returned batch belongs on the tunnel the request + /// arrived on, including the response to a Create Request this client declines. pub fn process_tunnel(&mut self, tunnel_type: SoftSyncTunnelType, payload: &[u8]) -> PduResult { - let pdu = decode_dvc_message(payload).map_err(|e| decode_err!(e))?; - let DrdynvcServerPdu::Data(data) = pdu else { - return Err(pdu_other_err!("only DVC data is permitted on a multitransport tunnel")); - }; - let channel_id = data.channel_id(); - let selected_tunnel = self - .tunnel_channels - .get(&channel_id) - .copied() - .ok_or_else(|| pdu_other_err!("received tunneled data for a channel not selected by Soft-Sync"))?; - if tunnel_type != selected_tunnel { + if !self.soft_sync_complete { return Err(pdu_other_err!( - "received tunneled data on a tunnel not selected for the dynamic channel" + "received tunneled DVC traffic before Soft-Sync completed" )); } - let messages = self.process_data(data)?; - Ok(DvcMessageBatch::new(channel_id, messages)) + let pdu = decode_dvc_message(payload).map_err(|e| decode_err!(e))?; + match pdu { + DrdynvcServerPdu::Data(data) => { + let channel_id = data.channel_id(); + let selected_tunnel = + self.tunnel_channels.get(&channel_id).copied().ok_or_else(|| { + pdu_other_err!("received tunneled data for a channel not selected by Soft-Sync") + })?; + if tunnel_type != selected_tunnel { + return Err(pdu_other_err!( + "received tunneled data on a tunnel not selected for the dynamic channel" + )); + } + let messages = self.process_data(data)?; + Ok(DvcMessageBatch::new(channel_id, messages)) + } + DrdynvcServerPdu::Create(create_request) => { + debug!( + ?tunnel_type, + "Got DVC Create Request PDU on a multitransport tunnel: {create_request:?}" + ); + let channel_id = create_request.channel_id(); + let (created, messages) = self.process_create(create_request)?; + if created { + self.tunnel_channels.insert(channel_id, tunnel_type); + } + Ok(DvcMessageBatch::new(channel_id, messages)) + } + DrdynvcServerPdu::Close(close) => { + debug!(?tunnel_type, "Got DVC Close PDU on a multitransport tunnel: {close:?}"); + let channel_id = close.channel_id(); + let messages = self.process_close(channel_id); + Ok(DvcMessageBatch::new(channel_id, messages)) + } + DrdynvcServerPdu::Capabilities(_) | DrdynvcServerPdu::SoftSyncRequest(_) => Err(pdu_other_err!( + "only DVC data, create and close PDUs are permitted on a multitransport tunnel" + )), + } + } + + /// Opens the requested dynamic channel and builds its Create Response, followed by any + /// start messages. Also returns whether the channel was opened. + fn process_create(&mut self, create_request: crate::pdu::CreateRequestPdu) -> PduResult<(bool, Vec)> { + let channel_id = create_request.channel_id(); + let channel_name = create_request.into_channel_name(); + let mut responses = Vec::new(); + + let (creation_status, start_messages) = + if let Some(dvc) = self.dynamic_channels.try_create_channel(&channel_name, channel_id) { + match dvc.start(channel_id) { + Ok(messages) => (CreationStatus::OK, messages), + Err(e) => { + debug!( + ?channel_id, error = %e, + "DVC start failed; removing channel and reporting NO_LISTENER" + ); + self.dynamic_channels.remove_by_channel_id(channel_id); + (CreationStatus::NO_LISTENER, Vec::new()) + } + } + } else { + (CreationStatus::NO_LISTENER, Vec::new()) + }; + + let create_response = DrdynvcClientPdu::Create(CreateResponsePdu::new(channel_id, creation_status)); + debug!("Send DVC Create Response PDU: {create_response:?}"); + responses.push(SvcMessage::from(create_response)); + + // If this DVC has start messages, send them. + if !start_messages.is_empty() { + responses.extend( + encode_dvc_messages(channel_id, start_messages, ChannelFlags::empty()).map_err(|e| encode_err!(e))?, + ); + } + + Ok((creation_status == CreationStatus::OK, responses)) + } + + /// Closes a dynamic channel at the server's request. The Close response is only sent for a + /// channel that was open. + fn process_close(&mut self, channel_id: DynamicChannelId) -> Vec { + self.tunnel_channels.remove(&channel_id); + if self.dynamic_channels.remove_by_channel_id(channel_id).is_some() { + let close_response = DrdynvcClientPdu::Close(ClosePdu::new(channel_id)); + debug!("Send DVC Close Response PDU: {close_response:?}"); + alloc::vec![SvcMessage::from(close_response)] + } else { + Vec::new() + } } fn process_data(&mut self, data: DrdynvcDataPdu) -> PduResult> { @@ -452,8 +535,6 @@ impl SvcProcessor for DrdynvcClient { } DrdynvcServerPdu::Create(create_request) => { debug!("Got DVC Create Request PDU: {create_request:?}"); - let channel_id = create_request.channel_id(); - let channel_name = create_request.into_channel_name(); if !self.cap_handshake_done { debug!( @@ -463,43 +544,12 @@ impl SvcProcessor for DrdynvcClient { responses.push(self.create_capabilities_response(CapsVersion::V2)); } - let (creation_status, start_messages) = - if let Some(dvc) = self.dynamic_channels.try_create_channel(&channel_name, channel_id) { - match dvc.start(channel_id) { - Ok(messages) => (CreationStatus::OK, messages), - Err(e) => { - debug!( - ?channel_id, error = %e, - "DVC start failed; removing channel and reporting NO_LISTENER" - ); - self.dynamic_channels.remove_by_channel_id(channel_id); - (CreationStatus::NO_LISTENER, Vec::new()) - } - } - } else { - (CreationStatus::NO_LISTENER, Vec::new()) - }; - - let create_response = DrdynvcClientPdu::Create(CreateResponsePdu::new(channel_id, creation_status)); - debug!("Send DVC Create Response PDU: {create_response:?}"); - responses.push(SvcMessage::from(create_response)); - - // If this DVC has start messages, send them. - if !start_messages.is_empty() { - responses.extend( - encode_dvc_messages(channel_id, start_messages, ChannelFlags::empty()) - .map_err(|e| encode_err!(e))?, - ); - } + let (_, messages) = self.process_create(create_request)?; + responses.extend(messages); } DrdynvcServerPdu::Close(close) => { debug!("Got DVC Close PDU: {close:?}"); - let channel_id = close.channel_id(); - if self.dynamic_channels.remove_by_channel_id(channel_id).is_some() { - let close_response = DrdynvcClientPdu::Close(ClosePdu::new(channel_id)); - debug!("Send DVC Close Response PDU: {close_response:?}"); - responses.push(SvcMessage::from(close_response)); - } + responses.extend(self.process_close(close.channel_id())); } DrdynvcServerPdu::Data(data) => { if self.tunnel_channels.contains_key(&data.channel_id()) { diff --git a/crates/ironrdp-session/src/active_stage.rs b/crates/ironrdp-session/src/active_stage.rs index f53e0aedac..06275ef8f5 100644 --- a/crates/ironrdp-session/src/active_stage.rs +++ b/crates/ironrdp-session/src/active_stage.rs @@ -230,26 +230,11 @@ impl ActiveStage { } } - // Drain the client-side EGFX compositor: composite each completed-frame - // output region into the image and surface it as a graphics update. EGFX - // data only ever arrives over a DVC, which is X224-carried, so this stays - // out of the Action::FastPath arm rather than running on every fast-path - // frame (the highest-frequency path in a session). - let (output_reset, graphics_updates) = self - .get_dvc_mut::() - .map(|mut gfx| { - let gfx = gfx.processor_mut(); - (gfx.take_output_reset(), gfx.drain_output()) - }) - .unwrap_or_default(); - if let Some((width, height)) = output_reset { - image.reset_preserving_pointer(width, height)?; - self.graphics_output_needs_full_refresh = true; - } - let (region, damage_regions) = - composite_graphics_updates(image, graphics_updates.into_iter().map(|u| (u.region, u.data)))?; - self.damage_regions.extend(damage_regions); - if let Some(region) = region { + // EGFX data only ever arrives over a DVC, which is X224-carried (or carried by a + // Soft-Sync tunnel, see `process_dvc_tunnel`), so the compositor is drained here + // rather than in the Action::FastPath arm, which runs on every fast-path frame + // (the highest-frequency path in a session). + if let Some(region) = self.drain_graphics_pipeline(image)? { stage_outputs.push(ActiveStageOutput::GraphicsUpdate(region)); } @@ -286,6 +271,36 @@ impl ActiveStage { } } + self.widen_to_full_refresh(image, &mut stage_outputs); + + Ok(stage_outputs) + } + + /// Drains the client-side EGFX compositor: follows a pending `ResetGraphics`, composites + /// each completed-frame output region into `image` and records its damage. + /// + /// Returns the union of the changed regions, or `None` when nothing was pending. + fn drain_graphics_pipeline(&mut self, image: &mut DecodedImage) -> SessionResult> { + let (output_reset, graphics_updates) = self + .get_dvc_mut::() + .map(|mut gfx| { + let gfx = gfx.processor_mut(); + (gfx.take_output_reset(), gfx.drain_output()) + }) + .unwrap_or_default(); + if let Some((width, height)) = output_reset { + image.reset_preserving_pointer(width, height)?; + self.graphics_output_needs_full_refresh = true; + } + let (region, damage_regions) = + composite_graphics_updates(image, graphics_updates.into_iter().map(|u| (u.region, u.data)))?; + self.damage_regions.extend(damage_regions); + Ok(region) + } + + /// After a graphics output reset, widens the first graphics update to the whole image so + /// the caller repaints everything the reset cleared. + fn widen_to_full_refresh(&mut self, image: &DecodedImage, stage_outputs: &mut [ActiveStageOutput]) { if self.graphics_output_needs_full_refresh && let Some(ActiveStageOutput::GraphicsUpdate(region)) = stage_outputs .iter_mut() @@ -301,8 +316,6 @@ impl ActiveStage { self.damage_regions.push(region.clone()); self.graphics_output_needs_full_refresh = false; } - - Ok(stage_outputs) } /// Replaces the fast-path processor wholesale. @@ -560,15 +573,28 @@ impl ActiveStage { /// /// Response messages remain unframed so the caller can encode them with /// [`SvcMessage::encode_unframed_pdu`] and send them through the selected tunnel. + /// + /// The graphics pipeline is one of the channels Windows moves onto the tunnel, so completed + /// EGFX frames are composited into `image` here exactly as [`Self::process`] does for DVC + /// data carried over TCP. The resulting graphics updates are returned next to the batch. pub fn process_dvc_tunnel( &mut self, + image: &mut DecodedImage, tunnel_type: SoftSyncTunnelType, payload: &[u8], - ) -> SessionResult { - self.get_svc_processor_mut::() + ) -> SessionResult<(DvcMessageBatch, Vec)> { + self.damage_regions.clear(); + let batch = self + .get_svc_processor_mut::() .ok_or_else(|| SessionError::general("DRDYNVC static channel is not available"))? .process_tunnel(tunnel_type, payload) - .map_err(SessionError::pdu) + .map_err(SessionError::pdu)?; + let mut stage_outputs = Vec::new(); + if let Some(region) = self.drain_graphics_pipeline(image)? { + stage_outputs.push(ActiveStageOutput::GraphicsUpdate(region)); + } + self.widen_to_full_refresh(image, &mut stage_outputs); + Ok((batch, stage_outputs)) } /// Prepares a resize request for routing over TCP or a Soft-Sync tunnel. @@ -1473,16 +1499,18 @@ mod tests { rdpei_ready, )))) .unwrap(); + let mut image = DecodedImage::new(PixelFormat::RgbA32, 4, 4); assert!( stage - .process_dvc_tunnel(SoftSyncTunnelType::LOSSY_UDP, &tunnel_data) + .process_dvc_tunnel(&mut image, SoftSyncTunnelType::LOSSY_UDP, &tunnel_data) .is_err() ); - let response = stage - .process_dvc_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &tunnel_data) + let (response, outputs) = stage + .process_dvc_tunnel(&mut image, SoftSyncTunnelType::RELIABLE_UDP, &tunnel_data) .unwrap(); assert_prepared_batch(&response, 2); + assert!(outputs.is_empty()); } fn active_stage_with_ready_dvcs() -> ActiveStage { diff --git a/crates/ironrdp-testsuite-core/tests/dvc/client.rs b/crates/ironrdp-testsuite-core/tests/dvc/client.rs index b4df109189..3a67bdeded 100644 --- a/crates/ironrdp-testsuite-core/tests/dvc/client.rs +++ b/crates/ironrdp-testsuite-core/tests/dvc/client.rs @@ -1,8 +1,8 @@ use ironrdp_core::{Decode as _, ReadCursor, encode_vec, impl_as_any}; use ironrdp_dvc::ironrdp_pdu::{PduResult, pdu_other_err}; use ironrdp_dvc::pdu::{ - DataPdu, DrdynvcClientPdu, DrdynvcDataPdu, DrdynvcServerPdu, SoftSyncChannelList, SoftSyncRequestPdu, - SoftSyncTunnelType, + ClosePdu, CreateRequestPdu, CreationStatus, DataPdu, DrdynvcClientPdu, DrdynvcDataPdu, DrdynvcServerPdu, + SoftSyncChannelList, SoftSyncRequestPdu, SoftSyncTunnelType, }; use ironrdp_dvc::{DrdynvcClient, DvcClientProcessor, DvcMessage, DvcMessageBatch, DvcProcessor}; use ironrdp_svc::SvcMessage; @@ -54,6 +54,31 @@ impl DvcProcessor for FailingDvc { impl DvcClientProcessor for FailingDvc {} +struct TunnelCreatedDvc; + +impl_as_any!(TunnelCreatedDvc); + +impl DvcProcessor for TunnelCreatedDvc { + fn channel_name(&self) -> &str { + "tunnel-created" + } + + fn start(&mut self, _channel_id: u32) -> PduResult> { + Ok(Vec::new()) + } + + fn process(&mut self, _channel_id: u32, _payload: &[u8]) -> PduResult> { + Ok(Vec::new()) + } +} + +impl DvcClientProcessor for TunnelCreatedDvc {} + +fn decode_client_pdu(message: &SvcMessage) -> DrdynvcClientPdu { + let encoded = message.encode_unframed_pdu().expect("DVC response should encode"); + DrdynvcClientPdu::decode(&mut ReadCursor::new(&encoded)).expect("DVC response should decode") +} + #[test] fn established_dynamic_channel_routes_recorded_data_without_negotiation() { let mut client = DrdynvcClient::new(); @@ -178,3 +203,85 @@ fn message_batch_rejects_a_mismatched_channel_id() { assert!(DvcMessageBatch::try_new(8, vec![message]).is_err()); } + +#[test] +fn channels_created_on_a_tunnel_are_bound_to_it() { + let mut client = DrdynvcClient::new().with_dynamic_channel(TunnelCreatedDvc); + client + .attach_established_dynamic_channel(7, RecordedDvc::default()) + .expect("recorded channel should attach"); + client.enable_soft_sync_tunnel(SoftSyncTunnelType::RELIABLE_UDP); + + let create = encode_vec(&DrdynvcServerPdu::Create(CreateRequestPdu::new( + 16, + "tunnel-created".to_owned(), + ))) + .expect("Create Request should encode"); + // Nothing may arrive on a tunnel before Soft-Sync completes. + assert!( + client + .process_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &create) + .is_err() + ); + + let soft_sync = encode_vec(&DrdynvcServerPdu::SoftSyncRequest(SoftSyncRequestPdu::new(vec![ + SoftSyncChannelList::new(SoftSyncTunnelType::RELIABLE_UDP, vec![7]), + ]))) + .expect("Soft-Sync request should encode"); + client + .process(&soft_sync) + .expect("Soft-Sync request should be accepted"); + + // Windows opens channels such as AUDIO_PLAYBACK_DVC directly on the tunnel after Soft-Sync. + let batch = client + .process_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &create) + .expect("Create Request on the tunnel should be processed"); + assert_eq!(batch.channel_id(), 16); + let [response] = batch.messages() else { + panic!("expected exactly one Create Response"); + }; + let DrdynvcClientPdu::Create(response) = decode_client_pdu(response) else { + panic!("expected a Create Response"); + }; + assert_eq!(response.creation_status(), CreationStatus::OK); + assert_eq!(client.tunnel_for_channel(16), Some(SoftSyncTunnelType::RELIABLE_UDP)); + + // Data for the new channel flows on the tunnel; the same data over TCP is rejected. + let data = encode_vec(&DrdynvcServerPdu::Data(DrdynvcDataPdu::Data(DataPdu::new( + 16, + Vec::new(), + )))) + .expect("DVC data should encode"); + assert!(client.process_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &data).is_ok()); + assert!(client.process(&data).is_err()); + + // A channel without a listener is declined on the tunnel and not bound to it. + let unknown = encode_vec(&DrdynvcServerPdu::Create(CreateRequestPdu::new( + 17, + "unknown".to_owned(), + ))) + .expect("Create Request should encode"); + let batch = client + .process_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &unknown) + .expect("declined Create Request should still be answered"); + assert_eq!(batch.channel_id(), 17); + let [response] = batch.messages() else { + panic!("expected exactly one Create Response"); + }; + let DrdynvcClientPdu::Create(response) = decode_client_pdu(response) else { + panic!("expected a Create Response"); + }; + assert_eq!(response.creation_status(), CreationStatus::NO_LISTENER); + assert_eq!(client.tunnel_for_channel(17), None); + + // Closing the channel on the tunnel answers the Close and unbinds it. + let close = encode_vec(&DrdynvcServerPdu::Close(ClosePdu::new(16))).expect("Close should encode"); + let batch = client + .process_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &close) + .expect("Close on the tunnel should be processed"); + let [response] = batch.messages() else { + panic!("expected exactly one Close response"); + }; + assert!(matches!(decode_client_pdu(response), DrdynvcClientPdu::Close(_))); + assert_eq!(client.tunnel_for_channel(16), None); +} From 470dbd01bd1415b61289b547271748f03bd970a1 Mon Sep 17 00:00:00 2001 From: AKolenda Date: Sat, 26 Sep 2026 12:40:24 -0600 Subject: [PATCH 2/4] fix(dvc): keep a switched tunnel in use after its channels close The reliable UDP tunnel counted as in use only while a channel was bound to it. Once the server closed the last bound channel, the client stopped processing tunnel payloads and buffered them, including a later Create Request that would bind a new channel, so the tunnel stalled for good. Record the tunnels the Soft-Sync response switched to and treat those as in use for the rest of the session. --- crates/ironrdp-dvc/src/client.rs | 25 ++++++++++---- crates/ironrdp-session/src/active_stage.rs | 16 +++++++-- .../tests/dvc/client.rs | 34 +++++++++++++++++++ 3 files changed, 65 insertions(+), 10 deletions(-) diff --git a/crates/ironrdp-dvc/src/client.rs b/crates/ironrdp-dvc/src/client.rs index 05c41d6ff5..f3cb32e642 100644 --- a/crates/ironrdp-dvc/src/client.rs +++ b/crates/ironrdp-dvc/src/client.rs @@ -130,6 +130,7 @@ pub struct DrdynvcClient { cap_handshake_done: bool, available_tunnels: BTreeSet, tunnel_channels: BTreeMap, + switched_tunnels: BTreeSet, soft_sync_complete: bool, } @@ -157,6 +158,7 @@ impl DrdynvcClient { cap_handshake_done: false, available_tunnels: BTreeSet::new(), tunnel_channels: BTreeMap::new(), + switched_tunnels: BTreeSet::new(), soft_sync_complete: false, } } @@ -344,6 +346,14 @@ impl DrdynvcClient { self.tunnel_channels.values().any(|selected| *selected == tunnel_type) } + /// Returns whether the Soft-Sync response switched DVC traffic to `tunnel_type`. + /// + /// The tunnel stays in use after every channel bound to it has closed, because the server + /// can open new channels on it at any time. + pub fn switched_to_tunnel(&self, tunnel_type: SoftSyncTunnelType) -> bool { + self.switched_tunnels.contains(&tunnel_type) + } + /// Processes raw DRDYNVC data received through `tunnel_type`. /// /// The channel's Soft-Sync-selected route is validated before the dynamic channel sees the @@ -351,15 +361,15 @@ impl DrdynvcClient { /// The returned batch carries the channel ID so response messages can be routed back onto /// the same tunnel. /// - /// Once Soft-Sync has completed, the server may also open and close dynamic channels - /// directly on a tunnel: Windows creates `AUDIO_PLAYBACK_DVC` and `RDS::Input` on the - /// reliable UDP tunnel. A channel created on a tunnel is bound to that tunnel for the rest - /// of its life, and every response in the returned batch belongs on the tunnel the request - /// arrived on, including the response to a Create Request this client declines. + /// Once the Soft-Sync response has switched to a tunnel, the server may also open and close + /// dynamic channels directly on it: Windows creates `AUDIO_PLAYBACK_DVC` and `RDS::Input` + /// on the reliable UDP tunnel. A channel created on a tunnel is bound to that tunnel for the + /// rest of its life, and every response in the returned batch belongs on the tunnel the + /// request arrived on, including the response to a Create Request this client declines. pub fn process_tunnel(&mut self, tunnel_type: SoftSyncTunnelType, payload: &[u8]) -> PduResult { - if !self.soft_sync_complete { + if !self.switched_tunnels.contains(&tunnel_type) { return Err(pdu_other_err!( - "received tunneled DVC traffic before Soft-Sync completed" + "received tunneled DVC traffic on a tunnel Soft-Sync did not switch to" )); } let pdu = decode_dvc_message(payload).map_err(|e| decode_err!(e))?; @@ -498,6 +508,7 @@ impl DrdynvcClient { tunnels_to_switch.push(list.tunnel_type()); } + self.switched_tunnels = tunnels_to_switch.iter().copied().collect(); let response = SvcMessage::from(DrdynvcClientPdu::SoftSyncResponse(SoftSyncResponsePdu::new( tunnels_to_switch, ))); diff --git a/crates/ironrdp-session/src/active_stage.rs b/crates/ironrdp-session/src/active_stage.rs index 06275ef8f5..050111d751 100644 --- a/crates/ironrdp-session/src/active_stage.rs +++ b/crates/ironrdp-session/src/active_stage.rs @@ -555,11 +555,11 @@ impl ActiveStage { Ok(()) } - /// Returns whether Soft-Sync moved any DVC to the reliable UDP tunnel. + /// Returns whether the Soft-Sync response switched DVC traffic to the reliable UDP tunnel. pub fn reliable_udp_dvc_tunnel_in_use(&self) -> bool { self.x224_processor .get_svc_processor::() - .is_some_and(|drdynvc| drdynvc.has_channels_on_tunnel(SoftSyncTunnelType::RELIABLE_UDP)) + .is_some_and(|drdynvc| drdynvc.switched_to_tunnel(SoftSyncTunnelType::RELIABLE_UDP)) } /// Returns the Soft-Sync tunnel selected for client messages on `channel_id`. @@ -1069,7 +1069,7 @@ mod tests { use ironrdp_core::{Decode as _, encode_vec}; use ironrdp_displaycontrol::pdu::{DisplayControlCapabilities, DisplayControlPdu}; use ironrdp_dvc::pdu::{ - CreateRequestPdu, DataPdu, DrdynvcDataPdu, DrdynvcServerPdu, SoftSyncChannelList, SoftSyncRequestPdu, + ClosePdu, CreateRequestPdu, DataPdu, DrdynvcDataPdu, DrdynvcServerPdu, SoftSyncChannelList, SoftSyncRequestPdu, }; use ironrdp_graphics::image_processing::PixelFormat; use ironrdp_pdu::gcc::MonitorFlags; @@ -1511,6 +1511,16 @@ mod tests { .unwrap(); assert_prepared_batch(&response, 2); assert!(outputs.is_empty()); + + // The tunnel stays in use after the server closes every channel routed to it. + for channel_id in [1, 2] { + process_drdynvc_pdu( + stage.get_svc_processor_mut::().unwrap(), + DrdynvcServerPdu::Close(ClosePdu::new(channel_id)), + ); + } + assert_eq!(stage.dvc_tunnel_for_channel(2), None); + assert!(stage.reliable_udp_dvc_tunnel_in_use()); } fn active_stage_with_ready_dvcs() -> ActiveStage { diff --git a/crates/ironrdp-testsuite-core/tests/dvc/client.rs b/crates/ironrdp-testsuite-core/tests/dvc/client.rs index 3a67bdeded..4b44880b93 100644 --- a/crates/ironrdp-testsuite-core/tests/dvc/client.rs +++ b/crates/ironrdp-testsuite-core/tests/dvc/client.rs @@ -285,3 +285,37 @@ fn channels_created_on_a_tunnel_are_bound_to_it() { assert!(matches!(decode_client_pdu(response), DrdynvcClientPdu::Close(_))); assert_eq!(client.tunnel_for_channel(16), None); } + +#[test] +fn tunnel_stays_in_use_after_its_channels_close() { + let mut client = DrdynvcClient::new().with_dynamic_channel(TunnelCreatedDvc); + client + .attach_established_dynamic_channel(7, RecordedDvc::default()) + .expect("recorded channel should attach"); + client.enable_soft_sync_tunnel(SoftSyncTunnelType::RELIABLE_UDP); + + let soft_sync = encode_vec(&DrdynvcServerPdu::SoftSyncRequest(SoftSyncRequestPdu::new(vec![ + SoftSyncChannelList::new(SoftSyncTunnelType::RELIABLE_UDP, vec![7]), + ]))) + .expect("Soft-Sync request should encode"); + client + .process(&soft_sync) + .expect("Soft-Sync request should be accepted"); + assert!(client.switched_to_tunnel(SoftSyncTunnelType::RELIABLE_UDP)); + + let close = encode_vec(&DrdynvcServerPdu::Close(ClosePdu::new(7))).expect("Close should encode"); + client.process(&close).expect("Close should be processed"); + assert!(!client.has_channels_on_tunnel(SoftSyncTunnelType::RELIABLE_UDP)); + assert!(client.switched_to_tunnel(SoftSyncTunnelType::RELIABLE_UDP)); + + // The server can still open a channel on the tunnel after the last one closed. + let create = encode_vec(&DrdynvcServerPdu::Create(CreateRequestPdu::new( + 16, + "tunnel-created".to_owned(), + ))) + .expect("Create Request should encode"); + client + .process_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &create) + .expect("Create Request on the tunnel should be processed"); + assert_eq!(client.tunnel_for_channel(16), Some(SoftSyncTunnelType::RELIABLE_UDP)); +} From 4fa8346123f0f9b7bc52d8ad2ea89f0b5bcb2011 Mon Sep 17 00:00:00 2001 From: AKolenda Date: Sun, 27 Sep 2026 23:14:46 -0600 Subject: [PATCH 3/4] refactor(dvc): close server-closed channels through close_channel The Close handler repeated `close_channel`. It now calls it, so the TCP and tunnel close paths share one implementation. `close_channel` drops a channel's tunnel binding before checking whether the channel is open, as the handler did. --- crates/ironrdp-dvc/src/client.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/crates/ironrdp-dvc/src/client.rs b/crates/ironrdp-dvc/src/client.rs index f3cb32e642..70217e529a 100644 --- a/crates/ironrdp-dvc/src/client.rs +++ b/crates/ironrdp-dvc/src/client.rs @@ -309,8 +309,8 @@ impl DrdynvcClient { } pub fn close_channel(&mut self, channel_id: u32) -> Option { - self.dynamic_channels.remove_by_channel_id(channel_id)?; self.tunnel_channels.remove(&channel_id); + self.dynamic_channels.remove_by_channel_id(channel_id)?; Some(SvcMessage::from(DrdynvcClientPdu::Close(ClosePdu::new(channel_id)))) } @@ -453,14 +453,11 @@ impl DrdynvcClient { /// Closes a dynamic channel at the server's request. The Close response is only sent for a /// channel that was open. fn process_close(&mut self, channel_id: DynamicChannelId) -> Vec { - self.tunnel_channels.remove(&channel_id); - if self.dynamic_channels.remove_by_channel_id(channel_id).is_some() { - let close_response = DrdynvcClientPdu::Close(ClosePdu::new(channel_id)); - debug!("Send DVC Close Response PDU: {close_response:?}"); - alloc::vec![SvcMessage::from(close_response)] - } else { - Vec::new() - } + let Some(close_response) = self.close_channel(channel_id) else { + return Vec::new(); + }; + debug!(channel_id, "Send DVC Close Response PDU"); + alloc::vec![close_response] } fn process_data(&mut self, data: DrdynvcDataPdu) -> PduResult> { From 0c520e85f541da050bf561d8bdbf46a92db679f9 Mon Sep 17 00:00:00 2001 From: AKolenda Date: Sun, 27 Sep 2026 23:14:46 -0600 Subject: [PATCH 4/4] fix(dvc): refuse a tunnel Create Request before the capabilities exchange A Create Request that arrives on TCP before the capabilities exchange is answered with a Capabilities Response first. Capabilities PDUs are not exchanged on a tunnel, so a Create Request on a tunnel before the exchange was processed without one and left the handshake open. The tunnel now refuses it. --- crates/ironrdp-dvc/src/client.rs | 8 +++ .../tests/dvc/client.rs | 50 ++++++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/crates/ironrdp-dvc/src/client.rs b/crates/ironrdp-dvc/src/client.rs index 70217e529a..27806d8639 100644 --- a/crates/ironrdp-dvc/src/client.rs +++ b/crates/ironrdp-dvc/src/client.rs @@ -393,6 +393,14 @@ impl DrdynvcClient { ?tunnel_type, "Got DVC Create Request PDU on a multitransport tunnel: {create_request:?}" ); + // The TCP path answers a Create Request that arrives before the capabilities + // exchange with a Capabilities Response first. Capabilities PDUs are not exchanged + // on a tunnel, so a tunnel refuses the out-of-order request instead. + if !self.cap_handshake_done { + return Err(pdu_other_err!( + "received a DVC Create Request on a multitransport tunnel before the capabilities exchange" + )); + } let channel_id = create_request.channel_id(); let (created, messages) = self.process_create(create_request)?; if created { diff --git a/crates/ironrdp-testsuite-core/tests/dvc/client.rs b/crates/ironrdp-testsuite-core/tests/dvc/client.rs index 4b44880b93..a7838de40a 100644 --- a/crates/ironrdp-testsuite-core/tests/dvc/client.rs +++ b/crates/ironrdp-testsuite-core/tests/dvc/client.rs @@ -1,8 +1,8 @@ use ironrdp_core::{Decode as _, ReadCursor, encode_vec, impl_as_any}; use ironrdp_dvc::ironrdp_pdu::{PduResult, pdu_other_err}; use ironrdp_dvc::pdu::{ - ClosePdu, CreateRequestPdu, CreationStatus, DataPdu, DrdynvcClientPdu, DrdynvcDataPdu, DrdynvcServerPdu, - SoftSyncChannelList, SoftSyncRequestPdu, SoftSyncTunnelType, + CapabilitiesRequestPdu, CapsVersion, ClosePdu, CreateRequestPdu, CreationStatus, DataPdu, DrdynvcClientPdu, + DrdynvcDataPdu, DrdynvcServerPdu, SoftSyncChannelList, SoftSyncRequestPdu, SoftSyncTunnelType, }; use ironrdp_dvc::{DrdynvcClient, DvcClientProcessor, DvcMessage, DvcMessageBatch, DvcProcessor}; use ironrdp_svc::SvcMessage; @@ -79,6 +79,15 @@ fn decode_client_pdu(message: &SvcMessage) -> DrdynvcClientPdu { DrdynvcClientPdu::decode(&mut ReadCursor::new(&encoded)).expect("DVC response should decode") } +fn exchange_capabilities(client: &mut DrdynvcClient) { + let caps = encode_vec(&DrdynvcServerPdu::Capabilities(CapabilitiesRequestPdu::new( + CapsVersion::V3, + None, + ))) + .expect("Capabilities Request should encode"); + client.process(&caps).expect("Capabilities Request should be processed"); +} + #[test] fn established_dynamic_channel_routes_recorded_data_without_negotiation() { let mut client = DrdynvcClient::new(); @@ -207,6 +216,7 @@ fn message_batch_rejects_a_mismatched_channel_id() { #[test] fn channels_created_on_a_tunnel_are_bound_to_it() { let mut client = DrdynvcClient::new().with_dynamic_channel(TunnelCreatedDvc); + exchange_capabilities(&mut client); client .attach_established_dynamic_channel(7, RecordedDvc::default()) .expect("recorded channel should attach"); @@ -289,6 +299,7 @@ fn channels_created_on_a_tunnel_are_bound_to_it() { #[test] fn tunnel_stays_in_use_after_its_channels_close() { let mut client = DrdynvcClient::new().with_dynamic_channel(TunnelCreatedDvc); + exchange_capabilities(&mut client); client .attach_established_dynamic_channel(7, RecordedDvc::default()) .expect("recorded channel should attach"); @@ -319,3 +330,38 @@ fn tunnel_stays_in_use_after_its_channels_close() { .expect("Create Request on the tunnel should be processed"); assert_eq!(client.tunnel_for_channel(16), Some(SoftSyncTunnelType::RELIABLE_UDP)); } + +#[test] +fn tunnel_refuses_a_create_request_before_the_capabilities_exchange() { + let mut client = DrdynvcClient::new().with_dynamic_channel(TunnelCreatedDvc); + client + .attach_established_dynamic_channel(7, RecordedDvc::default()) + .expect("recorded channel should attach"); + client.enable_soft_sync_tunnel(SoftSyncTunnelType::RELIABLE_UDP); + + let soft_sync = encode_vec(&DrdynvcServerPdu::SoftSyncRequest(SoftSyncRequestPdu::new(vec![ + SoftSyncChannelList::new(SoftSyncTunnelType::RELIABLE_UDP, vec![7]), + ]))) + .expect("Soft-Sync request should encode"); + client + .process(&soft_sync) + .expect("Soft-Sync request should be accepted"); + + let create = encode_vec(&DrdynvcServerPdu::Create(CreateRequestPdu::new( + 16, + "tunnel-created".to_owned(), + ))) + .expect("Create Request should encode"); + assert!( + client + .process_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &create) + .is_err() + ); + assert_eq!(client.tunnel_for_channel(16), None); + + exchange_capabilities(&mut client); + client + .process_tunnel(SoftSyncTunnelType::RELIABLE_UDP, &create) + .expect("Create Request on the tunnel should be processed after the capabilities exchange"); + assert_eq!(client.tunnel_for_channel(16), Some(SoftSyncTunnelType::RELIABLE_UDP)); +}