Correctly crop bitmap updates when TS_BITMAP_DATA.width is padded - #1436
Closed
Eugene (Eugeny) wants to merge 1 commit into
Closed
Correctly crop bitmap updates when TS_BITMAP_DATA.width is padded#1436Eugene (Eugeny) wants to merge 1 commit into
Eugene (Eugeny) wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a bitmap rendering corruption (diagonal "shear") in ironrdp-session that occurs when an RDP server pads TS_BITMAP_DATA.width beyond the destination rectangle width (spec-legal; xrdp pads to a multiple of 4 pixels). Previously, process_bitmap_update decoded each bitmap at update.width stride, but the DecodedImage::apply_* functions re-chunk the source buffer at update_rectangle.width(), so any width/row padding offset every subsequent row. The fix introduces a repack_bitmap_to_rectangle helper that crops each row to the rectangle width (and the row count to the rectangle height) before the buffer reaches the apply_* functions, covering both the compressed (RLE + RDP6) and uncompressed paths that were previously affected.
Changes:
- Added
repack_bitmap_to_rectangle, an overflow-guarded helper that re-packs padded pixel data to a tightly-packed, rectangle-sized buffer (returnsNonewhen no re-packing is needed). - Applied cropping via two closures (
crop_tightfor compressed streams,crop_paddedfor uncompressed rows padded to 4-byte boundaries) across all depths and both compressed/raw code paths. - Consolidated the previous uncompressed padded/unpadded branches into a single match now fed the cropped buffer.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) { | ||
| Ok(()) => image.apply_rgb24(&buf, &update.rectangle, true)?, | ||
| Ok(()) => { | ||
| let c = crop_tight(&buf, 3); |
Marc-André Moreau (mamoreau-devolutions)
added a commit
that referenced
this pull request
Jul 30, 2026
## 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>
Contributor
|
superseded by #1486 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Downstream issue: warp-tech/warpgate#2173
TL;DR: IronRDP decodes the bitmap incorrectly when
update.widthandupdate_rectangle.width()differAI disclosure: AI was used to investigate and fix this.
process_bitmap_updatedecodes each bitmap atupdate.width/update.height, butDecodedImage::apply_*_bitmapre-chunk the buffer atupdate_rectangle.width(). When a server pads the header width beyond the rectangle (spec-legal; xrdp always pads to a multiple of 4 pixels,e = (4 - width) & 3), each row is offset by the padding → diagonal shear. Flat fills look fine; text/images/the xrdp login dialog hatch. Affects 0.9.0 andmaster, all depths, both compressed and raw paths (apply_rgb16/15_bitmap,apply_bgr24_bitmap,apply_rgb32_bitmap,apply_rgb8_with_palette,apply_rgb24).Fix — treat the header width as source stride; crop each row to
rectangle.width()and the row count torectangle.height()before applying (as FreeRDP/mstsc do). Patch leavesapply_*untouched — feeding them a tightly-packed rectangle-sized buffer makes their existingchunks_exact(rectangle_width * …)correct.