Conversation
GitHub Actions
Error
github-actions
Bot
added
kind/protocol
|
This pull request may overlap with #2030. The rdpeudp-tokio changes match PR 2030 almost line-for-line: TunnelMessage, send_message/recv_message, recv dropping sub-headers, SubHeadersTooLarge, and the same tests. The session-side bandwidth/auto-detect work also matches PR 2012 (AutoDetectResponder, process_with_timestamp, identical testsuite tests); this PR appears to combine both plus new client tunnel wiring. 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
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
risk/high
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR correctly wires auto-detect (RTT and bandwidth) handling onto the reliable UDP tunnel: sub-headers flow through TunnelMessage/recv_message/send_message, the send-side 4 + sub-headers <= 255 guard matches MS-RDPEMT's one-byte HeaderLength, and per-transport AutoDetectResponder instances keep tunnel measurements separate from the message channel's. The approach and tests are sound. Remaining published issues: a failed tunnel auto-detect reply send aborts the session even when the unused-tunnel condition would otherwise degrade to TCP (medium); byte-count scope for continuous windows deviates from MS-RDPBCGR 3.2.5.14 on slow-path, fast-path, and tunnel-vs-message-channel paths (low); RTT responses on the tunnel rest on an encapsulation the protocol corpus does not define (low, question); and maintainability cleanups around duplicated encode-redecode helpers, doc(hidden) pub test seams, an always-Some Option parameter, and a tuple-pattern guard (low).
- [protocol] Fast-path frames are counted without their FastPathHeader bytes — low 🟡 — crates/ironrdp-session/src/active_stage.rs
The new fast-path counting records only the bytes remaining after FastPathHeader::decode, so the fast-path header bytes are never added to byteCount, a small systematic undercount relative to the MS-RDPBCGR 3.2.5.14 procedure, which exempts only the Security Header. The counted region also includes Security Header bytes under Standard RDP Security, as in the sibling X.224 path. No effect on PDU structure; only the reported byteCount value.
Benoît Cortier (CBenoit)
added
the
needs-author-action
The RDPEMT codec already handled sub-headers, but UdpTransport exposed only a PDU's data, so the Continuous Auto-Detection messages MS-RDPBCGR 1.3.9 carries there could be neither sent nor received. TunnelMessage carries both; send_message and recv_message use it, and send/recv keep working on data alone.
The sub-header examples carried a whole Bandwidth Measure Start, header bytes included. An auto-detect sub-header is the auto-detect structure itself (MS-RDPEMT 2.2.1.1.1): its SubHeaderLength and SubHeaderType are the structure's headerLength and headerTypeId, so on the wire those examples would repeat the header. The transport treats the data as opaque and the tests passed either way; the examples now carry only what follows the header.
…ment Nothing builds a default TunnelMessage, and a default one would be an empty Tunnel Data PDU. The FramedRead comment said the tunnel consumes the sub-headers before this point; recv drops them, and a caller that needs them uses recv_message.
The send_message precheck restates the HeaderLength bound that ironrdp-rdpemt's TunnelData encoder owns. The boundary test now also runs the largest accepted message and the smallest refused one through the real encoder, so the two cannot drift apart unnoticed, and the comment on the check cites MS-RDPEMT 2.2.1.1.
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.
66e276f to
33080ea
Compare
|
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 33080ea after rebasing onto current #2030 and master, retaining the merged #2008 graphics/channel handling and incorporating the #2012 follow-up. All eight inline review findings and the review-body fast-path accounting finding are addressed. Validation: 5 client tunnel integration tests, 21 session auto-detect integration tests, all-targets Clippy for client/session/core/extra test suites with warnings denied, workspace formatting, and diff checks pass. The shared public API workflow dependency failure is addressed separately by #2071, which needs to land on the base branch before that check can use the repair. |
github-actions
Bot
added
automation-failed
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 #2030 and #2012 and rebased onto current upstream master, including the merged #2008 channel/graphics handling.
__testfeature. A plain monotonic timestamp is required for tunnel measurements.Validation
CI-visible regression tests cover bandwidth and RTT wire layouts, unknown sub-headers, TCP fallback before channel migration, failure after migration, Start/Stop carrier counting, and separate TCP/UDP measurement windows. 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.