Conversation
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.
GitHub Actions
Error
github-actions
Bot
added
needs-review
|
This pull request may overlap with #2009. Both touch client-side answering of MS-RDPBCGR 2.2.14 auto-detect requests via an AutoDetectResponder in ironrdp-session: this PR adds RTT and bandwidth responses on the message channel with arrival timestamps, while 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 |
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.
github-actions
Bot
removed
the
needs-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR answers MS-RDPBCGR bandwidth-measurement auto-detect requests on the message channel via a new AutoDetectResponder, adds process_with_timestamp plumbing to ActiveStage and x224::Processor, and counts received bytes on the fast-path and X.224 paths. Independent review confirms the protocol behavior (types, security-header exclusion, payloadLength+8 accounting, window restart, 1 ms floor, lossy requests unanswered) against 2.2.14/3.2.5.14 and the connector precedent; no correctness or wire-format defect was found. Three low-severity quality findings are published: a duplicate FastPathHeader decode on every fast-path frame, silent discard of accumulated bytes when a Stop arrives without a timestamp (the connector logs this case), and duplicated payloadLength+8 arithmetic versus the connector's counted_len. The proposed removal of the responder's NetworkCharacteristicsResult arm is rejected as deliberate support for the UDP-tunnel instance in #2009.
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.
github-actions
Bot
added
needs-review
There was a problem hiding this comment.
The PR correctly answers MS-RDPBCGR auto-detect RTT and bandwidth requests on the message channel, threading arrival timestamps from Framed::last_read_at into new process_with_timestamp APIs and a reusable AutoDetectResponder. Verified against the head: counting sites do not overlap, the Basic Security Header is excluded on the message channel, fast-path counting is gated to open continuous windows and counts post-header bytes, repeated Start resets the window, timing saturates with a 1 ms floor, lossy requests are ignored, and the untimed path degrades to a zero-byte result. No protocol, correctness, or safety defects found. Three valid low-severity maintainability findings remain: an arm in AutoDetectResponder::respond unreachable within this crate (kept for #2009 reuse), an untimed connect-time Stop that reports unmeasured stop bytes against the 1 ms floor, and an optional consolidation of two pass-through bandwidth-count wrappers.
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. |
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.