Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions crates/ironrdp-client/src/rdp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,20 @@ pub enum RdpOutputEvent {
/// A cookie-based reconnect has completed successfully.
AutoReconnected,
Terminated(SessionResult<GracefulDisconnectReason>),
/// The transport carrying the dynamic channels changed.
///
/// Sent once the session is active, and again whenever a reliable RDP-UDP tunnel is
/// established, Soft-Sync moves the dynamic channels onto it, or the session falls back
/// to TCP.
#[cfg(feature = "udp")]
Transport {
/// The dynamic channels, the graphics pipeline among them, travel over the reliable
/// RDP-UDP tunnel.
reliable_udp: bool,
Comment on lines +210 to +212

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[skeptical] reliable_udp doc overclaims that the graphics pipeline moved onto UDP — low 🟡 — The flag is read from ActiveStage::reliable_udp_dvc_tunnel_in_use, which is documented as 'whether Soft-Sync moved any DVC to the reliable UDP tunnel' and is implemented via has_channels_on_tunnel, an any-channel check. With declined channels producing partial moves (handled by PR #2007), some DVCs can stay on TCP, potentially including the graphics pipeline. The doc asserts they all travel over UDP, so an embedder keying diagnostics or status off reliable_udp can report graphics-on-UDP when it is not. Narrow the wording to 'at least one dynamic channel' or expose graphics-channel-specific state.

/// The RDP-UDP version the tunnel's handshake settled on, while a tunnel is open,
/// whether or not Soft-Sync has moved channels onto it yet.
udp_version: Option<ironrdp_rdpeudp::pdu::UdpVersion>,
},
}

/// A tightly packed changed region from the composited desktop framebuffer.
Expand Down Expand Up @@ -3085,6 +3099,8 @@ async fn active_session(
let mut graceful_shutdown_sent = false;
let mut post_logon_redraw_requested = false;
let mut pending_udp_payload: Option<Vec<u8>> = None;
#[cfg(feature = "udp")]
let mut announced_transport = None;
let mut initial_outputs = if *graceful_close_receiver.borrow_and_update() {
graceful_shutdown_sent = true;
Some(active_stage.graceful_shutdown()?)
Expand Down Expand Up @@ -4241,6 +4257,30 @@ async fn active_session(
}
}

#[cfg(feature = "udp")]
{
let transport = (
active_stage.reliable_udp_dvc_tunnel_in_use(),
udp_tunnel
.transport
.as_ref()
.and_then(ironrdp_rdpeudp_tokio::UdpTransport::negotiated_version),
);
if announced_transport != Some(transport) {
announced_transport = Some(transport);
let (reliable_udp, udp_version) = transport;
let event = RdpOutputEvent::Transport {
reliable_udp,
udp_version,
};
if !send_active_output_event(output_event_sender, event, close_receiver).await? {
return Ok(RdpControlFlow::TerminatedGracefully(
GracefulDisconnectReason::UserInitiated,
));
}
}
}

if resize_queue.in_flight.is_none()
&& let Some(pending) = resize_queue.pending.as_ref()
{
Expand Down
3 changes: 3 additions & 0 deletions crates/ironrdp-rdpeudp-tokio/src/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,9 @@ impl Driver {
Event::Connected => {
if !self.connected_signaled {
self.connected_signaled = true;
if let Ok(mut shared) = self.shared.lock() {
shared.negotiated_version = self.conn.negotiated_version();
}
self.connected_notify.notify_one();
}
}
Expand Down
5 changes: 5 additions & 0 deletions crates/ironrdp-rdpeudp-tokio/src/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ pub(crate) struct SharedIo {

/// Set when the RDPEUDP2 connection has been cleanly shut down.
pub(crate) closed: bool,

/// The RDP-UDP version the handshake settled on, recorded by the driver once the
/// connection is established.
pub(crate) negotiated_version: Option<ironrdp_rdpeudp::pdu::UdpVersion>,
}

impl SharedIo {
Expand All @@ -93,6 +97,7 @@ impl SharedIo {
write_room_waker: None,
error: None,
closed: false,
negotiated_version: None,
}
}

Expand Down
6 changes: 6 additions & 0 deletions crates/ironrdp-rdpeudp-tokio/src/transport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,12 @@ impl UdpTransport {
}
}

/// The RDP-UDP version the handshake settled on: version 1 or 2 for MS-RDPEUDP,
/// version 3 for MS-RDPEUDP2.
pub fn negotiated_version(&self) -> Option<ironrdp_rdpeudp::pdu::UdpVersion> {
self.shared.lock().ok().and_then(|shared| shared.negotiated_version)
}

/// Whether the driver task is still running.
pub fn is_alive(&self) -> bool {
!self.driver_handle.is_finished()
Expand Down
9 changes: 9 additions & 0 deletions crates/ironrdp-rdpeudp/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,15 @@ impl RdpeudpConnection {
self.params.as_ref().map(|p| p.mtu)
}

/// The protocol version the handshake settled on, once it is complete: version 1 or 2
/// for MS-RDPEUDP, version 3 for MS-RDPEUDP2.
pub fn negotiated_version(&self) -> Option<UdpVersion> {
self.params.as_ref().map(|params| match params.wire {
WireFormat::V1 { version } => UdpVersion(version),
WireFormat::V2 => UdpVersion::V3,
})
}
Comment on lines +930 to +937

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[skeptical] negotiated_version returns Some before the handshake completes on the server path — low 🟡 — The doc states the value is available 'once it is complete', but accept() populates params with WireFormat::V2 while still in SynReceived, so a server-side connection reports Some(UdpVersion::V3) before the final ACK establishes the session. A caller using Some as an establishment signal is misled. In-tree use is unaffected because the tokio driver records the version only on Event::Connected, and the sibling mtu() accessor shares this early-params timing, but this is new public API text whose stated contract the implementation violates on the public accept path. Fix the doc or gate on the Established state.


/// Diagnostics for the MS-RDPEUDP version 1/2 data path; `None` on MS-RDPEUDP2.
pub fn v1_stats(&self) -> Option<V1Stats> {
let params = self.params.as_ref()?;
Expand Down
3 changes: 3 additions & 0 deletions crates/ironrdp-testsuite-core/tests/rdpeudp/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ fn full_handshake_client_server() {
.expect("handle SYN+ACK");

assert!(client.is_established());
assert_eq!(client.negotiated_version(), Some(UdpVersion::V3));

// Client should emit Connected event
let event = client.poll_event().expect("should have event");
Expand Down Expand Up @@ -1221,11 +1222,13 @@ fn a_client_follows_a_syn_ack_that_settles_on_version_2() {
let mut client = RdpeudpConnection::connect(default_config(100), t).expect("connect");
client.poll_transmit(t).expect("SYN");

assert_eq!(client.negotiated_version(), None);
let mut bytes = version_2_syn_ack();
client
.handle_datagram(&mut bytes, later(t, 50))
.expect("version 2 is a version both endpoints support");
assert!(client.is_established());
assert_eq!(client.negotiated_version(), Some(UdpVersion::V2));

// The final handshake ACK acknowledges the SYN+ACK in MS-RDPEUDP framing.
let ack = client.poll_transmit(later(t, 50)).expect("final ACK");
Expand Down
6 changes: 6 additions & 0 deletions crates/ironrdp-viewer/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -620,6 +620,12 @@ impl RpcApp {
debug!(?control, "RAIL control received");
}
RdpOutputEvent::WindowingOrders(_) => {}
RdpOutputEvent::Transport {
reliable_udp,
udp_version,
} => {
info!(reliable_udp, ?udp_version, "Session transport");
}
Comment on lines +623 to +628

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[skeptical + code-compressor] Transport state logged twice at info for the in-tree viewer — low 🟡 — The client logs info!(reliable_udp, ?udp_version, "Session transport") when it emits the new event (crates/ironrdp-client/src/rdp.rs:4272, added by this PR), and the viewer's new match arm logs the identical fields with the identical message on receipt (crates/ironrdp-viewer/src/app.rs:623-628, also added here). The viewer runs the client in-process, so every transport change produces two consecutive equivalent info lines in one process, in a PR whose purpose is log-based diagnostics. Any other embedder already gets the library-side line plus the event payload. Exactly one of the two logs is needed: either shrink the viewer arm to an empty match like the WindowingOrders arm above it, or drop the client-side info! in favor of embedder-owned presentation. Diagnostics-only impact, hence low severity.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 723ae4f. The client no longer logs the transport itself. Embedders log the event as they do the other session events, so the viewer arm keeps the single info line.

// Only produced when the client is built with `.with_desktop_updates()`, which the
// viewer does not opt into: it always presents full-frame `Image` snapshots instead.
RdpOutputEvent::DesktopUpdate(_) => {}
Expand Down
Loading