Skip to content

feat(egfx)!: switch WireToSurface1Pdu rectangles to exclusive bounds #1238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Benoît Cortier (CBenoit) merged 1 commit into from
May 15, 2026
Merged

feat(egfx)!: switch WireToSurface1Pdu rectangles to exclusive bounds #1238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions crates/ironrdp-egfx/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ use std::collections::BTreeMap;
use ironrdp_core::{Decode as _, ReadCursor, impl_as_any};
use ironrdp_dvc::{DvcClientProcessor, DvcMessage, DvcProcessor};
use ironrdp_graphics::zgfx;
use ironrdp_pdu::geometry::{InclusiveRectangle, Rectangle as _};
use ironrdp_pdu::geometry::{ExclusiveRectangle, Rectangle as _};
use ironrdp_pdu::{PduResult, decode_cursor, decode_err, pdu_other_err};
use tracing::{debug, trace, warn};

Expand Down Expand Up @@ -186,8 +186,8 @@ impl CodecCapabilities {
pub struct BitmapUpdate {
/// Surface this update applies to
pub surface_id: u16,
/// Destination rectangle within the surface
pub destination_rectangle: InclusiveRectangle,
/// Destination rectangle within the surface (exclusive `right`/`bottom`)
pub destination_rectangle: ExclusiveRectangle,

@glamberson Greg Lamberson (glamberson) May 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d6e827a. Changed >= to > for both right and bottom so a full-surface update (right == surface.width, bottom == surface.height) is no longer flagged as out of bounds. Comment updated to note the exclusive semantics.

/// Codec that produced this update
pub codec_id: Codec1Type,
/// RGBA pixel data (4 bytes per pixel), row-major
Expand Down Expand Up @@ -674,8 +674,10 @@ impl GraphicsPipelineClient {
return Err(pdu_other_err!("invalid destination rectangle ordering"));
}

// Validate destination rectangle against surface bounds
if rect.right >= surface.width || rect.bottom >= surface.height {
// Validate destination rectangle against surface bounds. The rectangle
// uses exclusive `right`/`bottom`, so a full-surface update has
// `right == surface.width` and `bottom == surface.height`, which is valid.
if rect.right > surface.width || rect.bottom > surface.height {
warn!(
surface_id = pdu.surface_id,
rect_right = rect.right,
Expand Down Expand Up @@ -706,7 +708,7 @@ impl GraphicsPipelineClient {
Ok(())
}

fn decode_avc420(&mut self, surface_id: u16, dest_rect: &InclusiveRectangle, bitmap_data: &[u8]) -> PduResult<()> {
fn decode_avc420(&mut self, surface_id: u16, dest_rect: &ExclusiveRectangle, bitmap_data: &[u8]) -> PduResult<()> {
let mut cursor = ReadCursor::new(bitmap_data);
let stream = Avc420BitmapStream::decode(&mut cursor).map_err(|e| decode_err!(e))?;

Expand Down
12 changes: 8 additions & 4 deletions crates/ironrdp-egfx/src/pdu/cmd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use ironrdp_core::{
};
use ironrdp_dvc::DvcEncode;
use ironrdp_pdu::gcc::Monitor;
use ironrdp_pdu::geometry::{ExclusiveRectangle, InclusiveRectangle};
use ironrdp_pdu::geometry::ExclusiveRectangle;
use ironrdp_pdu::{DecodeError, cast_length, ensure_size, read_padding, write_padding};
use tracing::warn;

Expand Down Expand Up @@ -306,13 +306,17 @@ impl<'de> Decode<'de> for GfxPdu {

/// 2.2.2.1 RDPGFX_WIRE_TO_SURFACE_PDU_1
///
/// `destination_rectangle` is an [`ExclusiveRectangle`] because MS-RDPEGFX
/// 2.2.1.4.1 RDPGFX_RECT16 specifies `right` and `bottom` as exclusive
/// (one-past-end), matching FreeRDP and the Windows reference clients.
///
/// [2.2.2.1]: <https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpegfx/fb919fce-cc97-4d2b-8cf5-a737a00ef1a6>
#[derive(Clone, PartialEq, Eq)]
pub struct WireToSurface1Pdu {
pub surface_id: u16,
pub codec_id: Codec1Type,
pub pixel_format: PixelFormat,
pub destination_rectangle: InclusiveRectangle,
pub destination_rectangle: ExclusiveRectangle,
pub bitmap_data: Vec<u8>,
}

Expand All @@ -331,7 +335,7 @@ impl fmt::Debug for WireToSurface1Pdu {
impl WireToSurface1Pdu {
const NAME: &'static str = "WireToSurface1Pdu";

const FIXED_PART_SIZE: usize = 2 /* SurfaceId */ + 2 /* CodecId */ + 1 /* PixelFormat */ + InclusiveRectangle::FIXED_PART_SIZE /* Dest */ + 4 /* BitmapDataLen */;
const FIXED_PART_SIZE: usize = 2 /* SurfaceId */ + 2 /* CodecId */ + 1 /* PixelFormat */ + ExclusiveRectangle::ENCODED_SIZE /* Dest */ + 4 /* BitmapDataLen */;
}

impl Encode for WireToSurface1Pdu {
Expand Down Expand Up @@ -363,7 +367,7 @@ impl<'a> Decode<'a> for WireToSurface1Pdu {
let surface_id = src.read_u16();
let codec_id = Codec1Type::try_from(src.read_u16())?;
let pixel_format = PixelFormat::try_from(src.read_u8())?;
let destination_rectangle = InclusiveRectangle::decode(src)?;
let destination_rectangle = ExclusiveRectangle::decode(src)?;
let bitmap_data_length = cast_length!("BitmapDataLen", src.read_u32())?;

ensure_size!(in: src, size: bitmap_data_length);
Expand Down
28 changes: 16 additions & 12 deletions crates/ironrdp-egfx/src/server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ use ironrdp_core::{Encode, EncodeResult, WriteCursor, decode, impl_as_any};
use ironrdp_dvc::{DvcEncode, DvcMessage, DvcProcessor, DvcServerProcessor};
use ironrdp_graphics::zgfx::{CompressionMode, Compressor, compress_and_wrap_egfx, wrap_uncompressed};
use ironrdp_pdu::gcc::Monitor;
use ironrdp_pdu::geometry::InclusiveRectangle;
use ironrdp_pdu::geometry::ExclusiveRectangle;
use ironrdp_pdu::{PduResult, decode_err};
use tracing::{debug, trace, warn};

Expand Down Expand Up @@ -1197,8 +1197,12 @@ impl GraphicsPipelineServer {
}
}

/// Compute bounding rectangle from regions
fn compute_dest_rect(regions: &[Avc420Region], default_width: u16, default_height: u16) -> InclusiveRectangle {
/// Compute bounding rectangle from regions.
///
/// Avc420Region uses inclusive bounds; the wire format (RDPGFX_RECT16) is
/// exclusive, so the returned ExclusiveRectangle adds 1 to the max right
/// and bottom of the inclusive bounding box.
fn compute_dest_rect(regions: &[Avc420Region], default_width: u16, default_height: u16) -> ExclusiveRectangle {
if let Some(first) = regions.first() {
let mut left = first.left;
let mut top = first.top;
Expand All @@ -1212,18 +1216,18 @@ impl GraphicsPipelineServer {
bottom = bottom.max(r.bottom);
}

InclusiveRectangle {
ExclusiveRectangle {
left,
top,
right,
bottom,
right: right.saturating_add(1),
bottom: bottom.saturating_add(1),
}
} else {
InclusiveRectangle {
ExclusiveRectangle {
left: 0,
top: 0,
right: default_width.saturating_sub(1),
bottom: default_height.saturating_sub(1),
right: default_width,
bottom: default_height,
}
}
}
Expand Down Expand Up @@ -1385,11 +1389,11 @@ impl GraphicsPipelineServer {
let timestamp = Self::make_timestamp(timestamp_ms);
let frame_id = self.frames.begin_frame(timestamp);

let dest_rect = InclusiveRectangle {
let dest_rect = ExclusiveRectangle {
left: 0,
top: 0,
right: dest_width.saturating_sub(1),
bottom: dest_height.saturating_sub(1),
right: dest_width,
bottom: dest_height,
};

self.output_queue
Expand Down
38 changes: 19 additions & 19 deletions crates/ironrdp-testsuite-core/tests/egfx/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use ironrdp_egfx::pdu::{
DeleteSurfacePdu, EndFramePdu, GfxPdu, PixelFormat, ResetGraphicsPdu, StartFramePdu, Timestamp, WireToSurface1Pdu,
};
use ironrdp_graphics::zgfx::wrap_uncompressed;
use ironrdp_pdu::geometry::InclusiveRectangle;
use ironrdp_pdu::geometry::ExclusiveRectangle;

// ============================================================================
// Test Handler
Expand Down Expand Up @@ -213,11 +213,11 @@ fn client_handles_uncompressed_via_process() {
surface_id: 1,
codec_id: Codec1Type::Uncompressed,
pixel_format: PixelFormat::XRgb,
destination_rectangle: InclusiveRectangle {
destination_rectangle: ExclusiveRectangle {
left: 0,
top: 0,
right: 3,
bottom: 3,
right: 4,
bottom: 4,
},
bitmap_data: vec![0u8; 4 * 4 * 4],
});
Expand Down Expand Up @@ -245,11 +245,11 @@ fn client_dispatches_avc420_via_process() {
surface_id: 1,
codec_id: Codec1Type::Avc420,
pixel_format: PixelFormat::XRgb,
destination_rectangle: InclusiveRectangle {
destination_rectangle: ExclusiveRectangle {
left: 0,
top: 0,
right: 15,
bottom: 15,
right: 16,
bottom: 16,
},
bitmap_data,
});
Expand All @@ -276,11 +276,11 @@ fn client_skips_avc420_without_decoder() {
surface_id: 1,
codec_id: Codec1Type::Avc420,
pixel_format: PixelFormat::XRgb,
destination_rectangle: InclusiveRectangle {
destination_rectangle: ExclusiveRectangle {
left: 0,
top: 0,
right: 15,
bottom: 15,
right: 16,
bottom: 16,
},
bitmap_data,
});
Expand Down Expand Up @@ -310,11 +310,11 @@ fn client_frame_ordering_via_process() {
surface_id: 1,
codec_id: Codec1Type::Uncompressed,
pixel_format: PixelFormat::XRgb,
destination_rectangle: InclusiveRectangle {
destination_rectangle: ExclusiveRectangle {
left: 0,
top: 0,
right: 3,
bottom: 3,
right: 4,
bottom: 4,
},
bitmap_data: vec![0u8; 4 * 4 * 4],
});
Expand Down Expand Up @@ -403,11 +403,11 @@ fn client_rejects_wire_to_unknown_surface() {
surface_id: 99, // does not exist
codec_id: Codec1Type::Uncompressed,
pixel_format: PixelFormat::XRgb,
destination_rectangle: InclusiveRectangle {
destination_rectangle: ExclusiveRectangle {
left: 0,
top: 0,
right: 3,
bottom: 3,
right: 4,
bottom: 4,
},
bitmap_data: vec![0u8; 4 * 4 * 4],
});
Expand All @@ -424,7 +424,7 @@ fn client_rejects_invalid_rectangle_ordering() {
surface_id: 1,
codec_id: Codec1Type::Uncompressed,
pixel_format: PixelFormat::XRgb,
destination_rectangle: InclusiveRectangle {
destination_rectangle: ExclusiveRectangle {
left: 50,
top: 0,
right: 10,
Expand All @@ -447,13 +447,13 @@ fn client_tolerates_out_of_bounds_rectangle() {
surface_id: 1,
codec_id: Codec1Type::Uncompressed,
pixel_format: PixelFormat::XRgb,
destination_rectangle: InclusiveRectangle {
destination_rectangle: ExclusiveRectangle {

@glamberson Greg Lamberson (glamberson) May 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d6e827a. Buffer is now 200 * 50 * 4 to match the exclusive rectangle dimensions.

left: 0,
top: 0,
right: 200, // exceeds surface width of 100
bottom: 50,
},
bitmap_data: vec![0u8; 201 * 51 * 4],
bitmap_data: vec![0u8; 200 * 50 * 4],
});
let result = client.process(0, &encode_for_process(&pdu));
assert!(
Expand Down
Loading