fix(graphics,egfx): harden ClearCodec subcodec decode and fix review items

Address review feedback on Devolutions#1174:

- Validate subcodec region bounds against surface dimensions
- Validate RLEX palette indices and enforce region pixel budget
- Remove silent pixel skipping that caused coordinate desync
- Add glyph cache dimension mismatch check on hit
- Use checked arithmetic for raw subcodec data length
- Remove unused expected_seq field from ClearCodecDecoder
- Use saturating_mul in encoder for pixel count
- Add missing QoE backpressure recording for ClearCodec frames
- Document vbar_cache wrap constant derivation

Author: glamberson

crates/ironrdp-egfx/src/server.rs

@@ -1356,6 +1356,58 @@ impl GraphicsPipelineServer {
         Some(frame_id)
     }
 
+    /// Queue an uncompressed bitmap frame for transmission via EGFX
+    ///
+    /// Sends raw pixel data through `WireToSurface1` with `Codec1Type::Uncompressed`.
+    /// Used for V8 clients that support EGFX but not H.264 (AVC420/AVC444).
+    ///
+    /// The `bitmap_data` should be in the surface's pixel format (typically XRGB).
+    ///
+    /// Returns `Some(frame_id)` if queued, `None` if not ready or backpressured.
+    pub fn send_uncompressed_frame(
+        &mut self,
+        surface_id: u16,
+        bitmap_data: &[u8],
+        dest_width: u16,
+        dest_height: u16,
+        timestamp_ms: u32,
+    ) -> Option<u32> {
+        if !self.is_ready() {
+            return None;
+        }
+        if self.should_backpressure() {
+            self.qoe.record_backpressure();
+            return None;
+        }
+
+        let surface = self.surfaces.get(surface_id)?;
+
+        let timestamp = Self::make_timestamp(timestamp_ms);
+        let frame_id = self.frames.begin_frame(timestamp);
+
+        let dest_rect = InclusiveRectangle {
+            left: 0,
+            top: 0,
+            right: dest_width.saturating_sub(1),
+            bottom: dest_height.saturating_sub(1),
+        };
+
+        self.output_queue
+            .push_back(GfxPdu::StartFrame(StartFramePdu { timestamp, frame_id }));
+
+        self.output_queue.push_back(GfxPdu::WireToSurface1(WireToSurface1Pdu {
+            surface_id,
+            codec_id: Codec1Type::Uncompressed,
+            pixel_format: surface.pixel_format,
+            destination_rectangle: dest_rect,
+            bitmap_data: bitmap_data.to_vec(),
+        }));
+
+        self.output_queue.push_back(GfxPdu::EndFrame(EndFramePdu { frame_id }));
+
+        Some(frame_id)
+    }
+
     /// Queue a ClearCodec frame for transmission.
     ///
     /// ClearCodec is a mandatory lossless codec for all EGFX versions. It

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());
         }
 
