Fix Win7 RDP PDU and bitmap stride handling - #1408
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets two Windows 7 RDP interoperability issues in ironrdp-session. First, it handles the case where a Win7 server concatenates multiple Share Control PDUs inside a single MCS Send Data Indication on the I/O channel by iterating over each sub-PDU. Second, it fixes bitmap rendering when the server sends bitmap data whose per-row stride (source width) is wider than the destination rectangle width, by introducing a normalize_bitmap_rows helper that repacks rows to the destination width before handing data to the apply_* functions.
Changes:
- Add
process_io_channel_data_indicationto split and process concatenated Share Control PDUs on the I/O channel. - Add
normalize_bitmap_rowshelper and route all compressed (RDP6/RLE) and uncompressed bitmap paths through it to strip stride padding / extra columns down to the destination rectangle width. - Replace the previous inline row-padding-stripping logic in the uncompressed path with the shared helper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/ironrdp-session/src/x224/mod.rs |
Routes I/O-channel data through a new PDU-splitting method; splitting assumes a Share Control Header and mis-handles Multitransport Request PDUs. |
crates/ironrdp-session/src/fast_path.rs |
Adds normalize_bitmap_rows and uses it across all bitmap color-depth paths to normalize source stride to the destination rectangle width. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let total_length = usize::from(u16::from_le_bytes([data[offset], data[offset + 1]])); | ||
| if total_length == 0 || offset + total_length > data.len() { | ||
| if offset == 0 { | ||
| return self.process_io_channel(data_ctx); | ||
| } | ||
| return Err(reason_err!( | ||
| "X224", | ||
| "invalid concatenated Share Control PDU length: {total_length}" | ||
| )); | ||
| } |
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you for the PR!
|
|
||
| const DEFAULT_POINTER_CACHE_SIZE: u16 = 32; | ||
|
|
||
| /// 中文注释：根据连接配置和服务端能力生成 Client Confirm Active，legacy 图形模式会跳过增强 surface 能力。 |
There was a problem hiding this comment.
Could you translate the comments in English? Not many of us understands Chinese. Thanks! 🙂
## Summary - Preserve `TS_BITMAP_DATA` source stride independently of destination bounds for raw, Interleaved RLE, and RDP6 bitmap updates. - Remove raw 4-byte scanline padding without collapsing padded source columns into following rows. - Crop only the source extent beyond the destination and reject empty dimensions explicitly; do not add framebuffer bounds suppression. - Render decoded RDP6 RGB data top-down and retain bottom-up rendering for raw and RLE data. ## Why this supersedes the overlapping proposals - #1252 identifies both dimensions but applies a clipped stride, which still misaddresses padded source rows; its broad framebuffer-clipping changes are intentionally excluded. - #1398 correctly identifies the RDP6 stride/orientation issue, but leaves raw and RLE paths unresolved. - #1408 includes the related stride issue but also mixes unrelated Win7 activation and web changes; its allocation/inference normalization is unnecessary once source stride is explicit. - #1436 correctly crops padded rows but repacks each path into temporary buffers; this PR preserves the source stride directly and covers all bitmap codecs. ## Specification basis - MS-RDPBCGR 2.2.9.1.1.3.1.2.2 (`TS_BITMAP_DATA`): separate destination bounds, dimensions, and bottom-up raw rows with 4-byte row padding. - MS-RDPBCGR 2.2.9.1.1.3.1.2.3 and 2.2.9.1.1.3.1.2.4: compressed bitmap header and Interleaved RLE stream. - MS-RDPEGDI 2.2.2.5.1: RDP 6.0 bitmap stream. ## Validation - `cargo test -p ironrdp-session --lib` - `cargo check -p ironrdp-session --all-features` - `cargo clippy -p ironrdp-session --all-targets -- -D warnings` - `cargo xtask check fmt -v` Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
partially superseded by #1486 if you want to rebase |
github-actions
Bot
added
scope/web
Benoît Cortier (CBenoit)
added
risk/high
|
High-risk Windows 7 RDP compatibility change reviewed: legacy mode clears Client Info compression and suppresses Surface Commands, Bitmap Codecs, and Frame Acknowledge when no codec configuration exists, while enhanced mode preserves advertised capabilities. Skeptical review found no high-confidence correctness, protocol, API, or safety findings. |
github-actions
Bot
added
risk/medium
No description provided.