Repository navigation
Conversation
GitHub Actions
Error
github-actions
Bot
added
kind/protocol
…#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>
Benoît Cortier (CBenoit)
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 #2014 adds client-side layout tracking to ResizeQueue to drop no-op Display Control resize requests and treats a framebuffer size change while a resize is in flight as completion via the graphics pipeline's ResetGraphics. Both fixes are well-motivated and correctly address the reported blanking reconnects on Windows hosts. Independent inspection confirms three remaining gaps: the completion heuristic accepts any framebuffer size change without correlating it to the in-flight request's dimensions while mark_in_flight eagerly records the layout as applied (so an out-of-band ResetGraphics or a declined request can permanently suppress later retries of an unapplied layout); scale-only resizes that leave pixel dimensions unchanged can never complete through this path and still fall back to a reconnect; and the deferred-request promotion path (pre-existing code) sends queued requests without re-checking whether they have become no-ops, reproducing the same timeout reconnect. A minor clea…
- [skeptical] No-op suppression is not applied when a deferred request is promoted for sending — medium 🟠 — crates/ironrdp-client/src/rdp.rs
asks_for_current_layout is consulted only when a fresh resize event arrives (line 3332). The promotion path (lines 4280-4295) sends a pending request unconditionally once in_flight is None and display control is ready, without re-checking whether it became a no-op. Counterexample verified in code: request A in flight, duplicate request B deferred at line 3338; A completes via the new ResetGraphics path, so framebuffer and layout now match B; B is promoted and sent, the server treats it as a no-op that never completes, and after DISPLAY_CONTROL_READY_TIMEOUT the session returns ReconnectWithNewSize at line 3641 - the same blanking reconnect this PR aims to remove. Re-applying the check in the promotion path is a small local change.
github-actions
Bot
added
ai-reviewed/1
bd7c7f3 to
b336e85
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. |
github-actions
Bot
added
automation-failed
|
Updated and rebased onto current master in b336e85. All review findings are addressed, including the deferred-request no-op check from the review body. Explicit ResetGraphics notifications now cover TCP and reliable UDP, including unchanged pixel sizes. Validation: 23 client library tests, 10 session integration tests, targeted all-targets Clippy with warnings denied, workspace formatting, and diff checks pass. The separate public API automation failure is the shared fresh dependency-resolution problem addressed by #2071; that workflow repair must land on the base branch to affect this PR. |
|
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
A Display Control resize was only considered complete once the server ran a Deactivation-Reactivation Sequence. With the graphics pipeline, Windows completes it with a ResetGraphics declaring the new output size instead, and the session already follows that by resizing the framebuffer. The client kept waiting for a reactivation that never came, so after the deadline every resize ended in a reconnect that blanked the window. An in-flight resize is now complete once the framebuffer size changes. A request for the layout the server already has, which is what a window reports right after it opens, is a no-op for the server: it neither reactivates nor resets graphics, so it ran into the same deadline. Such a request is now dropped, along with any deferred request it supersedes. The comparison includes the scale factor and physical size, starting from the scale factor the connection was made with, so a DPI change at the same pixel size is still sent.
b336e85 to
1e35f55
Compare
github-actions
Bot
added
risk/high
1e35f55 to
15cf872
Compare
There was a problem hiding this comment.
PR #2014 reworks client resize bookkeeping: ActiveStage gains take_graphics_output_reset() to surface RDPGFX ResetGraphics (including same-size resets over TCP and reliable UDP), and ResizeQueue gains a confirmed_layout cache, dimension-checked completion, and a no-reconnect timeout path for unacknowledged metadata-only requests. I independently verified the state machine, all call sites (reset consumer at rdp.rs:3785, reactivation at 4150, resize-event and pending-promotion arms), and the new tests; the core design is sound and well-covered. Four candidate findings survive review: (1) a genuine medium-severity behavior gap where a server-applied adjusted layout leaves the request in flight until the 3-second deadline and then triggers the reconnect fallback the PR set out to eliminate; (2) a plausible low-severity/questionable tension where a same-size reset records scale/physical-size metadata as confirmed even if the server ignored it, permanently suppressing identical retries; and…
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.
| fn completed(&mut self, desktop_size: (u16, u16)) -> bool { | ||
| if self | ||
| .in_flight | ||
| .as_ref() | ||
| .is_some_and(|in_flight| (in_flight.request.width, in_flight.request.height) == desktop_size) | ||
| { | ||
| self.confirmed_layout = self.in_flight.take().map(|in_flight| in_flight.request); | ||
| return true; | ||
| } | ||
|
|
||
| // An unsolicited or adjusted layout does not confirm an outstanding request. | ||
| self.confirmed_layout = None; | ||
| false | ||
| } |
There was a problem hiding this comment.
[skeptical] Mismatched completion leaves the request in flight, forcing the reconnect fallback for adjusted layouts — medium 🟠 — completed() only takes in_flight when the reported dimensions match; on mismatch it clears confirmed_layout and returns false while in_flight and its deadline survive. Both completion sites only observe the size: the ResetGraphics consumer (rdp.rs:3785) logs and the reactivation site (rdp.rs:4150) discards the bool. If the server applies an adjusted or unsolicited layout — the case the code's own comment at line 682 anticipates — nothing cancels the armed deadline, so timed_out_request sees the request dimensions differ from desktop_size, fails the metadata-only predicate, and returns a ReconnectWithNewSize with the originally requested size about 3 seconds after a resize that actually succeeded. The previous unconditional completed() treated this as a clean completion; the reconnect also re-requests dimensions the server just adjusted, so it cannot converge. Consider completing the request when the reset/reactivation signal arrives regardless of dimension match, or at least clearing in_flight on mismatch.
| /// Whether a request repeats the last completed layout, with nothing in flight | ||
| /// that could change it. Servers can ignore such requests without sending a reset. | ||
| fn asks_for_current_layout(&self, request: &ResizeRequest, desktop_size: (u16, u16)) -> bool { | ||
| self.in_flight.is_none() | ||
| && (request.width, request.height) == desktop_size | ||
| && self.confirmed_layout == Some(*request) | ||
| } |
There was a problem hiding this comment.
[skeptical] Pixel-only match records scale/physical metadata as confirmed, permanently suppressing retries — low 🟡 ❓ — completed() records the entire in_flight request — including scale_factor and physical_size — as confirmed_layout once the reported pixel dimensions match, and asks_for_current_layout then drops any identical follow-up as a no-op. The timeout path's own rationale (rdp.rs:733-736) states servers can ignore scale/physical-size changes and deliberately keeps such metadata retryable; if a server applies the pixel change (sending ResetGraphics) but ignores the optional metadata, the metadata is recorded as applied without being applied and every subsequent identical request is silently discarded, with no retry path. Whether real servers emit ResetGraphics while ignoring scale metadata cannot be established from the repository; if they do not, this is unreachable. The test resize_queue_completes_scale_only_and_physical_size_changes locks in the behavior rather than demonstrating server compliance.
| if (request.width, request.height) == desktop_size | ||
| && self | ||
| .in_flight | ||
| .as_ref() | ||
| .is_none_or(|in_flight| (in_flight.request.width, in_flight.request.height) == desktop_size) | ||
| { | ||
| // Display Control has no explicit acknowledgement. A server can ignore | ||
| // scale/physical-size changes or a layout it already has (MS-RDPEDISP 1.3). | ||
| // Reconnecting to the same pixel size cannot recover those changes. Leave | ||
| // metadata unconfirmed so a later request can retry it. | ||
| debug!("Display layout metadata was not confirmed; keeping the current connection"); | ||
| if self.in_flight.take().is_none() { | ||
| // Capabilities never arrived, so the metadata update cannot be sent. | ||
| self.pending = None; | ||
| } | ||
| // A newer deferred request still needs to be promoted and sent. | ||
| self.confirmed_layout = None; | ||
| None |
There was a problem hiding this comment.
[code-compressor] Metadata-only 'do not reconnect' predicate is encoded three times — low 🟡 — The same rule appears in three shapes: timed_out_request here (request matches desktop_size AND in_flight is none-or-matching, lines 727-731), the resize-event arm at lines 3429-3432, and the pending-promotion arm at lines 4423-4426. At the latter two sites in_flight is statically None (the defer arm at 3408 and the outer in_flight.is_none() guard at 4355 already caught it), so the strict predicate reduces to exactly their behavior. Extracting a single ResizeQueue method (e.g. is_metadata_only) for all three call sites, keeping their distinct debug logs and clearing steps, gives one canonical definition of when a same-pixel reconnect is skipped so a future change to the retry rule cannot miss a site. Trade-off: the timeout variant additionally takes in_flight and clears confirmed_layout, so only the predicate is shared.
| fn asks_for_current_layout(&self, request: &ResizeRequest, desktop_size: (u16, u16)) -> bool { | ||
| self.in_flight.is_none() | ||
| && (request.width, request.height) == desktop_size | ||
| && self.confirmed_layout == Some(*request) | ||
| } |
There was a problem hiding this comment.
[code-compressor] Hand-written (width, height) == desktop_size tuple comparison repeated at six added sites — low 🟡 — The pattern (request.width, request.height) == desktop_size is written out on added lines 676, 691, 727, 731, 3429 and 4423 of rdp.rs. A one-line inherent method on the private ResizeRequest struct (e.g. fn matches_size(&self, desktop_size: (u16, u16)) -> bool) replaces each tuple construction with a named call and keeps the pixel-size comparison defined once next to the type it compares. Purely mechanical and behavior-preserving: ResizeRequest is a private Copy type, so there are no API or borrow concerns. Without it the code is still correct, just noisier at every future size check.
github-actions
Bot
added
ai-reviewed/2
When the graphics pipeline is active, Windows can finish a Display Control resize with ResetGraphics instead of a deactivation/reactivation sequence. The client now consumes an explicit session notification for that reset, preventing the timeout-driven reconnect that blanks the desktop.
Rebased onto current master, including #2008's shared TCP/UDP graphics drain. Adds
ActiveStage::take_graphics_output_reset()without changing existing processing methods.Validation
rustls,rdpdrand warnings denied.git diff --checkpass.