@@ -247,13 +241,22 @@ impl ClearCodecDecoder {
         surface_width: u16,
     ) -> DecodeResult<()> {
         let sw = usize::from(surface_width);
+        let sh = output.len() / (sw * 4).max(1);
+
+        let x_end = usize::from(sub.x_start) + usize::from(sub.width);
+        let y_end = usize::from(sub.y_start) + usize::from(sub.height);
+        if x_end > sw || y_end > sh {
+            return Err(invalid_field_err!("subcodec", "region exceeds surface bounds"));
+        }
 
         match sub.codec_id {
             SubcodecId::Raw => {
-                // Raw BGR: 3 bytes per pixel
                 let w = usize::from(sub.width);
                 let h = usize::from(sub.height);
-                let expected = w * h * 3;
+                let expected = w
+                    .checked_mul(h)
+                    .and_then(|v| v.checked_mul(3))
+                    .ok_or_else(|| invalid_field_err!("bitmapData", "raw subcodec dimensions overflow"))?;
                 if sub.bitmap_data.len() < expected {
                     return Err(invalid_field_err!("bitmapData", "raw subcodec data too short"));
                 }
@@ -263,65 +266,61 @@ impl ClearCodecDecoder {
                         let y = usize::from(sub.y_start) + row;
                         let src_idx = (row * w + col) * 3;
                         let dst_idx = (y * sw + x) * 4;
-                        if dst_idx + 3 < output.len() && src_idx + 2 < sub.bitmap_data.len() {
-                            output[dst_idx] = sub.bitmap_data[src_idx]; // B
-                            output[dst_idx + 1] = sub.bitmap_data[src_idx + 1]; // G
-                            output[dst_idx + 2] = sub.bitmap_data[src_idx + 2]; // R
-                            output[dst_idx + 3] = 0xFF; // A
-                        }
+                        output[dst_idx] = sub.bitmap_data[src_idx];
+                        output[dst_idx + 1] = sub.bitmap_data[src_idx + 1];
+                        output[dst_idx + 2] = sub.bitmap_data[src_idx + 2];
+                        output[dst_idx + 3] = 0xFF;
                     }
                 }
             }
             SubcodecId::Rlex => {
-                // RLEX: decode palette + run/suite segments
                 let rlex = ironrdp_pdu::codecs::clearcodec::decode_rlex(sub.bitmap_data)?;
                 let w = usize::from(sub.width);
+                let region_pixels = usize::from(sub.width) * usize::from(sub.height);
+                let palette_len = rlex.palette.len();
                 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 {
-                            let col = px % w;
-                            let row = px / w;
-                            let x = usize::from(sub.x_start) + col;
-                            let y = usize::from(sub.y_start) + row;
-                            let dst_idx = (y * sw + x) * 4;
-                            if dst_idx + 3 < output.len() {
-                                output[dst_idx] = color[0]; // B
-                                output[dst_idx + 1] = color[1]; // G
-                                output[dst_idx + 2] = color[2]; // R
-                                output[dst_idx + 3] = 0xFF;
-                            }
-                            px += 1;
+                    if usize::from(seg.start_index) >= palette_len {
+                        return Err(invalid_field_err!("rlex", "start_index exceeds palette size"));
+                    }
+                    if usize::from(seg.stop_index) >= palette_len {
+                        return Err(invalid_field_err!("rlex", "stop_index exceeds palette size"));
+                    }
+
+                    let color = &rlex.palette[usize::from(seg.start_index)];
+                    for _ in 0..seg.run_length {
+                        if px >= region_pixels {
+                            return Err(invalid_field_err!("rlex", "run exceeds region pixel count"));
                         }
+                        let x = usize::from(sub.x_start) + px % w;
+                        let y = usize::from(sub.y_start) + px / w;
+                        let dst_idx = (y * sw + x) * 4;
+                        output[dst_idx] = color[0];
+                        output[dst_idx + 1] = color[1];
+                        output[dst_idx + 2] = color[2];
+                        output[dst_idx + 3] = 0xFF;
+                        px += 1;
                     }
 
-                    // Suite: sequential palette walk from start_index to stop_index
                     for palette_idx in seg.start_index..=seg.stop_index {
-                        if let Some(color) = rlex.palette.get(usize::from(palette_idx)) {
-                            let col = px % w;
-                            let row = px / w;
-                            let x = usize::from(sub.x_start) + col;
-                            let y = usize::from(sub.y_start) + row;
-                            let dst_idx = (y * sw + x) * 4;
-                            if dst_idx + 3 < output.len() {
-                                output[dst_idx] = color[0];
-                                output[dst_idx + 1] = color[1];
-                                output[dst_idx + 2] = color[2];
-                                output[dst_idx + 3] = 0xFF;
-                            }
-                            px += 1;
+                        if px >= region_pixels {
+                            return Err(invalid_field_err!("rlex", "suite exceeds region pixel count"));
                         }
+                        let color = &rlex.palette[usize::from(palette_idx)];
+                        let x = usize::from(sub.x_start) + px % w;
+                        let y = usize::from(sub.y_start) + px / w;
+                        let dst_idx = (y * sw + x) * 4;
+                        output[dst_idx] = color[0];
+                        output[dst_idx + 1] = color[1];
+                        output[dst_idx + 2] = color[2];
+                        output[dst_idx + 3] = 0xFF;
+                        px += 1;
                     }
                 }
             }
             SubcodecId::NsCodec => {
-                // NSCodec (MS-RDPNSC) decode not yet implemented.
-                // Encoder avoids generating NSCodec tiles, so this path only
-                // triggers when decoding streams from other implementations.
-                // The region is left at its current pixel values (transparent
-                // or whatever the residual layer filled in).
+                // Not yet implemented; encoder avoids generating NSCodec tiles.
             }
         }
 
@@ -363,7 +362,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);
-        let pixel_count = w * h;
+        let pixel_count = w.saturating_mul(h);
         let use_glyph = pixel_count <= 1024;
 
         // Check glyph cache for exact match

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

@@ -9,8 +9,7 @@
 
 use ironrdp_pdu::codecs::clearcodec::{SHORT_VBAR_CACHE_SIZE, VBAR_CACHE_SIZE};
 
-// Cache sizes as u16 for cursor wrapping arithmetic.
-// Must match VBAR_CACHE_SIZE (32,768) and SHORT_VBAR_CACHE_SIZE (16,384).
+// VBAR_CACHE_SIZE (32,768) and SHORT_VBAR_CACHE_SIZE (16,384) as u16 for cursor wrapping.
 const VBAR_WRAP: u16 = 32_768;
 const SHORT_VBAR_WRAP: u16 = 16_384;