fix(session): Fix bitmap stride mismatch and out-of-bounds writes for xRDP compatibility

[Amartya Anshuman (amartyaa)]

Fixes #1251

Summary

This PR fixes two related issues in ironrdp-session that cause diagonal bitmap distortion and index-out-of-bounds panics when connecting to xRDP servers.

Changes

crates/ironrdp-session/src/fast_path.rs

xRDP sends bitmap updates where the data dimensions (update.width × update.height) differ from the destination rectangle (update.rectangle). Per MS-RDPBCGR §2.2.9.1.1.3.1.2.2, bitmapWidth/bitmapHeight define the pixel data layout while destLeft/destTop/destRight/destBottom define the screen placement.
The apply_* methods use the rectangle's width as the row stride. Passing update.rectangle when it's wider/narrower than the actual data causes diagonal shearing.
Fix: Construct a blit_rect whose dimensions match the actual bitmap data, positioned at the destination's top-left corner:

let blit_rect = InclusiveRectangle {
    left: update.rectangle.left,
    top: update.rectangle.top,
    right: update.rectangle.left + update.width.saturating_sub(1),
    bottom: update.rectangle.top + update.height.saturating_sub(1),
};

crates/ironrdp-session/src/image.rs

Added bounds checks to all apply_* bitmap methods that were missing them:

• apply_rgb16_bitmap
• apply_rgb15_bitmap
• apply_bgr24_bitmap
• apply_rgb8_with_palette
• apply_rgb24_iter
• apply_rgb32_bitmap (both same-format and cross-format paths)
• data_for_rect (clamped to buffer length)

Each pixel write is now guarded with if dst_idx + bytes_per_pixel < self.data.len().
This is consistent with how other parts of the codebase handle edge-of-framebuffer updates — pixels beyond the buffer are silently clipped rather than panicking.

Also added a max_rows clamp in apply_rgb24_iter to prevent iterating beyond the framebuffer height.

Testing

• Tested against xRDP 0.10.x (Ubuntu 22.04, Xorg session with XFCE)
• Verified correct rendering at 8bpp, 15bpp, 16bpp, 24bpp, and 32bpp
• No more diagonal distortion on the xRDP login screen or desktop
• No more panics during session startup or bitmap updates near framebuffer edges
• Verified no regression when connecting to Windows RDP servers (Windows 10/11)

[Greg Lamberson (glamberson)]

Thanks for the report and the fix Amartya Anshuman (@amartyaa). The stride insight is correct: MS-RDPBCGR §2.2.9.1.1.3.1.2.2 does distinguish bitmapWidth/bitmapHeight (the pixel data layout) from destLeft/destTop/destRight/destBottom (the screen placement), and the existing apply_* methods used the destination rectangle's width as the row stride, which is exactly the conflation this fix addresses. The diagonal shear is what that mismatch looks like on screen.

A few suggestions a reviewer is likely to raise:

1. Horizontal over-paint when bitmapWidth > destRect.width(). The same section of MS-RDPBCGR continues with: "If the size of the bitmap data exceeds the size of the rectangle, the additional rows and columns MUST be discarded by the client." The blit_rect here uses update.width and update.height as its dimensions, anchored at destLeft/destTop. When the bitmap is wider than the destination rectangle, which is xRDP's typical padding-to-4-alignment case, the extra columns are still rendered to the right of destRight, into pixels that belong to the next destination over. The bounds checks prevent panics, but they do not stop wrong pixels from landing outside the destination area on the framebuffer. A spec-compliant version would also clip right to min(update.width, destRect.width()) and bottom to min(update.height, destRect.height()) when computing blit_rect. In practice the over-paint with xRDP is small, since the extra columns are typically padding, and subsequent updates usually cover them. That is probably why the fix works visually in your testing. The condition is still worth tightening for spec compliance.

2. Tests. A regression test for the stride case (construct a known-pattern bitmap whose data width differs from the destination width, call apply_rgb32_bitmap, assert specific pixel positions in the resulting DecodedImage) plus one for the out-of-bounds case (rectangle extending past self.width or self.height, assert no panic and that in-bounds pixels are written correctly) would lock these in.

