fix(session)!: serve channels and graphics that Windows moves onto a tunnel #2008
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,6 +130,7 @@ pub struct DrdynvcClient { | |
| cap_handshake_done: bool, | ||
| available_tunnels: BTreeSet<SoftSyncTunnelType>, | ||
| tunnel_channels: BTreeMap<DynamicChannelId, SoftSyncTunnelType>, | ||
| switched_tunnels: BTreeSet<SoftSyncTunnelType>, | ||
| 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, | ||
| } | ||
| } | ||
|
|
@@ -307,8 +309,8 @@ impl DrdynvcClient { | |
| } | ||
|
|
||
| pub fn close_channel(&mut self, channel_id: u32) -> Option<SvcMessage> { | ||
| self.tunnel_channels.remove(&channel_id); | ||
| self.dynamic_channels.remove_by_channel_id(channel_id)?; | ||
| Some(SvcMessage::from(DrdynvcClientPdu::Close(ClosePdu::new(channel_id)))) | ||
| } | ||
|
|
||
|
|
@@ -344,30 +346,126 @@ 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 | ||
| /// 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 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<DvcMessageBatch> { | ||
| if !self.switched_tunnels.contains(&tunnel_type) { | ||
| return Err(pdu_other_err!( | ||
| "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))?; | ||
| 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:?}" | ||
| ); | ||
| // 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 { | ||
| 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)) | ||
| } | ||
|
Comment on lines
+411
to
+416
Contributor
There was a problem hiding this comment. [skeptical] Closing the last tunnel-bound channel stalls tunnel traffic permanently with no recovery path — low 🟡 — The shared process_close removes the tunnel binding, so after the server closes every channel bound to the reliable UDP tunnel, has_channels_on_tunnel (and reliable_udp_dvc_tunnel_in_use) returns false while the UDP transport stays open. In rdp.rs the udp_payload arm then stores tunnel payloads in pending_udp_payload, which the loop top drains only while the tunnel is in use; the later Create Request that could re-bind a channel is itself such a payload, and a duplicate Soft-Sync request is rejected, so the state can never recover and tunnel traffic is silently buffered instead of failing loudly like the pre-change error path. The same unbinding now also happens for server Closes arriving over TCP. The code chain is verified; whether a Windows host actually closes all tunnel channels and reuses the tunnel within one session is unverified, so severity stays low.
Contributor
Author
There was a problem hiding this comment. Fixed in 963e3cf. The DRDYNVC client now records the tunnels its Soft-Sync response switched to ( |
||
| 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<SvcMessage>)> { | ||
| 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<SvcMessage> { | ||
| 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<Vec<SvcMessage>> { | ||
|
|
@@ -415,6 +513,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, | ||
| ))); | ||
|
|
@@ -452,8 +551,6 @@ impl SvcProcessor for DrdynvcClient { | |
| } | ||
| DrdynvcServerPdu::Create(create_request) => { | ||
| debug!("Got DVC Create Request PDU: {create_request:?}"); | ||
|
|
||
| if !self.cap_handshake_done { | ||
| debug!( | ||
|
|
@@ -463,43 +560,12 @@ impl SvcProcessor for DrdynvcClient { | |
| responses.push(self.create_capabilities_response(CapsVersion::V2)); | ||
| } | ||
|
|
||
| let (_, messages) = self.process_create(create_request)?; | ||
| responses.extend(messages); | ||
| } | ||
| DrdynvcServerPdu::Close(close) => { | ||
| debug!("Got DVC Close PDU: {close:?}"); | ||
| responses.extend(self.process_close(close.channel_id())); | ||
| } | ||
| DrdynvcServerPdu::Data(data) => { | ||
| if self.tunnel_channels.contains_key(&data.channel_id()) { | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[skeptical] Tunnel Create path skips the capabilities-response recovery the TCP path performs — low 🟡 — The TCP Create arm synthesizes a Capabilities Response when a Create Request arrives before the caps handshake (client.rs:548-554), defending against out-of-order server PDUs. process_tunnel's Create arm calls process_create with no such check, so a server that sends the Soft-Sync Request before capabilities and then a Create Request on the tunnel gets only a Create Response; cap_handshake_done stays false and a later TCP Create would emit a late caps response. Handling the precondition in the tunnel path (or asserting it) keeps the two Create paths consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 0c520e8. A Create Request that arrives on a tunnel before the capabilities exchange is now refused. The TCP path recovers by sending a Capabilities Response on DRDYNVC first. Capabilities PDUs are not exchanged on a tunnel, so the tunnel path cannot do the same. Soft-Sync does not depend on the DVC version, so the Soft-Sync request can't rule this case out either. Covered by the new
dvc::client::tunnel_refuses_a_create_request_before_the_capabilities_exchange; the two existing tunnel tests now run the capabilities exchange first.