Repository navigation
Conversation
GitHub Actions
Error
github-actions
Bot
added
needs-review
|
This pull request may overlap with #2009. Both PRs implement client-side answering of MS-RDPBCGR auto-detect requests with bandwidth measurement windows, per-transport responders, and byte-counting rules per 3.2.5.14; 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). |
github-actions
Bot
added
the
needs-review
|
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. It fits the auto-detect design already on master: the clock stays outside the state machine as in #1487, the arrival time comes from |
github-actions
Bot
removed
the
needs-review
github-actions
Bot
added
ai-reviewed/2
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
|
Pushed 5a0e311. Continuous measurements now count full TCP/fast-path frames when no RDP Security Header is present, and only post-security bytes when one is present. Untimed or missing windows return zero bytes. The redundant counting wrappers/header parse are gone, and the UDP responder reuse is documented. All 18 autodetect integration tests pass, including new IO/SVC framing, concatenated IO PDUs, multitransport security-header exclusion, and untimed/missing-window cases. Formatting and targeted Clippy with warnings denied pass. The earlier untimed-window log and counted_len helper fixes remain in place. |
github-actions
Bot
added
automation-failed
ignore this, my external review agent, idk why it activated on external PR |
|
Rechecked the failed notification against the latest commit: the normal build/test CI suite passes. The separate public API check fails before comparing changes because its fresh dependency resolution selects incompatible picky-krb 0.12.5 with sspi 0.21.3. The focused workflow repair is #2071, which builds both revisions with their committed lockfiles. It has passed a real IronRDP API build and unchanged/breaking/stale-lockfile fixtures locally. The workflow runs from the base branch, so this check needs that repair merged before a rerun can use it. |
|
Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes. |
github-actions
Bot
added
risk/medium
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.
5a0e311 to
4a873c0
Compare
github-actions
Bot
added
risk/high
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #2012 adds client-side answers to RTT and bandwidth-measure auto-detect requests via a new AutoDetectResponder wired into the x224 message-channel processor, with transport-supplied arrival timestamps threaded through new process_with_timestamp APIs. The core design is sound: counting is gated on an open continuous window, each frame is counted once per path, connect-time counting matches the connector's payload+8 rule, and untimed windows yield a 1 ms/0-byte floor. Residual issues are all low severity: full-frame counting assumes no RDP Security Header (Standard RDP Security paths over-count), a cross-type Stop silently destroys an open measurement window, Bandwidth Measure Stops are always answered even without a timed window (deliberate but partly undocumented), and the NetworkCharacteristicsResult arm is unreachable dead code kept speculatively for PR #2009.
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] Fast-path frames counted in full even when an RDP Security Header is present — low 🟡 — The continuous byte count records frame.len() unconditionally, assuming Enhanced RDP Security. Under Standard RDP Security (PROTOCOL_RDP, a supported IronRDP path) fast-path data carries the security flags byte and data signature, which MS-RDPBCGR 3.2.5.14 excludes from the count, inflating byteCount and skewing server-computed bandwidth. The added comment acknowledges the TLS-only assumption.
| // Ordinary TLS-protected IO data has no RDP Security Header. Count the | ||
| // entire frame once, including framing and any concatenated Share Control PDUs. | ||
| self.record_bandwidth_bytes(frame_len); |
There was a problem hiding this comment.
[protocol] Ordinary IO-channel frames counted in full, inconsistent with security-header-aware branches — low 🟡 — Ordinary IO-channel data adds the entire frame to the continuous count, which is correct only without an RDP Security Header. Under Standard RDP Security, server-to-client IO data always carries a Basic Security Header and 3.2.5.14 requires counting only bytes after it (also excluding TPKT/X224/MCS framing). The multitransport branch (line 283) and message-channel branch (line 501) already follow that rule, so this path over-reports byteCount inconsistently.
| let (time_delta_ms, byte_count) = match (measurement, received_at) { | ||
| (Some(measurement), Some(stopped_at)) => ( | ||
| u32::try_from(stopped_at.duration_since(measurement.started_at).as_millis()) | ||
| .unwrap_or(u32::MAX) | ||
| .max(UNMEASURABLE_INTERVAL_MS), | ||
| measurement.bytes.saturating_add(stop_bytes), | ||
| ), | ||
| (Some(measurement), None) => { | ||
| // The window was timed but this Stop was not, so there is nothing to | ||
| // divide the count by. Log the drop so it does not look like the | ||
| // ordinary no-window case. | ||
| debug!( | ||
| dropped_bytes = measurement.bytes, | ||
| "Bandwidth Measure Stop arrived with no arrival time although its window was open; \ | ||
| dropping the accumulated count" | ||
| ); | ||
| (UNMEASURABLE_INTERVAL_MS, 0) | ||
| } | ||
| (None, _) => (UNMEASURABLE_INTERVAL_MS, 0), | ||
| }; | ||
| Some(AutoDetectResponse::BandwidthMeasureResults { | ||
| sequence_number, | ||
| response_type: if continuous { | ||
| BW_RESULTS_CONTINUOUS | ||
| } else { | ||
| BW_RESULTS_CONNECT_TIME | ||
| }, | ||
| time_delta_ms, | ||
| byte_count, | ||
| }) |
There was a problem hiding this comment.
[protocol] Bandwidth Measure Stop always answered with a fabricated 1 ms/0-byte result when no timed window exists — low 🟡 — Any reliable or connect-time Stop gets a Bandwidth Measure Results reply, even with no window open or when the count was dropped for lack of an arrival time. This is a deliberate, documented convention (PR body; consuming servers discard zero-byte results rather than reading 0 kbps), so it is not a conformance bug, but the two no-measurement cases are handled asymmetrically: the untimed drop logs dropped_bytes while the never-opened (None, _) case returns silently. Aligning the logging would make the behavior diagnosable.
| let continuous = request_type == BW_STOP_RELIABLE_UDP; | ||
| let measurement = self | ||
| .bandwidth | ||
| .take() | ||
| .filter(|measurement| measurement.continuous == continuous); |
There was a problem hiding this comment.
[skeptical] A Stop of the other measurement type silently discards an open measurement window — low 🟡 ❓ — take().filter(|m| m.continuous == continuous) removes the window first and then drops it on a type mismatch: a BW_STOP_CONNECT_TIME while a continuous window is open (or vice versa) destroys the accumulated bytes and start time, replying 1 ms/0 bytes, and a later correctly typed Stop also reports the floor. MS-RDPBCGR does not define cross-type interleaving, so whether to preserve the window is unclear, but the state loss is an undocumented consequence of take() ordering, unlike every other documented edge case in this file, and no test covers a cross-type pair.
| request @ AutoDetectRequest::NetworkCharacteristicsResult { .. } => { | ||
| // The TCP message-channel processor surfaces this request itself. Keep this | ||
| // arm for the UDP tunnel responder introduced in PR #2009, which handles | ||
| // auto-detect requests without passing through that processor. | ||
| debug!(?request, "Received network characteristics from server"); | ||
| None | ||
| } |
There was a problem hiding this comment.
[skeptical + code-compressor] NetworkCharacteristicsResult arm in AutoDetectResponder::respond is unreachable dead code — low 🟡 — AutoDetectResponder is pub(crate) and its only caller, x224::Processor::process_message_channel (x224/mod.rs:537), intercepts NetworkCharacteristicsResult at lines 533-536 and returns ProcessorOutput::AutoDetect before invoking respond, so this arm can never execute. It is behaviorally identical to the catch-all fallback arm (debug log + None), differing only in wording, and is kept speculatively for PR #2009's UDP responder; even under that PR the fallback would serve. The arm and its comment should move with #2009 rather than ship as unreachable code in this PR.
github-actions
Bot
added
ai-reviewed/3
The session answered RTT requests on the message channel but ignored Bandwidth Measure Start and Stop. It now returns measurement results for session-time auto-detection through a shared
AutoDetectResponder.The caller supplies arrival timestamps through
ActiveStage::process_with_timestampandx224::Processor::process_with_timestamp; the client usesFramed::last_read_at. Existingprocesscallers retain the untimed fallback. #2009 uses a separate responder instance for tunnel traffic so TCP and UDP measurements remain independent.Validation
All 18 session auto-detect integration tests pass, including complete framing, Security Header boundaries, repeated Start, untimed Stop, elapsed-time limits, and lossy-request handling. Targeted Clippy with warnings denied, workspace formatting, and diff checks pass. The current regular CI suite is green; the separate API automation dependency failure is addressed by #2071.