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. |
`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 PRs implement client-side answering of bandwidth measure Start/Stop via a shared AutoDetectResponder in ironrdp-session, expose ActiveStage::process_with_timestamp and x224::Processor::process_with_timestamp for caller-supplied arrival timestamps, count bytes per MS-RDPBCGR 3.2.5.14 including the security-header exclusion, and touch the same autodetect testsuite file. This PR extends the same scope to the reliable UDP tunnel and its sub-header encapsulation. 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
![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 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.
There was a problem hiding this comment.
No code change. IronRDP supports Standard RDP Security only at ENCRYPTION_LEVEL_NONE with no RC4 Security Exchange (see the check in ironrdp-connector connection.rs). At that level, fast-path and slow-path data PDUs have no security header: FASTPATH_OUTPUT_ENCRYPTED is never set, so there is no dataSignature. The PDUs that do carry a Basic Security Header (message channel, multitransport) already have its 4 bytes subtracted. An 8-byte signature header only exists with encryption, which IronRDP cannot negotiate. If RC4 support is ever added, the counting points will need the header length. Until then the count matches 3.2.5.14 on every path IronRDP can negotiate.
There was a problem hiding this comment.
Follow-up: 4630f9b rewords the comments on the fast-path, IO and SVC counting points to give this reason. Under Enhanced RDP Security, or Standard RDP Security at ENCRYPTION_LEVEL_NONE (the only level IronRDP supports), these PDUs have no securityHeader (MS-RDPBCGR 2.2.8.1.1.2), and the fast-path decoder never skips a dataSignature. This matches #2012.
| /// 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.
There was a problem hiding this comment.
No change in this PR. As the review notes, returning the time with the message would change the public recv_message signature that #2030 just merged, and the getter was chosen to avoid that. Behaviour is the same either way. I will leave the signature change for a follow-up, if maintainers want it.
github-actions
Bot
added
ai-reviewed/3
… type MS-RDPBCGR 3.2.5.14 has a reliable Stop stop the timer and send the store, whichever Start opened the window. Only a connect-time Stop in a connect-time window adds its payload and header to the count.
github-actions
Bot
added
automation-failed
A Bandwidth Measure Stop of the other measurement type no longer ends the open window, so a later Stop of the matching type still reports it. Log a Stop that finds no matching window, and state why fast-path, IO and SVC data carry no RDP Security Header. This matches Devolutions#2012.
github-actions
Bot
added
risk/high
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.