3. Inconsistent bounds-check operators. Five sites use if dst_idx + 3 < self.data.len() for the 4-byte RGBA writes that follow. One site (the cross-format 32bpp slice copy near line 858) uses if dst_idx + SRC_COLOR_DEPTH <= self.data.len(). Both are arithmetically correct, but mixing < and <= for the same conceptual check looks like drift. Picking one form for consistency would clean this up.

4. Silent clipping vs debug_assert!. The bounds-check pattern silently drops out-of-bounds pixel writes. That matches the existing edge-of-framebuffer policy and the rect_fits early return in apply_rgb32_bitmap, so it is consistent. But it also masks any future bug that produces a wrong dst_idx. A debug_assert! or a trace! log on hit would surface those during development without affecting release behavior.

Downstream context: this is relevant to Lamco's xRDP interop testing. Our test fleet uses xRDP 0.10.x on Ubuntu as a non-Windows comparison server, and IronRDP clients connecting there hit the diagonal-distortion symptom you describe. A version of this fix landing upstream would resolve a recurring noise item in our fleet runs. If a revised version lands, I can run it through our xRDP test fleet to confirm.

[Amartya Anshuman (amartyaa)]

Hello Greg Lamberson (@glamberson) !!
Thanks for the thorough review! I've addressed all four points. TLDR:

1. Horizontal over-paint clipping - Updated blit_rect construction in fast_path.rs to clip dimensions to the smaller of the bitmap data and the destination rectangle.This ensures that when xRDP pads bitmapWidth to a 4-byte alignment boundary (e.g., bitmapWidth=64 for a destRight - destLeft + 1 = 61 rectangle), the extra 3 padding columns are discarded rather than painted into adjacent framebuffer pixels.

2. Tests - Added 4 regression tests in a new #[cfg(test)] mod tests block at the end of image.rs.

3. Consistent bounds-check operators - Standardized all 7 bounds checks to use <= .

4. debug_assert! - Added debug_assert! before each bounds check with a descriptive message identifying which method triggered it.

---

Happy to iterate further. Looking forward to Lamco's xRDP fleet results once this lands!

[Copilot]

Pull request overview

Fixes diagonal bitmap shearing and out-of-bounds panics observed when ironrdp-session connects to xRDP servers, which send BitmapData whose pixel dimensions (update.width/update.height) don't match the destination rectangle. The fix derives a blit_rect whose size is the per-spec clip of bitmap-data vs. destination-rect dimensions and routes all apply_* calls through it, plus adds per-pixel/per-slice bounds guards in image.rs and enables unit tests for the crate.

Changes:

• In fast_path.rs::process_bitmap_update, build a blit_rect from min(update.width, rect.width) / min(update.height, rect.height) and pass it to every apply_* path instead of update.rectangle.
• In image.rs, add debug_assert! + if dst_idx + DST_COLOR_DEPTH <= self.data.len() per-pixel guards to all slow-path apply methods, a max_rows clamp in apply_rgb24_iter, and a silent clamp in data_for_rect; also add a tests module covering rgb32 placement, rgb16 in-bounds writes, oversize rect rejection, and data_for_rect clamping.
• In ironrdp-session/Cargo.toml, comment out test = false to enable the new unit tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
crates/ironrdp-session/src/fast_path.rs	Introduces clipped blit_rect and routes all apply_* calls through it to fix stride/shearing.
crates/ironrdp-session/src/image.rs	Adds redundant-with-rect_fits per-pixel guards, clamps data_for_rect, and adds a unit-test module.
crates/ironrdp-session/Cargo.toml	Comments out test = false to allow the new tests to run.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Greg Lamberson (glamberson)]

Hi Amartya Anshuman (@amartyaa), your four points are addressed cleanly. The blit_rect clipping in fast_path.rs matches the MS-RDPBCGR §2.2.9.1.1.3.1.2.2 read on destRect vs bitmapWidth/bitmapHeight semantics, the new #[cfg(test)] mod tests location follows the IronRDP convention, and the <= standardization paired with debug_assert! reads well in debug builds.

