Repository navigation
Conversation
GitHub Actions
Error
github-actions
Bot
added
kind/protocol
AKolenda
changed the title
|
On the overlap notice: #2009 and #2012 are complementary, not duplicates. #2012 answers the auto-detect requests that arrive on the TCP message channel, in the x224 processor. #2009 answers the ones Windows sends on the RDPEMT tunnel, in the tunnel driver. Each request is answered on the transport it arrived on, and each transport keeps its own measurement window, so the two never answer the same request. |
…#2007) Windows lists every dynamic channel it intends to move in its Soft-Sync request, including the ones the client declined with NO_LISTENER. Against a Windows 11 host the request lists channels 2, 6, 7, 8, 9, 10, 11 and 12 (CoreInput, MouseCursor, Graphics, Video, Geometry, ...), and only channel 7, the graphics pipeline, is open. `process_soft_sync_request` dropped a whole channel list as soon as one ID in it was not open. The tunnel was then never switched, and the channels the client had opened stayed on TCP while the server was already sending them on the tunnel (MS-RDPEDYC 3.2.5.3.1). Unopened channels are now skipped one by one, and the tunnel is switched for the rest. ## Testing - New `dvc::client::soft_sync_skips_channels_the_client_did_not_open` in `ironrdp-testsuite-core`. - Live, against a Windows 11 host over RDP-UDP version 2, with the viewer built from a branch that also carries the tunnel and client PRs of this series: the Soft-Sync request above now switches the tunnel, and the graphics pipeline moves onto it. ## Checks - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warnings` - `cargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra`, plus the lib tests of the crates touched here - `cargo test --workspace --locked` on a branch that merges this PR with the other Windows interop PRs from this series - `typos` on the changed files ## Series These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on `master` and can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order. - #2007 fix(dvc): Soft-Sync tunnel with declined channels - #2008 fix(session)!: channels and graphics on the tunnel - #2009 fix(rdpeudp): auto-detect on the tunnel - #2010 fix(graphics)!: SRL streams from Windows - #2011 fix(egfx): bitmap cache across ResetGraphics - #2012 feat(session): bandwidth measurements during the session - #2013 feat(client): graphics pipeline and RDP-UDP version options - #2014 fix(client): resize reconnects on the graphics pipeline - #2015 feat(client): transport event - #2016 feat(cliprdr): Linux clipboard backend - #2017 feat(rdpdr): printer on Linux and macOS Co-authored-by: AKolenda <testedemail2222@gmail.com>
|
Thanks for this, and for testing it live against Windows 11 over RDP-UDP. Your reading of the sub-header is right: MS-RDPEMT 2.2.1.1.1 says an auto-detect sub-header conforms to the auto-detect structure, so SubHeaderLength and SubHeaderType are that structure's headerLength and headerTypeId. It caught a bug on my server side: #2031 was repeating those two bytes inside SubHeaderData and decoding the client's results from the data alone. That is fixed in 9cea2c6. One design question. This PR answers the tunnel's auto-detect inside the transport's read pump, with its own bandwidth window, while #2012 answers the same measurements on the message channel in the session layer, with a second window built on the caller-supplied timestamps from #1487. #2030 exposes a Tunnel Data PDU's sub-headers to the caller through |
ac811f1 to
642d39b
Compare
GitHub Actions
Error
AKolenda
changed the title
|
Yes, that works and it's cleaner, so I've reworked this PR that way. The client now reads the tunnel with One difference from a single window: #2012's handling now lives in an This PR is now stacked on #2030 and #2012. Its commits apply cleanly on top of #2031, and clippy and the workspace tests pass there, so it should follow your stack without trouble. Thanks for spotting the duplication. |
github-actions
Bot
added
automation-failed
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #2009 answers auto-detect requests arriving in RDPEMT tunnel sub-headers by routing them through a session-side AutoDetectResponder and replying via the transport's new send_message, with per-transport measurement windows and a shared tunnel-failure policy. The change is well-structured, tested, and the tunnel-failure policy correctly preserves the before/after-DVC-migration distinction. Five low-to-medium issues remain: the per-transport bandwidth state deviates from MS-RDPBCGR 3.2.5.14's single byte-count store and timer (medium), a dead pending_udp_payload assignment with a misleading comment, tunnel timestamps measuring dequeue time rather than wire arrival contrary to the new doc contract, measurement rules pinned only by synthetic tests with no live Windows run for this revision, and a __test feature permanently widening the published ironrdp-client surface for workspace tests. None block correctness of the connection; all are quality/spec-conformance concerns.
Reduced coverage: optional reviewer code-compressor was unavailable.
| /// Answers the auto-detect requests the server sends in the sub-headers of one Tunnel Data | ||
| /// PDU ([MS-RDPEMT] 2.2.1.1.1), and counts the `data_len` bytes of higher-layer data the PDU | ||
| /// carries for an open bandwidth measurement. Returns the responses to send back on the | ||
| /// tunnel. | ||
| /// | ||
| /// The tunnel keeps its own measurement, apart from the message channel's, because a | ||
| /// measurement on the tunnel counts only the data that follows the tunnel PDU header | ||
| /// ([MS-RDPBCGR] 3.2.5.14). As on the message channel, received bytes are counted before | ||
| /// handling control messages: a Start resets the count, and a Stop includes the carrying | ||
| /// PDU's data in the result. Sub-header bytes themselves never count on the tunnel. | ||
| /// `received_at` is the time the PDU arrived, from one monotonic clock for the whole tunnel. | ||
| /// | ||
| /// [MS-RDPEMT]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpemt/4f538fd7-3aca-4e7d-a213-13eb5f95c1ad | ||
| /// [MS-RDPBCGR]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpbcgr/16ffa852-8aa7-481c-99a0-36c1a9a198f6 | ||
| pub fn process_tunnel_auto_detect( | ||
| &mut self, | ||
| requests: Vec<AutoDetectRequest>, | ||
| data_len: usize, | ||
| received_at: MonotonicInstant, | ||
| ) -> Vec<AutoDetectResponse> { | ||
| self.tunnel_auto_detect.record_bytes(data_len); | ||
| requests | ||
| .into_iter() | ||
| .filter_map(|request| self.tunnel_auto_detect.respond(request, Some(received_at))) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
[skeptical] Reworked tunnel measurement rules lack live-server validation for this revision — low 🟡 ❓ — An earlier revision of this PR was live-tested against Windows 11 over RDP-UDP, and the maintainer confirmed its sub-header framing (fixing a server-side bug in #2031). This revision, however, re-architected the path after that run: the responder moved into the session, and process_tunnel_auto_detect encodes new behavioral claims - carrier-PDU counting order (a Start resets before its carrier counts, a Stop includes its carrier), exclusion of tunnel header/sub-header bytes, separate TCP/tunnel windows, and answering RTT requests on the tunnel although MS-RDPEMT only specifies bandwidth encapsulation there. All are pinned only by synthetic fixtures, and the PR body states no live Windows interoperability run was performed for this revision. An incorrect rule would not break the connection but would skew the server's bandwidth adaptation, so severity is low; confirmation against a real Windows server is missing.
There was a problem hiding this comment.
No code change for this one. This revision has not been run against a live Windows server, and CI cannot do that. The counting rules follow MS-RDPBCGR 3.2.5.14 (count only the data after the tunnel header; one store and timer shared with the main connection, as of 3d70762) and are pinned by fixture tests. RTT requests are answered on the tunnel because Windows sends them there; MS-RDPEMT does not say either way. A live run against Windows is still needed before relying on the measured values.
github-actions
Bot
added
ai-reviewed/2
The session answered RTT requests on the message channel but ignored Bandwidth Measure Start and Stop, so a server running continuous network auto-detection (MS-RDPBCGR 2.2.14, 3.2.5.14) never got a Bandwidth Measure Results PDU back. Windows runs continuous detection by default. The x224 processor now keeps a bandwidth window: - A continuous Start (0x0014) opens a window, and every server byte received until the Stop (0x0429) is counted. Following 3.2.5.14, only the bytes after a Basic Security Header count on the message channel. On the IO and static channels the MCS user data is counted, and on the fast path the data after the fast-path header, which is what FreeRDP counts too. - A connect-time Start (0x1014) counts Bandwidth Measure Payload PDUs and the 0x002B Stop the way the connector does: payloadLength plus the eight header bytes. - A repeated Start restarts the window, and the Results report the elapsed time floored at 1 ms (the server divides by it) and the byte count. - Lossy (0x0114/0x0629) requests belong to a lossy tunnel and are not answered on this channel. Timing follows the connector: the caller passes the frame's arrival time through the new `ActiveStage::process_with_timestamp` and `x224::Processor::process_with_timestamp`, and the client passes the time its framed reader filled the buffer. Without a timestamp no window opens and a Stop is answered with a zero-byte, 1 ms result instead of a figure the client never measured. `process` keeps working that way.
Move the handling of RTT and bandwidth measurement requests out of the x224 processor into an `AutoDetectResponder`, so the message channel and a multitransport tunnel can answer them with the same code. Each transport keeps its own responder, because a continuous measurement counts the data received on its own transport ([MS-RDPBCGR] 3.2.5.14). No behavior change on the message channel.
Decode the fast-path header for the byte count only while a continuous measurement is running, since `fast_path::Processor::process` decodes it again. Keep the payloadLength plus header rule in one `counted_len` helper, and log a timed window that is dropped because its Stop has no arrival time, as the connector does. Adds a test for fast-path counting.
The client dropped the sub-headers of every Tunnel Data PDU, so the auto-detect requests Windows sends on the UDP tunnel once it is established (MS-RDPEMT 2.2.1.1.1, MS-RDPBCGR 2.2.14) were never answered. The client now reads the tunnel with `recv_message` and hands those requests to the session, which answers them with the same code as the message channel: - The request handling moves out of the x224 processor into an `AutoDetectResponder`. The message channel and the tunnel each have one, because a continuous measurement counts the data of its own transport. - `ActiveStage::process_tunnel_auto_detect` answers the requests of one Tunnel Data PDU and counts its higher-layer data. MS-RDPBCGR 3.2.5.14 counts only the data after the tunnel PDU header, and that data follows the sub-headers, so the data of the PDU carrying a Start is counted and the data of the one carrying a Stop is not. - The client times the tunnel against its own monotonic clock and sends each set of responses back with `send_message`, in a Tunnel Data PDU with no data. A tunnel sub-header is the auto-detect structure itself: its SubHeaderLength and SubHeaderType are the structure's headerLength and headerTypeId. Requests are decoded from the whole sub-header, and responses are encoded the same way.
`ironrdp-session` and `ironrdp-client` set `test = false`, so their inline tests never ran under `cargo test --workspace`. Moves them to the testsuites and exposes the two sub-header helpers as `#[doc(hidden)]`, as the client already does for `connect_preferring_direct`. The separate-window test now opens both windows and feeds data to each.
33080ea to
2664936
Compare
github-actions
Bot
added
size/XL
|
This pull request may overlap with #2012. Both implement session-time auto-detect answering through a shared AutoDetectResponder in ironrdp-session, add process_with_timestamp on ActiveStage and x224::Processor, apply the same byte-count and 1 ms floor rules, and wire the client. This PR additionally answers the same requests on the reliable UDP tunnel sub-headers. This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide. Note LLM-assisted content (no human feedback). |
…aders The read pump now stamps each Tunnel Data PDU when it reads it, and `UdpTransport::last_received_at` reports that time for the message last returned by `recv`/`recv_message`. The reading comes from `ironrdp_async::monotonic_now`, now public, which is the clock behind `Framed::last_read_at`, so tunnel and main-connection arrival times can be compared. `TunnelMessage::auto_detect_requests` and `TunnelMessage::from_auto_detect_responses` convert between tunnel sub-headers and auto-detect PDUs, so the client no longer needs its own copy of this wire mapping.
MS-RDPBCGR 3.2.5.14 defines one Network Characteristics Byte Count store and one timer. A continuous window now counts data from both transports, and a Start on one transport can be closed by a Stop on the other; the result still goes back on the transport the Stop arrived on. `process_tunnel_auto_detect` uses the message channel's responder and takes the transport's read time, from the same clock as the TCP reader. In the client, tunnel arrival times now come from the transport's read pump instead of the dequeue in the session loop, the tunnel auto-detect helpers come from ironrdp-rdpeudp-tokio, and the `__test` feature that exposed the private `udp` module is gone. Also drop a dead assignment to `pending_udp_payload` on the send-failure path.
2664936 to
3d70762
Compare
There was a problem hiding this comment.
PR #2009 answers RTT and bandwidth auto-detect requests on the reliable UDP tunnel with one measurement window shared across TCP and UDP, per MS-RDPBCGR 3.2.5.14, with arrival timestamps plumbed from both transports on a single monotonic clock. The core design is sound: shared responder state, transport-side stamping, count-before-dispatch ordering, and the disable_failed_tunnel policy each check out against the head code and tests. Three low-severity items are published: the unconditional byte counts would include RDP Security Header bytes if Standard RDP Security were ever negotiated (a commented, unenforced premise on public API), a type-mismatched non-lossy Bandwidth Measure Stop drops the open window's count instead of reporting it, and the UDP arrival timestamp reaches the client through a stateful field plus accessor rather than traveling with the message. The skeptical claim of inconsistent per-path framing rules is rejected: the differences follow from the spec's own header-e…
Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.
| // TLS-protected fast-path frames have no RDP Security Header, so the | ||
| // continuous bandwidth count includes the entire frame. | ||
| self.x224_processor.record_bandwidth_bytes(frame.len()); |
There was a problem hiding this comment.
[protocol + skeptical] Bandwidth byte count includes RDP Security Header bytes if Standard RDP Security is ever negotiated — low 🟡 — MS-RDPBCGR 3.2.5.14 requires excluding Security Header bytes from the Network Characteristics Byte Count when one is present, but the counts are unconditional: fast-path frames are counted whole here, whole slow-path SVC/IO frames (including TPKT/X224/MCS) in x224/mod.rs, and the message channel subtracts only the 4-byte Basic Security Header so an encrypted header's 8-byte signature would also count. The TLS-only premise is stated solely in comments on the public process_with_timestamp API, not enforced or feature-gated. No in-tree caller runs Standard RDP Security with security headers today, so practical impact is a systematically inflated byteCount only for such future callers; under TLS or Enhanced RDP Security the behavior conforms.
| let measurement = self | ||
| .bandwidth | ||
| .take() | ||
| .filter(|measurement| measurement.continuous == continuous); |
There was a problem hiding this comment.
[protocol] Type-mismatched Bandwidth Measure Stop discards the open window's count — low 🟡 — take().filter(|m| m.continuous == continuous) removes the measurement from the store and then drops it when the Stop's requestType does not match the open window's, so a 0x0429 (or 0x002B) Stop of the other type answers timeDelta 1 and byteCount 0 even though the store held an accumulated count. MS-RDPBCGR 3.2.5.14 requires a non-lossy Stop to stop the timer and send the store's contents; only the 0x0629 lossy Stop has a verification step that skips the response. Concrete deviation, but it only arises if a server interleaves a connect-time and a continuous measurement, which Windows does not normally do.
| /// A [`TunnelMessage`] with the time the read pump read its Tunnel Data PDU. | ||
| #[derive(Debug)] | ||
| pub(crate) struct ReceivedMessage { | ||
| pub(crate) message: TunnelMessage, | ||
| pub(crate) received_at: MonotonicInstant, | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| impl<T: Into<TunnelMessage>> From<T> for ReceivedMessage { | ||
| fn from(message: T) -> Self { | ||
| Self { | ||
| message: message.into(), | ||
| received_at: ironrdp_async::monotonic_now(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[code-compressor] UDP arrival timestamp travels via a stateful field and accessor instead of alongside the message — low 🟡 — ReceivedMessage, the channel type change across transport.rs/tunnel.rs/framed.rs, the last_received_at field initialized in four constructors, the public accessor, and the client's received_at: transport.last_received_at() all move one MonotonicInstant from the read pump to the sole recv_message caller, which always sees Some so the client's Option holds an unreachable None. A smaller shape returns the time with the message (e.g. Option<(TunnelMessage, MonotonicInstant)>), deleting the struct, field, initializations, accessor, and Option threading with identical behavior. Tradeoff: it would change the public recv_message signature added in recently merged #2030, which the getter design deliberately avoids; suitable as a follow-up.
github-actions
Bot
added
ai-reviewed/3
Windows sends auto-detect requests in UDP tunnel sub-headers. The client now passes those requests to the session and sends the responses back on the same tunnel, using the transport API from #2030 and the shared responder from #2012.
This PR is stacked on #2012 and rebased onto current upstream master, which now includes #2030 and the #2008 channel/graphics handling.
TunnelMessage::auto_detect_requests,TunnelMessage::from_auto_detect_responses). The read pump stamps each tunnel PDU when it reads it (UdpTransport::last_received_at), on the same clock as the TCP reader'slast_read_at.Validation
CI-visible regression tests cover bandwidth and RTT wire layouts, unknown sub-headers, Start/Stop carrier counting, a measurement window shared across TCP and UDP, and the read pump's arrival timestamps. The shared #2012 tests cover full TCP/fast-path framing and untimed measurements.
No new live Windows interoperability run has been performed for this revision.