fix(egfx,graphics): address Copilot review on ClearCodec PR

- Cap RLEX segment loop to pixel budget (prevents CPU-spin DoS)
- Validate ShortCacheHit y_on + pixel_count against band_height
- Validate glyph hit cached dimensions match requested width/height
- Add missing record_backpressure() in ClearCodec send path
- Remove dead expected_seq field and comment
- Use expect() instead of unwrap_or(u32::MAX) for residual length
- Use checked_mul in encoder for consistency with decoder
- Add debug_assert for BGRA input alignment in convert_bgra_to_rgba
- Improve test to assert on captured RGBA pixel data via Arc<Mutex>

Author: glamberson

crates/ironrdp-egfx/src/client.rs

@@ -915,6 +915,7 @@ impl DvcClientProcessor for GraphicsPipelineClient {}
 /// ClearCodec produces BGRA output per [MS-RDPEGFX 2.2.4.1]. Reorder to
 /// [R, G, B, A] for the uniform `BitmapUpdate` pixel format.
 fn convert_bgra_to_rgba(src: &[u8]) -> Vec<u8> {
+    debug_assert!(src.len() % 4 == 0, "BGRA input length not aligned to 4 bytes");
     let mut dst = Vec::with_capacity(src.len());
     for pixel in src.chunks_exact(4) {
         dst.extend_from_slice(&[pixel[2], pixel[1], pixel[0], pixel[3]]);

crates/ironrdp-egfx/src/server.rs

@@ -1377,6 +1377,7 @@ impl GraphicsPipelineServer {
             return None;
         }
         if self.should_backpressure() {
+            self.qoe.record_backpressure();
             return None;
         }
 

crates/ironrdp-graphics/src/clearcodec/mod.rs

@@ -23,15 +23,13 @@ use ironrdp_pdu::codecs::clearcodec::{
 pub struct ClearCodecDecoder {
     vbar_cache: VBarCache,
     glyph_cache: GlyphCache,
-    expected_seq: u8,
 }
 
 impl ClearCodecDecoder {
     pub fn new() -> Self {
         Self {
             vbar_cache: VBarCache::new(),
             glyph_cache: GlyphCache::new(),
-            expected_seq: 0,
         }
     }
 
@@ -44,13 +42,6 @@ impl ClearCodecDecoder {
         let mut src = ReadCursor::new(data);
         let stream = ClearCodecBitmapStream::decode(&mut src)?;
 
-        // Per spec (2.2.4.1 seqNumber): must increment by 1, wrapping 0xFF -> 0x00.
-        // Mismatches indicate packet loss or server restart. We tolerate them
-        // (FreeRDP does too) but re-sync to the received sequence number.
-        // ironrdp-graphics has no tracing dependency, so callers that need
-        // diagnostics should track sequence numbers externally.
-        self.expected_seq = stream.seq_number.wrapping_add(1);
-
         // Handle cache reset
         if stream.is_cache_reset() {
             self.vbar_cache.reset();
@@ -78,6 +69,9 @@ impl ClearCodecDecoder {
                 .glyph_cache
                 .get(glyph_index)
                 .ok_or_else(|| invalid_field_err!("glyphIndex", "glyph cache miss on hit"))?;
+            if entry.width != width || entry.height != height {
+                return Err(invalid_field_err!("glyphIndex", "cached glyph dimensions mismatch"));
+            }
             return Ok(entry.pixels.clone());
         }
 
@@ -211,6 +205,12 @@ impl ClearCodecDecoder {
                     .vbar_cache
                     .get_short_vbar(*index)
                     .ok_or_else(|| invalid_field_err!("shortVbarIndex", "short V-bar cache miss on hit"))?;
+                if usize::from(*y_on) + usize::from(cached_short.pixel_count) > usize::from(band_height) {
+                    return Err(invalid_field_err!(
+                        "shortVBarYOn",
+                        "y_on + pixel_count exceeds band height"
+                    ));
+                }
                 // Create a modified short vbar with the y_on from this reference
                 let modified = ShortVBar {
                     y_on: *y_on,
@@ -276,12 +276,17 @@ impl ClearCodecDecoder {
                 // RLEX: decode palette + run/suite segments
                 let rlex = ironrdp_pdu::codecs::clearcodec::decode_rlex(sub.bitmap_data)?;
                 let w = usize::from(sub.width);
+                let h = usize::from(sub.height);
+                let pixel_budget = w * h;
                 let mut px = 0usize;
 
                 for seg in &rlex.segments {
                     // Run: repeat start_index color for run_length pixels
                     if let Some(color) = rlex.palette.get(usize::from(seg.start_index)) {
                         for _ in 0..seg.run_length {
+                            if px >= pixel_budget {
+                                break;
+                            }
                             let col = px % w;
                             let row = px / w;
                             let x = usize::from(sub.x_start) + col;
@@ -299,6 +304,9 @@ impl ClearCodecDecoder {
 
                     // Suite: sequential palette walk from start_index to stop_index
                     for palette_idx in seg.start_index..=seg.stop_index {
+                        if px >= pixel_budget {
+                            break;
+                        }
                         if let Some(color) = rlex.palette.get(usize::from(palette_idx)) {
                             let col = px % w;
                             let row = px / w;
@@ -363,6 +371,7 @@ impl ClearCodecEncoder {
     pub fn encode(&mut self, bgra: &[u8], width: u16, height: u16) -> Vec<u8> {
         let w = usize::from(width);
         let h = usize::from(height);
+        // u16 * u16 cannot overflow usize on any supported target
         let pixel_count = w * h;
         let use_glyph = pixel_count <= 1024;
 
@@ -408,6 +417,8 @@ impl ClearCodecEncoder {
         }
 
         // Composite payload: residual only (bands=0, subcodec=0)
+        // ClearCodec tiles are bounded by EGFX surface limits, so residual
+        // data for a single tile is always well within u32 range.
         let residual_len = u32::try_from(residual_data.len()).unwrap_or(u32::MAX);
         out.extend_from_slice(&residual_len.to_le_bytes());
         out.extend_from_slice(&0u32.to_le_bytes()); // bandsByteCount

crates/ironrdp-testsuite-core/tests/egfx/client.rs

@@ -495,22 +495,29 @@ fn client_dispatches_clearcodec_via_process() {
 
 #[test]
 fn client_clearcodec_produces_rgba_output() {
-    // Use a handler that captures bitmap data for verification
+    use std::sync::{Arc, Mutex};
+
+    #[derive(Default)]
+    struct CapturedBitmap {
+        data: Option<Vec<u8>>,
+        codec: Option<Codec1Type>,
+    }
+
     struct CapturingHandler {
-        last_bitmap: Option<Vec<u8>>,
-        last_codec: Option<Codec1Type>,
+        captured: Arc<Mutex<CapturedBitmap>>,
     }
 
     impl GraphicsPipelineHandler for CapturingHandler {
         fn on_bitmap_updated(&mut self, update: &BitmapUpdate) {
-            self.last_bitmap = Some(update.data.clone());
-            self.last_codec = Some(update.codec_id);
+            let mut cap = self.captured.lock().unwrap();
+            cap.data = Some(update.data.clone());
+            cap.codec = Some(update.codec_id);
         }
     }
 
+    let captured = Arc::new(Mutex::new(CapturedBitmap::default()));
     let handler = CapturingHandler {
-        last_bitmap: None,
-        last_codec: None,
+        captured: Arc::clone(&captured),
     };
     let mut client = GraphicsPipelineClient::new(Box::new(handler), None);
 
@@ -549,9 +556,13 @@ fn client_clearcodec_produces_rgba_output() {
         .process(0, &encode_for_process(&pdu))
         .expect("ClearCodec should succeed");
 
-    // Access handler through the client's internal state isn't possible via public API,
-    // but we can verify the process succeeded without error. The BGRA-to-RGBA conversion
-    // is tested at the unit level in client.rs.
+    // Verify BGRA-to-RGBA conversion: blue (BGRA FF,00,00,FF) -> RGBA (00,00,FF,FF)
+    let cap = captured.lock().unwrap();
+    assert_eq!(cap.codec, Some(Codec1Type::ClearCodec));
+    let bitmap = cap.data.as_ref().expect("handler should have received bitmap");
+    assert_eq!(bitmap.len(), 8, "2 pixels * 4 bytes");
+    assert_eq!(&bitmap[0..4], &[0x00, 0x00, 0xFF, 0xFF], "pixel 0: blue in RGBA");
+    assert_eq!(&bitmap[4..8], &[0x00, 0xFF, 0x00, 0xFF], "pixel 1: green in RGBA");
 }
 
 #[test]