Copilot's follow-up review has some substantive items worth working through. In particular, the asymmetric-clipping note at fast_path.rs:211 extends our original review #1: update.width.min(update.rectangle.width()) covers xRDP's wider-dest case, but per the same MS-RDPBCGR section the spec also requires handling the inverse (bitmap data wider than the rectangle, where chunks_exact(clipped_width * bpp) would re-introduce the same diagonal shearing this PR is fixing). And the data_for_rect silent-clamp note at image.rs:197 is a real contract change worth either documenting or replacing with an !self.rect_fits(rect) early return matching the pattern elsewhere in the file.

I'll run the patched version against our xRDP 0.10.x test fleet once you've worked through Copilot's review; running against an in-flight revision wouldn't tell us much about the final shape. Should take 1-2 days from your next push.

[Amartya Anshuman (amartyaa)]

Hello Greg Lamberson (@glamberson)! I think all the points are covered now.

[Greg Lamberson (glamberson)]

Amartya Anshuman (@amartyaa) First, apologies for the slow turnaround on getting back to you here. Thanks for the revision. The data_for_rect change (early return + empty slice on a non-fitting rect, plus the debug_assert) looks good, and the stride-versus-columns split in the apply_* methods (chunks_exact(stride_width) for the row stride, .take(rectangle_width) for the columns) is the right shape for the asymmetric case.

I think one piece of Copilot's asymmetric-clipping point is still open, though, in the bitmap-wider-than-rect direction. It is at the call sites rather than in the apply_* methods.

All of the decoded buffers have a row stride of update.width:

• RDP6: decode_bitmap_stream_to_rgb24(..., update.width, update.height)
• RLE: rle::decompress(..., update.width, update.height, bpp)
• uncompressed with padding: the padding-strip copies row_bytes = width * bytes_per_pixel (width = update.width) into buf
• uncompressed without padding: raw update.bitmap_data at update.width stride

But only the RDP6 path passes update.width as data_stride:

Ok(()) => image.apply_rgb24(&buf, &blit_rect, update.width, true)?,

The RLE and uncompressed paths pass clipped_width instead, for example:

Ok(RlePixelFormat::Rgb16) => image.apply_rgb16_bitmap(&buf, &blit_rect, clipped_width)?,
...
32 => image.apply_rgb32_bitmap(&buf, PixelFormat::BgrX32, &blit_rect, clipped_width)?,

When update.width > update.rectangle.width() (the xRDP case where bitmapWidth is padded up past the destination), clipped_width = rect.width < update.width. So chunks_exact(clipped_width * depth) walks a buffer that is actually update.width-strided, and each row starts at the wrong offset. The .take(rectangle_width) correctly prevents horizontal over-paint, but it does not realign the rows, so the diagonal shear is still there. Concretely, with update.width = 8 and rect.width = 5, chunks_exact(5) makes the second row begin at pixel 5 of the first source row.

The fix is small and matches what the RDP6 path already does: pass update.width (not clipped_width) as data_stride in the four non-RDP6 call sites. blit_rect already carries the clipped width, so the rectangle_width-based .take(...) inside each apply_* still caps the written columns correctly. So it ends up as: read at update.width stride, write clipped_width columns.

One note on the new tests: they call the apply_* functions directly with the true stride (for example apply_rgb32_bitmap(..., data_stride = 8) with a rect width of 5), which is why they pass. That exercises the apply_* logic correctly but does not go through the fast_path.rs call sites, so it does not cover the value passed there. A small test that drives a wider-than-rect bitmap through Processor for the RLE or uncompressed path would lock this down.

Once that is in, I will run the result through our xRDP test fleet and report back. Heads-up that the branch is currently showing as conflicting with master, so it will need a rebase as well.

[Marc-André Moreau (mamoreau-devolutions)]

partially superseded by #1486 if you want to rebase

[Benoît Cortier (CBenoit)]

Automated review found two blocking QOIZ decoding denial-of-service risks in crates/ironrdp-session/src/fast_path.rs: an empty compressed payload initializes a zero-length output buffer and can loop forever because resizing retains zero capacity, and attacker-controlled Zstandard content can cause unbounded output-buffer doubling before QOI validation, permitting memory exhaustion. Please bound decompressed output to destination or negotiated codec limits and reject empty or non-progressing streams.